From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 899A73858D39 for ; Wed, 24 May 2023 17:33:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 899A73858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1684949600; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=jzYx0+u4ks8mZevZYFlTMi6420wlG+tIPlM+JPaN7IQ=; b=GCUXUnN08/+FTUQmWSyv9lTQaERkcgMbC2w0dguOEbF+lFhWDZ7VLvgPGgt5eyMaciPZsa x3P4DmF/h7ccSehhi3EVpEzNJxxeRjzv+PNUfHs7BMsBoDnInf6M85xsY6fDmm+sJisetD aqkc0yyonQevzfHOZpGlMm+ieTEflHA= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-114-FeHITkD1P6qxg5Kl1HN1Mg-1; Wed, 24 May 2023 13:33:18 -0400 X-MC-Unique: FeHITkD1P6qxg5Kl1HN1Mg-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-3f5ec8aac77so8062055e9.3 for ; Wed, 24 May 2023 10:33:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684949597; x=1687541597; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=jzYx0+u4ks8mZevZYFlTMi6420wlG+tIPlM+JPaN7IQ=; b=enhiaznrX+HwqKxTpY0cZfBuvVES0Wf4kyKqe8xSPy2oa3ViqrnnWWXuTHXMLWkdwZ KKMxypC2zKTXxWPVEYVUWZanbmqfV6lEzhrl7Yi/Q6Unhh5IgioKBMVP+R7920cNZEjP 3RmHVXDcYHlJJtuzgYOsEzoi+BMRTyplzdqn0cF1A64gMa6AkFkZjqp0z9a4jSUO8Z94 i8N+6A1VsbNoPxrvnr+TacxnRrGOP9jgxVEGAG0nxseS04SRPb3J6zceshQsLlYXRm/9 DntdpWZ35f5+Nzs+0ORcTGfURT6yMXBz29GpcJhP9yoKJe21rW0b7NwFfYir0AzKC4sF NjdQ== X-Gm-Message-State: AC+VfDxPPvA3KC4ykSv5UgIOwGBpUfa0O/Y6p+hv86O8M5S8FTETeWWV kx7ThF+FBjjS+GrL0VACqNUDYdfYwlj0rLvwNIJVdXxOc6O1c5g84R30853E7K0XwzXXA8QwSyp dULqqggEGnZqnxZOf/UvwJdo3yFt3eg== X-Received: by 2002:a7b:cbd2:0:b0:3f6:e42:8f85 with SMTP id n18-20020a7bcbd2000000b003f60e428f85mr341404wmi.37.1684949597154; Wed, 24 May 2023 10:33:17 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6zRaViP5ifuyUcxlqn/5Wusj90rc3ZAle9L5ozQxvdggdncnRsgxh0xT21HGgYHcK+wlOEUA== X-Received: by 2002:a7b:cbd2:0:b0:3f6:e42:8f85 with SMTP id n18-20020a7bcbd2000000b003f60e428f85mr341387wmi.37.1684949596699; Wed, 24 May 2023 10:33:16 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id z23-20020a1c4c17000000b003f18b942338sm3032006wmf.3.2023.05.24.10.33.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 May 2023 10:33:16 -0700 (PDT) From: Andrew Burgess To: Matti Puputti , gdb-patches@sourceware.org Subject: Re: [PATCH v3 1/1] gdb, infcmd: Support jump command in multi-inferior case. In-Reply-To: <20230425133819.3524820-1-matti.puputti@intel.com> References: <20230425133819.3524820-1-matti.puputti@intel.com> Date: Wed, 24 May 2023 18:33:14 +0100 Message-ID: <87wn0x63dx.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Matti Puputti writes: > Fixes the issue where jump failed if multiple inferiors run the same source. > > See the below example > > $ gdb -q ./simple > Reading symbols from ./simple... > (gdb) break 2 > Breakpoint 1 at 0x114e: file simple.c, line 2. > (gdb) run > Starting program: /temp/simple > > Breakpoint 1, main () at simple.c:2 > 2 int a = 42; > (gdb) add-inferior > [New inferior 2] > Added inferior 2 on connection 1 (native) > (gdb) inferior 2 > [Switching to inferior 2 [] ()] > (gdb) info inferiors > Num Description Connection Executable > 1 process 6250 1 (native) /temp/simple > * 2 1 (native) > (gdb) file ./simple > Reading symbols from ./simple... > (gdb) run > Starting program: /temp/simple > > Thread 2.1 "simple" hit Breakpoint 1, main () at simple.c:2 > 2 int a = 42; > (gdb) info inferiors > Num Description Connection Executable > 1 process 6250 1 (native) /temp/simple > * 2 process 6705 1 (native) /temp/simple > (gdb) jump 3 > Unreasonable jump request > (gdb) > > In this example, jump fails because the debugger finds two different > locations, one for each inferior. > Solution is to limit the search to the current program space. > --- > gdb/infcmd.c | 3 +- > gdb/linespec.c | 7 +- > gdb/linespec.h | 5 +- > gdb/testsuite/gdb.base/jump.exp | 177 +++++++++++++++++--------------- > 4 files changed, 103 insertions(+), 89 deletions(-) > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 103899432f7..a79b5d3d4d6 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -1077,7 +1077,8 @@ jump_command (const char *arg, int from_tty) > error_no_arg (_("starting address")); > > std::vector sals > - = decode_line_with_last_displayed (arg, DECODE_LINE_FUNFIRSTLINE); > + = decode_line_with_last_displayed (arg, DECODE_LINE_FUNFIRSTLINE, > + current_program_space); I wonder if we should stop using decode_line_with_last_displayed here, and instead use decode_line_with_current_source? It feels like a jump should always be relative to the current location, rather than what we happen to have listed. > if (sals.size () != 1) > error (_("Unreasonable jump request")); > > diff --git a/gdb/linespec.c b/gdb/linespec.c > index 7d969f37fbf..920fd3d37b7 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -3231,7 +3231,8 @@ decode_line_with_current_source (const char *string, int flags) > /* See linespec.h. */ > > std::vector > -decode_line_with_last_displayed (const char *string, int flags) > +decode_line_with_last_displayed (const char *string, int flags, > + program_space *search_pspace) > { > if (string == 0) > error (_("Empty line specification.")); > @@ -3240,10 +3241,10 @@ decode_line_with_last_displayed (const char *string, int flags) > current_language); > std::vector sals > = (last_displayed_sal_is_valid () > - ? decode_line_1 (locspec.get (), flags, NULL, > + ? decode_line_1 (locspec.get (), flags, search_pspace, > get_last_displayed_symtab (), > get_last_displayed_line ()) This change is why I started looking at the details of this patch again. I'm not sure this change makes sense, I would have expected us to use get_last_displayed_pspace () here instead of search_pspace, but then it becomes harder to describe what search_pspace does as it would only be used below. It we switch jump to use decode_line_with_current_source then I think this problem goes away, the changes to decode_line_with_last_displayed can be dropped. At the end of this email you'll find a patch that applies on top of your V3, which implements the change I'm suggesting. This passes all the 'jump' tests I could find in the testsuite, so I think it's fine, but it would be great to hear your thoughts. Thanks, Andrew > - : decode_line_1 (locspec.get (), flags, NULL, NULL, 0)); > + : decode_line_1 (locspec.get (), flags, search_pspace, NULL, 0)); > > if (*string) > error (_("Junk at end of line specification: %s"), string); > diff --git a/gdb/linespec.h b/gdb/linespec.h > index d5e7334fe2d..0eb9cb5d9f2 100644 > --- a/gdb/linespec.h > +++ b/gdb/linespec.h > @@ -139,10 +139,11 @@ extern std::vector decode_line_with_current_source > (const char *, int); > > /* Given a string, return the line specified by it, using the last displayed > - codepoint's values as defaults, or nothing if they aren't valid. */ > + codepoint's values as defaults, or nothing if they aren't valid. > + Limit the search to given program space, if specified. */ > > extern std::vector decode_line_with_last_displayed > - (const char *, int); > + (const char *, int, program_space *search_pspace = nullptr); > > /* Does P represent one of the keywords? If so, return > the keyword. If not, return NULL. */ > diff --git a/gdb/testsuite/gdb.base/jump.exp b/gdb/testsuite/gdb.base/jump.exp > index 032c4a6279d..4c0d4ed2dbb 100644 > --- a/gdb/testsuite/gdb.base/jump.exp > +++ b/gdb/testsuite/gdb.base/jump.exp > @@ -18,99 +18,110 @@ clear_xfail "*-*-*" > > standard_testfile .c > > -# Build the test case > -if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } { > - untested "failed to compile" > - return -1 > - } > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { > + return -1 > +} > > > -# Start with a fresh gdb > +proc do_tests {} { > + global decimal srcfile > + > + # Set a breakpoint on the statement that we're about to jump to. > + # The statement doesn't contain a function call. > + set non_call_line [gdb_get_line_number "bp-on-non-call"] > + gdb_breakpoint "$non_call_line" > + set bp_on_non_call \ > + [get_integer_valueof "\$bpnum" "INVALID" "bp_on_non_call"] > + > + # Can we jump to the statement? Do we stop there? > + gdb_test "jump $non_call_line" \ > + "Breakpoint ${bp_on_non_call}(\.${decimal})?, .*${srcfile}:$non_call_line.*" \ > + "jump to non-call" > + > + # Set a breakpoint on the statement that we're about to jump to. > + # The statement does contain a function call. > + set call_line [gdb_get_line_number "bp-on-call"] > + gdb_breakpoint "$call_line" > + set bp_on_call [get_integer_valueof "\$bpnum" "INVALID" "bp_on_call"] > + > + # Can we jump to the statement? Do we stop there? > + gdb_test "jump $call_line" \ > + "Breakpoint ${bp_on_call}(\.${decimal})?, .*${srcfile}:$call_line.*" \ > + "jump to call" > + > + # If we disable the breakpoint at the function call, and then > + # if we jump to that statement, do we not stop there, but at > + # the following breakpoint? > + gdb_test_no_output "disable $bp_on_call" "disable breakpoint on call" > + > + gdb_test "jump $call_line" \ > + "Breakpoint ${bp_on_non_call}(\.${decimal})?, .*${srcfile}:$non_call_line.*" \ > + "jump to call with disabled breakpoint" > + > + # Disable the breakpoint at the non-function call, so it won't hit > + # if do_test is called again. > + gdb_test_no_output "disable ${bp_on_non_call}" "disable bp_on_non_call" > + > + # Verify that GDB responds gracefully to the "jump" command without > + # an argument. > + gdb_test "jump" "Argument required .starting address.*" \ > + "jump without argument disallowed" > + > + # Verify that GDB responds gracefully to the "jump" command with > + # trailing junk. > + gdb_test "jump $call_line 100" \ > + "malformed linespec error: unexpected number, \"100\"" \ > + "jump with trailing argument junk" > + > + # Verify that GDB responds gracefully to a request to jump out of > + # the current function. (Note that this will very likely cause the > + # inferior to die. Be prepared to rerun the inferior, if further > + # testing is desired.) > + # Try it both ways: confirming and not confirming the jump. > + set out_line [gdb_get_line_number "out-of-func"] > + gdb_test "jump $out_line" \ > + "Not confirmed.*" \ > + "aborted jump out of current function" \ > + "Line $out_line is not in `main'. Jump anyway.*y or n. $" \ > + "n" > + > + gdb_test "jump $out_line" \ > + "Continuing at.*" \ > + "jump out of current function" \ > + "Line $out_line is not in `main'. Jump anyway.*y or n. $" \ > + "y" > +} > > -clean_restart ${binfile} > > -if {![runto_main]} { > - return -1 > +set inferiors 1 > +if {![use_gdb_stub]} { > + set inferiors 2 > } > > -# Set a breakpoint on the statement that we're about to jump to. > -# The statement doesn't contain a function call. > -# > -set bp_on_non_call 0 > -set non_call_line [gdb_get_line_number "bp-on-non-call"] > -gdb_test_multiple "break $non_call_line" "break before jump to non-call" { > - -re "\[Bb\]reakpoint (${decimal}) at ${hex}: file .*${srcfile}, line $non_call_line.*$gdb_prompt $" { > - set bp_on_non_call $expect_out(1,string) > - pass "break before jump to non-call" > +# Run to main, add inferiors if needed. > +for {set inf 1} {$inf <= $inferiors} {incr inf} { > + if {$inf != 1} { > + # Start a new inferior, and run it with the same executable. > + gdb_test "add-inferior -exec ${binfile}" \ > + "Added inferior ${inf}.*" \ > + "add inferior ${inf} with -exec " > + gdb_test "inferior ${inf}" \ > + "Switching to inferior ${inf} .*" \ > + "switch to inferior ${inf}" > + } > + if {![runto_main]} { > + return -1 > } > } > > -# Can we jump to the statement? Do we stop there? > -# > -gdb_test "jump $non_call_line" "Breakpoint ${decimal}, .*${srcfile}:$non_call_line.*" \ > - "jump to non-call" > - > -# Set a breakpoint on the statement that we're about to jump to. > -# The statement does contain a function call. > -# > -set bp_on_call 0 > -set call_line [gdb_get_line_number "bp-on-call"] > -gdb_test_multiple "break $call_line" "break before jump to call" { > - -re "\[Bb\]reakpoint (${decimal}) at ${hex}: file .*${srcfile}, line $call_line.*$gdb_prompt $" { > - set bp_on_call $expect_out(1,string) > - pass "break before jump to call" > +# Run tests on all inferiors. > +for {set inf 1} {$inf <= $inferiors} {incr inf} { > + with_test_prefix "inferior $inf" { > + # Switch to the target inferior. > + gdb_test "inferior $inf" ".*Switching to inferior $inf .*" > + # Run the tests. > + do_tests > } > } > > -# Can we jump to the statement? Do we stop there? > -# > -gdb_test "jump $call_line" \ > - "Breakpoint ${decimal}, .*${srcfile}:$call_line.*" \ > - "jump to call" > - > -# If we disable the breakpoint at the function call, and then > -# if we jump to that statement, do we not stop there, but at > -# the following breakpoint? > -# > -gdb_test_no_output "disable $bp_on_call" "disable breakpoint on call" > - > -gdb_test "jump $call_line" "Breakpoint ${decimal}, .*${srcfile}:$non_call_line.*" \ > - "jump to call with disabled breakpoint" > - > -# Verify that GDB responds gracefully to the "jump" command without > -# an argument. > -# > -gdb_test "jump" "Argument required .starting address.*" \ > - "jump without argument disallowed" > - > - > -# Verify that GDB responds gracefully to the "jump" command with > -# trailing junk. > -# > -gdb_test "jump $call_line 100" \ > - "malformed linespec error: unexpected number, \"100\"" \ > - "jump with trailing argument junk" > - > - > -# Verify that GDB responds gracefully to a request to jump out of > -# the current function. (Note that this will very likely cause the > -# inferior to die. Be prepared to rerun the inferior, if further > -# testing is desired.) > -# > -# Try it both ways: confirming and not confirming the jump. > -# > - > -set out_line [gdb_get_line_number "out-of-func"] > -gdb_test "jump $out_line" \ > - "Not confirmed.*" \ > - "aborted jump out of current function" \ > - "Line $out_line is not in `main'. Jump anyway.*y or n. $" \ > - "n" > - > -gdb_test "jump $out_line" \ > - "Continuing at.*" \ > - "jump out of current function" \ > - "Line $out_line is not in `main'. Jump anyway.*y or n. $" \ > - "y" > - > gdb_exit > -- > 2.25.1 > > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928 --- diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 1be72ed09f0..1d10e2908ca 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -1068,8 +1068,7 @@ jump_command (const char *arg, int from_tty) error_no_arg (_("starting address")); std::vector sals - = decode_line_with_last_displayed (arg, DECODE_LINE_FUNFIRSTLINE, - current_program_space); + = decode_line_with_current_source (arg, DECODE_LINE_FUNFIRSTLINE); if (sals.size () != 1) error (_("Unreasonable jump request")); diff --git a/gdb/linespec.c b/gdb/linespec.c index 920fd3d37b7..9f03a4b3604 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -3220,7 +3220,8 @@ decode_line_with_current_source (const char *string, int flags) location_spec_up locspec = string_to_location_spec (&string, current_language); std::vector sals - = decode_line_1 (locspec.get (), flags, NULL, cursal.symtab, cursal.line); + = decode_line_1 (locspec.get (), flags, cursal.pspace, cursal.symtab, + cursal.line); if (*string) error (_("Junk at end of line specification: %s"), string); @@ -3231,8 +3232,7 @@ decode_line_with_current_source (const char *string, int flags) /* See linespec.h. */ std::vector -decode_line_with_last_displayed (const char *string, int flags, - program_space *search_pspace) +decode_line_with_last_displayed (const char *string, int flags) { if (string == 0) error (_("Empty line specification.")); @@ -3241,10 +3241,10 @@ decode_line_with_last_displayed (const char *string, int flags, current_language); std::vector sals = (last_displayed_sal_is_valid () - ? decode_line_1 (locspec.get (), flags, search_pspace, + ? decode_line_1 (locspec.get (), flags, NULL, get_last_displayed_symtab (), get_last_displayed_line ()) - : decode_line_1 (locspec.get (), flags, search_pspace, NULL, 0)); + : decode_line_1 (locspec.get (), flags, NULL, NULL, 0)); if (*string) error (_("Junk at end of line specification: %s"), string); diff --git a/gdb/linespec.h b/gdb/linespec.h index 0eb9cb5d9f2..c0eef7894d9 100644 --- a/gdb/linespec.h +++ b/gdb/linespec.h @@ -143,7 +143,7 @@ extern std::vector decode_line_with_current_source Limit the search to given program space, if specified. */ extern std::vector decode_line_with_last_displayed - (const char *, int, program_space *search_pspace = nullptr); + (const char *, int); /* Does P represent one of the keywords? If so, return the keyword. If not, return NULL. */