diff mbox

[review] Add gdb_compile_openmp to lib/gdb.exp

Message ID gerrit.1572841567000.I94048b8b0940c707ce0529a6bcfa6e4eace49101@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 4, 2019, 4:26 a.m. UTC 
  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(-)

Comments

Simon Marchi (Code Review) Nov. 5, 2019, 3:27 p.m. UTC | #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.
Simon Marchi (Code Review) Nov. 6, 2019, 12:33 a.m. UTC | #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.
Simon Marchi (Code Review) Nov. 6, 2019, 4:09 p.m. UTC | #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`.
Simon Marchi (Code Review) Nov. 6, 2019, 4:19 p.m. UTC | #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.
Simon Marchi (Code Review) Nov. 6, 2019, 4:24 p.m. UTC | #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?
Simon Marchi Nov. 6, 2019, 4:26 p.m. UTC | #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. UTC | #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. UTC | #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)
Pedro Alves Nov. 6, 2019, 4:30 p.m. UTC | #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. UTC | #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...
Simon Marchi (Code Review) Nov. 6, 2019, 11:14 p.m. UTC | #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.
Pedro Alves Nov. 8, 2019, 3:05 p.m. UTC | #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. UTC | #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
diff mbox

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