Re: [PATCH v2] Make '{add-,}symbol-file' not care about the position of command line arguments

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Thursday, November 30 2017, Pedro Alves wrote:

> On 11/30/2017 04:24 AM, Sergio Durigan Junior wrote:
>> Changes from v1:
>> 
>> - Commit message has been rewritten.
>> 
>> - Implemented position-independent argument parsing for
>>   'add-symbol-file'.
>> 
>> - Added testcases.
>
> Looks like you missed the comment about "--".  Take a look at
> maintenance_print_symbols for an example of a command
> that supports ending options with "--".  Can you add that
> while you're at it, please?  For a test, I'd suggest
> e.g., "symbol-file -- -non-existent-file" and confirming
> gdb errors out.  That's simpler than actually creating a file.

Oh, I didn't understand that you were asking me to implement it.  I
thought it was a comment for "something to be done later".  I'll do it,
then.

>> +      if (*arg != '-')
>>  	{
>> -	  /* It's an option (starting with '-') or it's an argument
>> -	     to an option.  */
>>  	  if (expecting_sec_name)
>>  	    {
>>  	      sect_opt sect = { arg, NULL };
>>  	      sect_opts.push_back (sect);
>> -	      expecting_sec_name = 0;
>> +	      expecting_sec_name = false;
>>  	    }
>>  	  else if (expecting_sec_addr)
>>  	    {
>>  	      sect_opts.back ().value = arg;
>> -	      expecting_sec_addr = 0;
>> +	      expecting_sec_addr = false;
>>  	    }
>> -	  else if (strcmp (arg, "-readnow") == 0)
>> -	    flags |= OBJF_READNOW;
>> -	  else if (strcmp (arg, "-s") == 0)
>> +	  else if (filename == NULL)
>> +	    {
>> +	      /* First non-option argument is always the filename.  */
>> +	      filename.reset (tilde_expand (arg));
>> +	    }
>> +	  else if (!seen_addr)
>>  	    {
>> -	      expecting_sec_name = 1;
>> -	      expecting_sec_addr = 1;
>> +	      /* The second non-option argument is always the text
>> +		 address at which to load the program.  */
>> +	      sect_opt sect = { ".text", arg };
>> +	      sect_opts.push_back (sect);
>> +	      seen_addr = true;
>
> Does this push_back directly here mean that these
> two commands end up with different semantics?
>
>  (gdb) add-symbol-file FILE 0 -s .text 0x1000
>  (gdb) add-symbol-file -s .text 0x1000 FILE 0

The arguments are printed in a different order, yes:

(gdb) add-symbol-file -s .text 0x100 ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0
add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
        .text_addr = 0x100
        .text_addr = 0x0
(y or n) n
Not confirmed.
(gdb) add-symbol-file  ./gdb/testsuite/outputs/gdb.base/relocate/relocate.o 0 -s .text 0x100
add symbol table from file "./gdb/testsuite/outputs/gdb.base/relocate/relocate.o" at
        .text_addr = 0x0
        .text_addr = 0x100
(y or n) n
Not confirmed.

> Not sure that's a good idea.

I'll work on it and make sure they're always printed in the same way.

> Please add a test with "-s .text"...

Will do.

>> +# Check that we can pass parameters using any position in the command
>> +# line.
>> +gdb_test "add-symbol-file -readnow $binfile 0x0 -s .bss 0x3" \
>> +    "Not confirmed\." \
>> +    "add-symbol-file positionless -readnow" \
>> +    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
>> +    "n"
>> +# When we use -s as the first argument, the section will be printed
>> +# first as well.
>> +gdb_test "add-symbol-file -s .bss 0x3 -readnow $binfile 0x0" \
>> +    "Not confirmed\." \
>> +    "add-symbol-file positionless -s" \
>> +    "add symbol table from file \"${binfile}\" at\r\n\t\.bss_addr = 0x3\r\n\t\.text_addr = 0x0\r\n\\(y or n\\) " \
>> +    "n"
>> +gdb_test "add-symbol-file $binfile 0x0 -s .bss 0x3" \
>> +    "Not confirmed\." \
>> +    "add-symbol-file positionless -s, no -readnow" \
>> +    "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x0\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
>> +    "n"
>
> Using a number != 0x0 is a little better, since its easy for
> a variable to end up always zero-initialized / zero-propagated
> by mistake, and the test wouldn't notice.

OK.

>> +# Since we're here, might as well test the 'symbol-file' command and
>> +# if its arguments can also be passed at any position.
>> +gdb_test "symbol-file -readnow $binfile" \
>> +    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
>> +    "symbol-file with -readnow first"
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir $srcdir/$subdir
>
> Just use clean_restart with no argument.

Done.

>> +gdb_test "symbol-file $binfile -readnow" \
>> +    "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
>> +    "symbol-file with -readnow second"
>> +gdb_test "symbol-file -readnow" \
>> +    "no symbol file name was specified" \
>> +    "symbol-file without filename"
>> +
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir $srcdir/$subdir
>
> Ditto.

Done.

>> +
>>  gdb_test "add-symbol-file ${binfile} 0 -s" \
>>      "Missing section name after .-s." \
>>      "add-symbol-file bare -s"
>> 
>
> Thanks,
> Pedro Alves

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/