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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-05-25 01:23, Petr Tesarik wrote:
> 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".

Thanks for confirming.

> 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 think it would be a good idea.  It would improve the usability of 
add-symbol-file and keep both commands more or less in sync, so it 
sounds like a win-win to me.

I assume the second positional argument of add-symbol-file (which gives 
the start address of .text) would become optional?  You just need to 
think carefully about what should happen when mixing the different 
arguments.  The .text positional argument and -o would probably be 
mutually exclusive?  -o applies to all sections, but you could use -s to 
override the address of a particular section?

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

Yep, you can get wild!

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

I think GDB would benefit of having such a variant of getopt to 
standardize how arguments are parsed throughout the CLI.  And to avoid 
having to do manual parsing like this, which often gives a fragile 
result.

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

If you are up for it, it would be appreciated!

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

But but... it's the ideal time to work on side-quest patches! ;)

Thanks,

Simon