[review] Add gdb_compile_openmp to lib/gdb.exp

Message ID gerrit.1572841567000.I94048b8b0940c707ce0529a6bcfa6e4eace49101@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Add gdb_compile_openmp to lib/gdb.exp
Related show

Commit Message

Simon Marchi (Code Review) Nov. 4, 2019, 4:26 a.m.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................

Add gdb_compile_openmp to lib/gdb.exp

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_compile_openmp): New proc.
	(build_executable_from_specs): Add an "openmp" option.

Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
---
M gdb/testsuite/lib/gdb.exp
1 file changed, 24 insertions(+), 1 deletion(-)




-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-MessageType: newchange

Comments

Simon Marchi (Code Review) Nov. 5, 2019, 3:27 p.m. | #1
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1: Code-Review+2

Thank you.  This looks good to me.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 15:27:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 6, 2019, 12:33 a.m. | #2
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 
PS1, Line 5961: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5956 | 	    lappend objects "${binfile}${i}.o"
5957 | 	    incr i
5958 | 	}
5959 | 	set ret [$func $objects "${binfile}" executable $options]
5960 |     }
5961 >     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 
5966 |     return 0

(I wanted to select all lines from 5930 to 5961 to show the whole context in the email notification, but I don't think that's possible, right?)

Do we need both the 

 elseif [string match gdb_compile_openmp $func] {

and the

 } else {

branches?

If you keep just the else branch, and tweak it like your new gdb_compile_openmp branch:

- if { [gdb_compile "${s}" "${binfile}${i}.o" object $local_options] != "" }
+ if { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" }

wouldn't that work the same?  I mean, $func defaults to gdb_compile.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 00:33:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 6, 2019, 4:09 p.m. | #3
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 
PS1, Line 5961: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5956 | 	    lappend objects "${binfile}${i}.o"
5957 | 	    incr i
5958 | 	}
5959 | 	set ret [$func $objects "${binfile}" executable $options]
5960 |     }
5961 >     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 
5966 |     return 0

> (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notification, but I don't think that's possible, right?)


Select the code, like you would in a GUI text editor, then press `c`.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 06 Nov 2019 16:09:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 6, 2019, 4:19 p.m. | #4
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5930 
PS1, Line 5930: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5925 | 	    } else {
5926 | 		lappend sources_path "$srcdir/$subdir/$s"
5927 | 	    }
5928 | 	}
5929 | 	set ret [gdb_compile_rust $sources_path "${binfile}" $options]
5930 >     } elseif [string match gdb_compile_openmp $func] {
5931 > 	set objects {}
5932 > 	set i 0
5933 > 	foreach {s local_options} $args {
5934 > 	    if { ! [regexp "^/" "$s"] } then {
5935 > 		set s "$srcdir/$subdir/$s"
5936 > 	    }
5937 > 	    if  { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" } {
5938 > 		untested $testname
5939 > 		return -1
5940 > 	    }
5941 > 	    lappend objects "${binfile}${i}.o"
5942 > 	    incr i
5943 > 	}
5944 > 	set ret [$func $objects "${binfile}" executable $options]
5945 >     } else {
5946 > 	set objects {}
5947 > 	set i 0
5948 > 	foreach {s local_options} $args {
5949 > 	    if { ! [regexp "^/" "$s"] } then {
5950 > 		set s "$srcdir/$subdir/$s"
5951 > 	    }
5952 > 	    if  { [gdb_compile "${s}" "${binfile}${i}.o" object $local_options] != "" } {
5953 > 		untested $testname
5954 > 		return -1
5955 > 	    }
5956 > 	    lappend objects "${binfile}${i}.o"
5957 > 	    incr i
5958 > 	}
5959 > 	set ret [$func $objects "${binfile}" executable $options]
5960 >     }
5961 |     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 

Testing comment with multiple lines selected.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 06 Nov 2019 16:19:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 6, 2019, 4:24 p.m. | #5
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 
PS1, Line 5961: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5956 | 	    lappend objects "${binfile}${i}.o"
5957 | 	    incr i
5958 | 	}
5959 | 	set ret [$func $objects "${binfile}" executable $options]
5960 |     }
5961 >     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 
5966 |     return 0

> > (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notificatio […]


Nice, that worked!  I think it would be a good idea to mention this in our Gerrit wiki, to invite/remind people do this?



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 06 Nov 2019 16:24:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment
Simon Marchi Nov. 6, 2019, 4:26 p.m. | #6
On 2019-11-06 11:24 a.m., Pedro Alves (Code Review) wrote:
> Pedro Alves has posted comments on this change.

> 

> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503

> ......................................................................

> 

> 

> Patch Set 1:

> 

> (1 comment)

> 

> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 

> File gdb/testsuite/lib/gdb.exp:

> 

> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 

> PS1, Line 5961: 

> 5887 | proc build_executable_from_specs {testname executable options args} {

>      | ...

> 5956 | 	    lappend objects "${binfile}${i}.o"

> 5957 | 	    incr i

> 5958 | 	}

> 5959 | 	set ret [$func $objects "${binfile}" executable $options]

> 5960 |     }

> 5961 >     if  { $ret != "" } {

> 5962 |         untested $testname

> 5963 |         return -1

> 5964 |     }

> 5965 | 

> 5966 |     return 0

> 

>>> (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notificatio […]

> 

> Nice, that worked!  I think it would be a good idea to mention this in our Gerrit wiki, to invite/remind people do this?


Sure!

(This is sent as an email to test replying to comments by email)
Simon Marchi Nov. 6, 2019, 4:28 p.m. | #7
On 2019-11-06 11:24 a.m., Pedro Alves (Code Review) wrote:
> Pedro Alves has posted comments on this change.

> 

> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503

> ......................................................................

> 

> 

> Patch Set 1:

> 

> (1 comment)

> 

> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 

> File gdb/testsuite/lib/gdb.exp:

> 

> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 

> PS1, Line 5961: 

> 5887 | proc build_executable_from_specs {testname executable options args} {

>      | ...

> 5956 | 	    lappend objects "${binfile}${i}.o"

> 5957 | 	    incr i

> 5958 | 	}

> 5959 | 	set ret [$func $objects "${binfile}" executable $options]

> 5960 |     }

> 5961 >     if  { $ret != "" } {

> 5962 |         untested $testname

> 5963 |         return -1

> 5964 |     }

> 5965 | 

> 5966 |     return 0

> 

>>> (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notificatio […]

> 

> Nice, that worked!  I think it would be a good idea to mention this in our Gerrit wiki, to invite/remind people do this?


Attempt #2:

Sure!

(This is sent as an email to test replying to comments by email)

> -- 

> Gerrit-Project: binutils-gdb

> Gerrit-Branch: master

> Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101

> Gerrit-Change-Number: 503

> Gerrit-PatchSet: 1

> Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>

> Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>

> Gerrit-CC: Pedro Alves <palves@redhat.com>

> Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>

> Gerrit-Comment-Date: Wed, 06 Nov 2019 16:24:15 +0000

> Gerrit-HasComments: Yes

> Gerrit-Has-Labels: No

> Comment-In-Reply-To: Pedro Alves <palves@redhat.com>

> Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>

> Gerrit-MessageType: comment

>
Simon Marchi (Code Review) Nov. 6, 2019, 4:29 p.m. | #8
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

On 2019-11-06 11:24 a.m., Pedro Alves (Code Review) wrote:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 
PS1, Line 5961: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5956 | 	    lappend objects "${binfile}${i}.o"
5957 | 	    incr i
5958 | 	}
5959 | 	set ret [$func $objects "${binfile}" executable $options]
5960 |     }
5961 >     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 
5966 |     return 0

> Nice, that worked!  I think it would be a good idea to mention this in our Gerrit wiki, to invite/re […]


>>> (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notificatio […]



Attempt #2:

Sure!

(This is sent as an email to test replying to comments by email)



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 06 Nov 2019 16:29:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment
Pedro Alves Nov. 6, 2019, 4:30 p.m. | #9
[gerrit removed]

On 11/6/19 4:26 PM, Simon Marchi wrote:
> On 2019-11-06 11:24 a.m., Pedro Alves (Code Review) wrote:


>>>> (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notificatio […]

>>

>> Nice, that worked!  I think it would be a good idea to mention this in our Gerrit wiki, to invite/remind people do this?

> 

> Sure!

> 

> (This is sent as an email to test replying to comments by email)


The handling for TABs makes the indentation levels hard to follow, though:

 https://sourceware.org/ml/gdb-patches/2019-11/msg00168.html

Should the email notification replace tabs with 8 spaces?

Thanks,
Pedro Alves
Simon Marchi (Code Review) Nov. 6, 2019, 11:05 p.m. | #10
Kevin Buettner has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5961 
PS1, Line 5961: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5956 | 	    lappend objects "${binfile}${i}.o"
5957 | 	    incr i
5958 | 	}
5959 | 	set ret [$func $objects "${binfile}" executable $options]
5960 |     }
5961 >     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 
5966 |     return 0

> >>> (I wanted to select all lines from 5930 to 5961 to show the whole context in the email notificat […]


The reason I didn't do it that way - i.e. change 'gdb_compile' to '$func" in the "else" clause - is due to the fact that the "pthreads" option uses the the "else" clause.

For "pthreads", I think that we want to call gdb_compile when making the .o files and gdb_compile_pthreads when making the executable. If I make the change that you propose, then gdb_compile_pthreads is used for both executables and objects.

I gave it a try with the substitution that you propose. It does work, but the log file output is somewhat different than before.  Prior to making that substitution, there was a single PASS with message "successfully compiled posix threads test case".  After the substitution, there are now N+1 of these messages, one for each of the N source files, plus one more for the linking phase.

I can still make this change if you want; but if I do this, I think that I should also add some code to gdb_compile_pthreads so that it will call gdb_compile (and also avoid printing the PASS message) when $type is not "executable".

Let me know how you'd like to proceed...



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 06 Nov 2019 23:05:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 6, 2019, 11:14 p.m. | #11
Kevin Buettner has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp 
File gdb/testsuite/lib/gdb.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503/1/gdb/testsuite/lib/gdb.exp@5930 
PS1, Line 5930: 
5887 | proc build_executable_from_specs {testname executable options args} {
     | ...
5925 | 	    } else {
5926 | 		lappend sources_path "$srcdir/$subdir/$s"
5927 | 	    }
5928 | 	}
5929 | 	set ret [gdb_compile_rust $sources_path "${binfile}" $options]
5930 >     } elseif [string match gdb_compile_openmp $func] {
5931 > 	set objects {}
5932 > 	set i 0
5933 > 	foreach {s local_options} $args {
5934 > 	    if { ! [regexp "^/" "$s"] } then {
5935 > 		set s "$srcdir/$subdir/$s"
5936 > 	    }
5937 > 	    if  { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" } {
5938 > 		untested $testname
5939 > 		return -1
5940 > 	    }
5941 > 	    lappend objects "${binfile}${i}.o"
5942 > 	    incr i
5943 > 	}
5944 > 	set ret [$func $objects "${binfile}" executable $options]
5945 >     } else {
5946 > 	set objects {}
5947 > 	set i 0
5948 > 	foreach {s local_options} $args {
5949 > 	    if { ! [regexp "^/" "$s"] } then {
5950 > 		set s "$srcdir/$subdir/$s"
5951 > 	    }
5952 > 	    if  { [gdb_compile "${s}" "${binfile}${i}.o" object $local_options] != "" } {
5953 > 		untested $testname
5954 > 		return -1
5955 > 	    }
5956 > 	    lappend objects "${binfile}${i}.o"
5957 > 	    incr i
5958 > 	}
5959 > 	set ret [$func $objects "${binfile}" executable $options]
5960 >     }
5961 |     if  { $ret != "" } {
5962 |         untested $testname
5963 |         return -1
5964 |     }
5965 | 

> Testing comment with multiple lines selected.


Selecting multiple lines seems to have worked, both in gerrit and in email.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 06 Nov 2019 23:14:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment
Pedro Alves Nov. 8, 2019, 3:05 p.m. | #12
On 11/6/19 11:05 PM, Kevin Buettner (Code Review) wrote:

> For "pthreads", I think that we want to call gdb_compile when 

> making the .o files and gdb_compile_pthreads when making the executable. 

> If I make the change that you propose, then gdb_compile_pthreads is used

> for both executables and objects.


I suppose we could have different $func-like variables for compiling
and linking.

> I can still make this change if you want; but if I do this, I think that

> I should also add some code to gdb_compile_pthreads so that it will

> call gdb_compile (and also avoid printing the PASS message) when

> $type is not "executable".


Hmm, yes, pedantically we're supposed to compile .o files with -pthread also,
not just when linking, as it may enable predefined #defines that are necessary
for correct use of threads, like _REENTRANT, which used to be important for
glibc, though it isn't nowadays, I believe.

On a side note, it seems to me that better than calling gdb_compile_pthreads
repeatedly, it'd be better if we called a gdb_caching_proc that probed which
variant of -pthread, -lpthread, etc. is necessary and returned it, so that
build_executable_from_specs would just append the option to the set of options.
That would make gdb_compile_shlib_pthreads unnecessary, for example.  In general
these gdb_compile_foo functions been like a worse idea than options to me, since
they don't allow combinations.

Thanks,
Pedro Alves
Simon Marchi (Code Review) Dec. 3, 2019, 4:40 p.m. | #13
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503
......................................................................


Patch Set 2: Code-Review+2


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 2
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 03 Dec 2019 16:40:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2d395ef..48d19f9 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4146,6 +4146,14 @@ 
     }
 }
 
+# Build an OpenMP program from SOURCE.  See prefatory comment for
+# gdb_compile, above, for discussion of the parameters to this proc.
+
+proc gdb_compile_openmp {source dest type options} {
+    lappend options "additional_flags=-fopenmp"
+    return [gdb_compile $source $dest $type $options]
+}
+
 # Send a command to GDB.
 # For options for TYPE see gdb_stdin_log_write
 
@@ -5891,7 +5899,7 @@ 
     }
 
     set func gdb_compile
-    set func_index [lsearch -regexp $options {^(pthreads|shlib|shlib_pthreads)$}]
+    set func_index [lsearch -regexp $options {^(pthreads|shlib|shlib_pthreads|openmp)$}]
     if {$func_index != -1} {
 	set func "${func}_[lindex $options $func_index]"
     }
@@ -5919,6 +5927,21 @@ 
 	    }
 	}
 	set ret [gdb_compile_rust $sources_path "${binfile}" $options]
+    } elseif [string match gdb_compile_openmp $func] {
+	set objects {}
+	set i 0
+	foreach {s local_options} $args {
+	    if { ! [regexp "^/" "$s"] } then {
+		set s "$srcdir/$subdir/$s"
+	    }
+	    if  { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" } {
+		untested $testname
+		return -1
+	    }
+	    lappend objects "${binfile}${i}.o"
+	    incr i
+	}
+	set ret [$func $objects "${binfile}" executable $options]
     } else {
 	set objects {}
 	set i 0