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 DFB6D3858296 for ; Tue, 21 Feb 2023 16:56:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DFB6D3858296 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=1676998591; 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=g3t8ubzuGj5/dL2INhwd17nl4+mFhBTMLKS/RvPOOJU=; b=WytBObtgPFH1fBTlD1p0CD7brS+VM7RaBYSnqJscPXQi9usRjaFd5xaVcIGMcLVvdJ1uO/ pJr7LfsIT3dsTwtnPc1+WLFA3B5EBaAPj6VX5Ni5c1ucfHCHDTH0g8oM9rSwllZabbzjfn L7Ug9UPZEusL48k19Ey+TK7/KHZf+Qc= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-138-wZmNNZkvOPSHhnSQXnGoTg-1; Tue, 21 Feb 2023 11:56:30 -0500 X-MC-Unique: wZmNNZkvOPSHhnSQXnGoTg-1 Received: by mail-wm1-f70.google.com with SMTP id z6-20020a7bc7c6000000b003e0107732f4so2277527wmk.1 for ; Tue, 21 Feb 2023 08:56:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=g3t8ubzuGj5/dL2INhwd17nl4+mFhBTMLKS/RvPOOJU=; b=FxE2oKqTQkyJUD5zt85XD5kuG2IDK6UlKSqQ4aP2lY5owLxwbVGyGFg56CGPCAb2le 7pGDYYVSx1rFJLCY+n4mLYBomV+3m2RMRlOw6X519UBuPa8tVXKRDhpwmQkWmLgGv6F4 kt3qYUd1vHVhMNYAV1amjMVD9Iw45dpa1ygchw1nM87NSA+2q5eq7ytwZgTbHQb90FGX nNDjoRjIrMdxLUCed4lBLCpsT9TtbaUDWrUEvJ/o7yBrezJcC4Rjb6Syxy8opgVW9iHz g/SKetfij7G8acMgDpiCt5Zm2DYloax+hsS3IXUkQYz+UjEK2f2C9ZSYkFayrLfOxzwW yXCg== X-Gm-Message-State: AO0yUKVLWgXe1ALYNPhsXbbFRM73fDg8hZ6Os75FV63rp7puvgzuoHAo Vc54rOLd8BV02iSSLRwnmfx8YqAgVK5WHTRk3UVhvgzGk6FhfyhEQOAzpkOS8NEXa49k75Tydi4 R6iXiGX3hxXhG2E01dl0YfYT2mNs= X-Received: by 2002:a05:6000:1819:b0:2c5:617d:43a3 with SMTP id m25-20020a056000181900b002c5617d43a3mr3868742wrh.71.1676998588685; Tue, 21 Feb 2023 08:56:28 -0800 (PST) X-Google-Smtp-Source: AK7set8CkFu85vX+o08E4Wu2EwUfvGUVZ6r+UNrnp9H8jS8sDwr4sdDTdLJ8QDWdIqq8uPEavTtE6w== X-Received: by 2002:a05:6000:1819:b0:2c5:617d:43a3 with SMTP id m25-20020a056000181900b002c5617d43a3mr3868732wrh.71.1676998588315; Tue, 21 Feb 2023 08:56:28 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id o2-20020a5d4742000000b002c59c6abc10sm5618932wrs.115.2023.02.21.08.56.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Feb 2023 08:56:27 -0800 (PST) From: Andrew Burgess To: Matti Puputti , gdb-patches@sourceware.org Subject: Re: [PATCH 1/1] gdb, infcmd: Support jump command in multi-inferior case. In-Reply-To: <20230221151355.3566249-1-matti.puputti@intel.com> References: <20230221151355.3566249-1-matti.puputti@intel.com> Date: Tue, 21 Feb 2023 16:56:26 +0000 Message-ID: <87ttzfj6th.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 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 via Gdb-patches 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 | 2 +- > gdb/testsuite/gdb.base/jump.exp | 186 +++++++++++++++++++------------- > 4 files changed, 116 insertions(+), 82 deletions(-) > mode change 100644 => 100755 gdb/testsuite/gdb.base/jump.exp > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index fd88b8ca328..2f981cc895f 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -1078,7 +1078,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); > if (sals.size () != 1) > error (_("Unreasonable jump request")); > > diff --git a/gdb/linespec.c b/gdb/linespec.c > index d3def7ae070..5969c46e685 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 ()) > - : 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..70e04cbbb21 100644 > --- a/gdb/linespec.h > +++ b/gdb/linespec.h > @@ -142,7 +142,7 @@ extern std::vector decode_line_with_current_source > codepoint's values as defaults, or nothing if they aren't valid. */ > > extern std::vector decode_line_with_last_displayed > - (const char *, int); > + (const char *, int, program_space *search_pspace = nullptr); The comment above this declaration should be updated to mention the new parameter and explain how to use it. I notice there's only two uses of decode_line_with_last_displayed, the other being in the 'info line' function. I don't think it's something to do in this patch; but I wonder if it would make sense to update that call to restrict to the current program space too? If I say: (gdb) info line 3 I'm probably asking about the current inferior.. Have you thought about this at all? > > /* 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 > old mode 100644 > new mode 100755 > index 032c4a6279d..1040f6355a5 > --- a/gdb/testsuite/gdb.base/jump.exp > +++ b/gdb/testsuite/gdb.base/jump.exp > @@ -25,92 +25,124 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb > } > > > -# Start with a fresh gdb > - > -clean_restart ${binfile} I know this is not your fault; the test is clearly pretty old, but it would be great, if instead of moving this clean_restart around we instead updated the test to use prepare_for_testing. > - > -if {![runto_main]} { > - return -1 > -} > - > -# 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" > +proc do_tests {} { > + global decimal hex srcfile gdb_prompt > + # 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}: .*${srcfile}.*$non_call_line.*$gdb_prompt $" { > + set bp_on_non_call $expect_out(1,string) > + pass "break before jump to non-call" Similarly, here, instead of tweaking the pattern, this can all be replaced with: # 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"] which I think is much clearer. > + } > } > -} > > -# 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" > + # Can we jump to the statement? Do we stop there? > + # > + gdb_test "jump $non_call_line" "Breakpoint ${decimal}(\.${decimal})?, .*${srcfile}:$non_call_line.*" \ > + "jump to non-call" We collected bp_on_non_call above, but we never actually use it. Again, this is not your fault, this was just not a great test to begin with, but we could change this pattern here to replace the first $decimal with $bp_on_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}: .*${srcfile}.*$call_line.*$gdb_prompt $" { > + set bp_on_call $expect_out(1,string) > + pass "break before jump to call" This can be cleaned up like the earlier one. > + } > } > -} > > -# 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" > + # Can we jump to the statement? Do we stop there? > + # > + gdb_test "jump $call_line" \ > + "Breakpoint ${decimal}(\.${decimal})?, .*${srcfile}:$call_line.*" \ > + "jump to call" And we can use $bp_on_call to replace the first $decimal here. Thanks, Andrew > + > + # 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}(\.${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" > +} > > -# Verify that GDB responds gracefully to the "jump" command without > -# an argument. > -# > -gdb_test "jump" "Argument required .starting address.*" \ > - "jump without argument disallowed" > > +# Start with a fresh gdb. > > -# 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" > +clean_restart ${binfile} > > +set inferiors 1 > +if {![use_gdb_stub]} { > + set inferiors 2 > +} > > -# 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. > -# > +# 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] then { > + perror "Couldn't run inferior ${inf} to main" > + return -1 > + } > +} > > -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" > +# 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 > + } > +} > > 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