Re: [PATCH] Add an optional offset option to the "symbol-file" command

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2018-05-23 04:49, Petr Tesarik wrote:
> Hi all,
>
> any comment on my patch? If it's not good, can you elaborate on what
> needs improvement, please?
>
> Petr T

Hi Petr,

Sorry about that, with the volume on the list patches fall through the 
cracks some times, it is perfectly appropriate to ping them after a 
while as you did.

I am not against adding that new feature to "symbol-file" (it seems 
useful), but I am just wondering first if you can achieve the same thing 
using "add-symbol-file" instead.  add-symbol-file doesn't take an 
offset, but the beginning address of the .text section.  Other sections 
(e.g. .data and .bss) need to be specified separately though.  I'd then 
like to know if it would be possible to make symbol-file similar to 
add-symbol-file, and if that would be practical/easy to use.  On one 
side, it would be weird if symbol-file and add-symbol-file had different 
syntaxes to achieve the same thing, but on the other hand having to 
specify a single offset for all sections of the object file (probably 
enough 99.9% of the time) sounds much easier than having to specify the 
base addresses of multiple sections...

I don't have big comments on the patch itself, just nits here and there.

> > @@ -1222,16 +1222,20 @@ symbol_file_add (const char *name, 
> > symfile_add_flags add_flags,
> >  void
> >  symbol_file_add_main (const char *args, symfile_add_flags add_flags)
> >  {
> > -  symbol_file_add_main_1 (args, add_flags, 0);
> > +  symbol_file_add_main_1 (args, add_flags, 0, 0);
> >  }
> >
> >  static void
> >  symbol_file_add_main_1 (const char *args, symfile_add_flags 
> > add_flags,
> > -			objfile_flags flags)
> > +			objfile_flags flags, CORE_ADDR offset)
> >  {
> > +  struct objfile *objfile;
> > +
> >    add_flags |= current_inferior ()->symfile_flags | SYMFILE_MAINLINE;
> >
> > -  symbol_file_add (args, add_flags, NULL, flags);
> > +  objfile = symbol_file_add (args, add_flags, NULL, flags);

You can declare and assign the variable on the same line.


> > @@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int 
> > from_tty)
> >  	    flags |= OBJF_READNOW;
> >  	  else if (strcmp (arg, "-readnever") == 0)
> >  	    flags |= OBJF_READNEVER;
> > +	  else if (strcmp (arg, "-o") == 0)
> > +	    expecting_offset = true;

This doesn't handle correctly (IMO) "symbol-file foo -o", which should 
be rejected with an error message.  I think it would be simpler to do 
something like this:

	  else if (strcmp (arg, "-o") == 0)
	    {
	      arg = built_argv[++idx];
	      if (arg == NULL)
		error (_("Missing argument to -o"));

	      offset = parse_and_eval_address (arg);
	    }



> > diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> > index 34da102c62..68431cb035 100644
> > --- a/gdb/testsuite/ChangeLog
> > +++ b/gdb/testsuite/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2018-04-27  Petr Tesarik  <ptesarik@suse.com>
> > +
> > +	* gdb.base/relocate.exp: Add test for "symbol-file -o ".
> > +
> >  2018-04-26  Pedro Alves  <palves@redhat.com>
> >
> >  	* gdb.base/gnu-ifunc.exp (set-break): Test that GDB resolves
> > diff --git a/gdb/testsuite/gdb.base/relocate.exp 
> > b/gdb/testsuite/gdb.base/relocate.exp
> > index 89f2fffcd9..4383e79cb2 100644
> > --- a/gdb/testsuite/gdb.base/relocate.exp
> > +++ b/gdb/testsuite/gdb.base/relocate.exp
> > @@ -196,6 +196,39 @@ if { "${function_foo_addr}" == 
> > "${new_function_foo_addr}" } {
> >    pass "function foo has a different address"
> >  }
> >
> > +# Load the object using symbol-file with an offset and check that
> > +# all addresses are moved by that offset.
> > +
> > +set offset 0x10000
> > +clean_restart
> > +gdb_test "symbol-file -o $offset $binfile" \
> > +    "Reading symbols from ${binfile}\.\.\.done\." \
> > +    "symbol-file with offset"
> > +
> > +# Make sure the address of a static variable is moved by offset.
> > +set new_static_foo_addr [get_var_address static_foo]
> > +if { "${new_static_foo_addr}" == "${static_foo_addr}" + $offset } {
> > +  pass "static variable foo is moved by offset"
> > +} else {
> > +  fail "static variable foo is moved by offset"
> > +}

You can simplify these using gdb_assert:

gdb_assert "${new_static_foo_addr} == ${static_foo_addr} + $offset" \
    "static variable foo is moved by offset"

Thanks,

Simon