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

[Petr Tesarik <ptesarik@suse.cz>]

Hi Simon,

On Thu, 24 May 2018 09:33:30 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

>[...]
> 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.

No problem.

> 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...

You bet! In fact, I was kind of expecting this question.

Yes, technically, add-symbol-file can be used for the same purpose, but
it is very inconvenient. Most notably, it requires listing all ELF
sections explicitly, and the Linux kernel has a lot of them (typically
a few dozen).

So, my current solution is:

 1. exec-file vmlinux
 2. info target
 3. # parse the output, adding an offset to each section's start
 4. add-symbol-file vmlinux $textaddr -s ... # a very long list
 5. exec-file  # to make sure that only target memory is used

Although I have already written a Python script to initialise the
session, it's ugly, especially when it comes to parsing the output of
"info target".

Regarding consistency, add-symbol-file does not currently have any
conflicting "-o" option, so I can add one for the same purpose.

Shall I do that?

> I don't have big comments on the patch itself, just nits here and
> there.
>[...]
> >> -			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.

Indeed. I tend to forget that gdb has switched to C++.

> >> @@ -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);
> 	    }

Ah, so that's how it's done. Honestly, I was quite surprised to find no
variant of getopt() here...

>[...]
> >> +# 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"

Will do. I should probably simplify other similar stanzas in the test
suite in a separate patch.

Thank you for your review! I will send an improved patch soon (but
maybe not today, as I'm at a conference).

Petr T