RE: [PATCH v3 1/1] gdb, infcmd: Support jump command with same line in multiple symtabs

[Andrew Burgess <aburgess@redhat.com>]

"Puputti, Matti" <matti.puputti@intel.com> writes:

> Hi Andrew,
>
> Thank you for your comments.
> I plan to follow all your suggestions in the next patch.
>
> On the document update, what you think about this proposal:
> "If @var{locspec} resolves to more than one address,
> those outside the current compilation unit are ignored.  If still not a
> unique address found, the command aborts before jumping. "

Sounds reasonable to me, I'm sure Eli will provide a proper docs review
when the patch is posted.

Thanks,
Andrew


>
> Br,
> Matti Puputti
>
>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Friday, April 28, 2023 4:40 PM
>> To: Puputti, Matti <matti.puputti@intel.com>; gdb-patches@sourceware.org
>> Cc: blarsen@redhat.com
>> Subject: Re: [PATCH v3 1/1] gdb, infcmd: Support jump command with same
>> line in multiple symtabs
>> 
>
> < . . . >
>
>> > +      sals.erase (std::remove_if (sals.begin (), sals.end (),
>> > +		  [] (symtab_and_line &sal)
>> > +		    {
>> > +		      struct symtab_and_line cursal
>> > +			  = get_current_source_symtab_and_line ();
>> > +		      return sal.symtab != cursal.symtab;
>> 
>> Maybe worrying about nothing, but I wonder if we should move the call to
>> get_current_source_symtab_and_line outside of the lambda, and then
>> capture cursal by reference.
>> 
>> Also, the 'symtab_and_line &sal' can be made const.
>> 
>> > +		    }), sals.end ());
>> > +      if (sals.size () != 1)
>> > +	error (_("Unreasonable jump request"));
>> > +    }
>> 
>> This all makes sense, but I think you need to update the docs.  The docs
>> currently say:
>> 
>>   ... If @var{locspec} resolves to more than one address, the command
>>   aborts before jumping.
>> 
>> Which isn't true any more right?
>> 
>
> < . . . >
>
>> > +
>> > +standard_testfile .c
>> > +set srcfile2 jump_multiple_objfiles_foo.c
>> > +set srcfile3 jump_multiple_objfiles.h
>> 
>> If you rename 'jump_multiple_objfiles_foo.c' to
>> 'jump_multiple_objfiles-foo.c', then you can replace these three lines
>> with:
>> 
>>   standard_testfile .c -foo.c .h
>> 
>> which is the preferred approach these days.
>> 
>
> < . . . >
>
>> > +
>> > +if { ![runto_main] } {
>> > +    perror "couldn't run to breakpoint"
>> 
>> Drop this perror call.  runto_main will FAIL if appropriate.
>> 
>> Thanks,
>> Andrew
>> 
>
> < . . . >
> 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