Message ID | gerrit.1572841567000.I94048b8b0940c707ce0529a6bcfa6e4eace49101@gnutoolchain-gerrit.osci.io |
---|---|
State | New, archived |
Headers |
Received: (qmail 120172 invoked by alias); 4 Nov 2019 04:26:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 120163 invoked by uid 89); 4 Nov 2019 04:26:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=fopenmp, HContent-Transfer-Encoding:8bit X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 04 Nov 2019 04:26:10 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 2CDC320407; Sun, 3 Nov 2019 23:26:09 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [8.43.85.239]) by mx1.osci.io (Postfix) with ESMTP id D715820322 for <gdb-patches@sourceware.org>; Sun, 3 Nov 2019 23:26:07 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id B4D9425B28 for <gdb-patches@sourceware.org>; Sun, 3 Nov 2019 23:26:07 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Sun, 3 Nov 2019 23:26:07 -0500 From: "Kevin Buettner (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io> To: gdb-patches@sourceware.org Message-ID: <gerrit.1572841567000.I94048b8b0940c707ce0529a6bcfa6e4eace49101@gnutoolchain-gerrit.osci.io> Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange Subject: [review] Add gdb_compile_openmp to lib/gdb.exp X-Gerrit-Change-Id: I94048b8b0940c707ce0529a6bcfa6e4eace49101 X-Gerrit-Change-Number: 503 X-Gerrit-ChangeURL: <https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/503> X-Gerrit-Commit: d50e7cd8527c61b307f06a048dcfb6e8647b9149 References: <gerrit.1572841567000.I94048b8b0940c707ce0529a6bcfa6e4eace49101@gnutoolchain-gerrit.osci.io> Reply-To: kevinb@redhat.com, gdb-patches@sourceware.org MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8 |
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
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.
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 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`.
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.
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?
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)
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 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 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
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...
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.
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
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 --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