RE: [PATCH v2 1/1] gdb, infcmd: Support jump command in multi-inferior case.

["Puputti, Matti" <matti.puputti@intel.com>]

Hi Andrew,

Thank you for your review and (conditional) approval.
I have uploaded new patch to address your comments. In addition I did my
best to clean out some other empty lines and trailing '#' lines, too.
And finally, I updated the commit date.
I hope the new patch is good.

Patch 3: https://sourceware.org/pipermail/gdb-patches/2023-April/199095.html

As you surely have noticed, this is my very first patch series landing in GDB.
If you are ok with the latest patch, can you push it for me, as I don't have
write access?

I plan to upstream more patches in the future and would like to have
"write after approval" access.  Can you help me with that as well? 

Br,
Matti Puputti

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Monday, April 24, 2023 4:11 PM
> To: Puputti, Matti <matti.puputti@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 1/1] gdb, infcmd: Support jump command in multi-
> inferior case.
> 
> Matti Puputti <matti.puputti@intel.com> 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 [<null>] (<noexec>)]
> >     (gdb) info inferiors
> >       Num  Description       Connection           Executable
> >       1    process 6250      1 (native)           /temp/simple
> >     * 2    <null>            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.
> 
> Thanks for the fixes.  I have a couple of minor nits but I think this is
> pretty much OK.
> 
> > ---
> >  gdb/infcmd.c                    |   3 +-
> >  gdb/linespec.c                  |   7 +-
> >  gdb/linespec.h                  |   5 +-
> >  gdb/testsuite/gdb.base/jump.exp | 193 ++++++++++++++++++-------------
> -
> >  4 files changed, 119 insertions(+), 89 deletions(-)
> >
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index a851fe1f8c8..18a537847cf 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<symtab_and_line> 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 36f2ef46a7c..536636851e7 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<symtab_and_line>
> > -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<symtab_and_line> 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..0eb9cb5d9f2 100644
> > --- a/gdb/linespec.h
> > +++ b/gdb/linespec.h
> > @@ -139,10 +139,11 @@ extern std::vector<symtab_and_line>
> 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<symtab_and_line> 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..ca214993025 100644
> > --- a/gdb/testsuite/gdb.base/jump.exp
> > +++ b/gdb/testsuite/gdb.base/jump.exp
> > @@ -18,99 +18,126 @@ 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 bp_on_non_call 0
> 
> This line is now redundant as we unconditionally set a value below.
> 
> > +    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"]
> 
> This line is a bit long.  Please wrap it somewhere.
> 
> > +
> > +    # 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 bp_on_call 0
> 
> Again this line can be removed.
> 
> > +    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 be
> > +    # there if do_test is called again.
> 
> I think s/there/hit/ in this comment.  When I read this I initially
> expected the code to be deleting the breakpoint rather than just
> disabling it.
> 
> > +    gdb_test_no_output "disable ${bp_on_non_call}" "disable
> bp_on_non_call"
> > +
> > +
> 
> Can delete one of the empty lines added here.
> 
> > +    # Verify that GDB responds gracefully to the "jump" command without
> > +    # an argument.
> > +    #
> > +    gdb_test "jump" "Argument required .starting address.*" \
> > +	"jump without argument disallowed"
> > +
> > +
> 
> Again, only need a single blank line here.
> 
> > +    # 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"
> > +
> > +
> 
> And here.
> 
> > +    # 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.
> > +    #
> 
> Drop this random trailing '#' line.
> 
> > +
> > +    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] then {
> 
> We don't use the 'then' keyword, this should be written like this:
> 
>   if {![runto_main]} {
> 
> 
> > +	perror "Couldn't run inferior ${inf} to main"
> 
> I don't think this perror call is needed here.  runto_main will already
> emit a FAIL if something goes wrong.
> 
> > +	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
> > +return 0
> 
> This 'return 0' can be dropped.
> 
> With these fixed:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> 
> Thanks,
> Andrew
> 
> > --
> > 2.25.1
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de <http://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

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://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