[review] Add gdb_compile_openmp to lib/gdb.exp

[review] Add gdb_compile_openmp to lib/gdb.exp

[Kevin Buettner (Code Review)]

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(-)



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

-- 
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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Tom Tromey (Code Review)]

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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Pedro Alves (Code Review)]

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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Simon Marchi (Code Review)]

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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Pedro Alves (Code Review)]

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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Pedro Alves (Code Review)]

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

Re: [review] Add gdb_compile_openmp to lib/gdb.exp

[Simon Marchi]

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)

Re: [review] Add gdb_compile_openmp to lib/gdb.exp

[Simon Marchi]

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
> 

[review] Add gdb_compile_openmp to lib/gdb.exp

[Simon Marchi (Code Review)]

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

Re: [review] Add gdb_compile_openmp to lib/gdb.exp

[Pedro Alves]

[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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Kevin Buettner (Code Review)]

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

[review] Add gdb_compile_openmp to lib/gdb.exp

[Kevin Buettner (Code Review)]

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

Re: [review] Add gdb_compile_openmp to lib/gdb.exp

[Pedro Alves]

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

[review v2] Add gdb_compile_openmp to lib/gdb.exp

[Kevin Buettner (Code Review)]

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, 13 insertions(+), 2 deletions(-)



diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2d395ef..1731a3e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3941,6 +3941,9 @@
 # against several different thread libraries, to see which one this
 # system has.
 proc gdb_compile_pthreads {source dest type options} {
+    if {$type != "executable"} {
+	return [gdb_compile $source $dest $type $options]
+    }
     set built_binfile 0
     set why_msg "unrecognized error"
     foreach lib {-lpthreads -lpthread -lthread ""} {
@@ -4146,6 +4149,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 +5902,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]"
     }
@@ -5926,7 +5937,7 @@
 	    if { ! [regexp "^/" "$s"] } then {
 		set s "$srcdir/$subdir/$s"
 	    }
-	    if  { [gdb_compile "${s}" "${binfile}${i}.o" object $local_options] != "" } {
+	    if  { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" } {
 		untested $testname
 		return -1
 	    }

-- 
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: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset

[review v2] Add gdb_compile_openmp to lib/gdb.exp

[Pedro Alves (Code Review)]

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

[pushed] Add gdb_compile_openmp to lib/gdb.exp

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Kevin Buettner.

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.
	(gdb_compile_pthreads): Add non-executable case.

Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
---
M gdb/testsuite/ChangeLog
M gdb/testsuite/lib/gdb.exp
2 files changed, 19 insertions(+), 2 deletions(-)



diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 52edbc1..d3510d4 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2019-12-10  Kevin Buettner  <kevinb@redhat.com>
+
+            * lib/gdb.exp (gdb_compile_openmp): New proc.
+            (build_executable_from_specs): Add an "openmp" option.
+            (gdb_compile_pthreads): Add non-executable case.
+
 2019-12-10  Tom Tromey  <tromey@adacore.com>
 
 	* gdb.ada/unchecked_union.exp: New file.
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index aa77446..806e5a0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3951,6 +3951,9 @@
 # against several different thread libraries, to see which one this
 # system has.
 proc gdb_compile_pthreads {source dest type options} {
+    if {$type != "executable"} {
+	return [gdb_compile $source $dest $type $options]
+    }
     set built_binfile 0
     set why_msg "unrecognized error"
     foreach lib {-lpthreads -lpthread -lthread ""} {
@@ -4156,6 +4159,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
 
@@ -5901,7 +5912,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]"
     }
@@ -5936,7 +5947,7 @@
 	    if { ! [regexp "^/" "$s"] } then {
 		set s "$srcdir/$subdir/$s"
 	    }
-	    if  { [gdb_compile "${s}" "${binfile}${i}.o" object $local_options] != "" } {
+	    if  { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" } {
 		untested $testname
 		return -1
 	    }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 3
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-MessageType: newpatchset

[pushed] Add gdb_compile_openmp to lib/gdb.exp

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

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.
	(gdb_compile_pthreads): Add non-executable case.

Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
---
M gdb/testsuite/ChangeLog
M gdb/testsuite/lib/gdb.exp
2 files changed, 19 insertions(+), 2 deletions(-)


diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 52edbc1..d3510d4 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2019-12-10  Kevin Buettner  <kevinb@redhat.com>
+
+            * lib/gdb.exp (gdb_compile_openmp): New proc.
+            (build_executable_from_specs): Add an "openmp" option.
+            (gdb_compile_pthreads): Add non-executable case.
+
 2019-12-10  Tom Tromey  <tromey@adacore.com>
 
 	* gdb.ada/unchecked_union.exp: New file.
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index aa77446..806e5a0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3951,6 +3951,9 @@
 # against several different thread libraries, to see which one this
 # system has.
 proc gdb_compile_pthreads {source dest type options} {
+    if {$type != "executable"} {
+	return [gdb_compile $source $dest $type $options]
+    }
     set built_binfile 0
     set why_msg "unrecognized error"
     foreach lib {-lpthreads -lpthread -lthread ""} {
@@ -4156,6 +4159,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
 
@@ -5901,7 +5912,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]"
     }
@@ -5936,7 +5947,7 @@
 	    if { ! [regexp "^/" "$s"] } then {
 		set s "$srcdir/$subdir/$s"
 	    }
-	    if  { [gdb_compile "${s}" "${binfile}${i}.o" object $local_options] != "" } {
+	    if  { [$func "${s}" "${binfile}${i}.o" object $local_options] != "" } {
 		untested $testname
 		return -1
 	    }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101
Gerrit-Change-Number: 503
Gerrit-PatchSet: 3
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-MessageType: merged