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

Sergio Durigan Junior
2017-11-30T12:38:12Z

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