[v2] Make '{add-,}symbol-file' not care about the position of command line arguments
Commit Message
Changes from v1:
- Commit message has been rewritten.
- Implemented position-independent argument parsing for
'add-symbol-file'.
- Added testcases.
This is a bug that's been detected while doing the readnever work.
If you use 'symbol-file' or 'add-symbol-file', the position of each
argument passed to the command matters. This means that if you do:
(gdb) symbol-file -readnow /foo/bar
The symbol file specified will (correctly) have all of its symbols
read by GDB (because of the -readnow flag). However, if you do:
(gdb) symbol-file /foo/bar -readnow
GDB will silently ignore the -readnow flag, because it was specified
after the filename. This is not a good thing to do and may confuse
the user.
To address that, I've modified the argument parsing mechanisms of
symbol_file_command and add_symbol_file_command to be
"position-independent". I have also added one error call at the end
of add_symbol_file_command's argument parsing logic, which now clearly
complains if no filename has been specified.
This patch provides a testcase for both commands, in order to make
sure that the argument order does not matter. It has been
regression-tested on BuildBot.
gdb/ChangeLog:
2017-11-29 Sergio Durigan Junior <sergiodj@redhat.com>
* symfile.c (symbol_file_command): Call
'symbol_file_add_main_1' only after processing all command
line options.
(add_symbol_file_command): Modify logic to make arguments
position-independent.
gdb/testsuite/ChangeLog:
2017-11-29 Sergio Durigan Junior <sergiodj@redhat.com>
* gdb.base/relocate.exp: Add tests to guarantee that arguments
to 'symbol-file' and 'add-symbol-file' can be
position-independent.
---
gdb/symfile.c | 61 +++++++++++++++++++------------------
gdb/testsuite/gdb.base/relocate.exp | 38 +++++++++++++++++++++++
2 files changed, 70 insertions(+), 29 deletions(-)
Comments
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.
> + 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
Not sure that's a good idea.
Please add a test with "-s .text"...
> +# 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.
> +# 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.
> +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.
> +
> gdb_test "add-symbol-file ${binfile} 0 -s" \
> "Missing section name after .-s." \
> "add-symbol-file bare -s"
>
Thanks,
Pedro Alves
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,
On 11/30/2017 12:38 PM, Sergio Durigan Junior wrote:
> 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.
I'd rather do it at the same time because there's the possibility
that implementing it suggests implementing any-position arguments
differently. Or maybe not. But I'd rather that we didn't have
to redo any-position support later on again.
>>
>> 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.
TBC, the worry was not really about the printing (that's minor),
but what is the actual resulting .text address that the symbol file
it loaded at in the end. But sounds like the printing order can
be used as proxy for checking that. It's not as robust as
actually loading the symbols and then checking what happened
(with e.g., printing a symbol's address), but it's good
enough.
Thanks,
Pedro Alves
On Thursday, November 30 2017, Pedro Alves wrote:
>>> 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.
>
> TBC, the worry was not really about the printing (that's minor),
> but what is the actual resulting .text address that the symbol file
> it loaded at in the end. But sounds like the printing order can
> be used as proxy for checking that. It's not as robust as
> actually loading the symbols and then checking what happened
> (with e.g., printing a symbol's address), but it's good
> enough.
Right, I got it.
The way I made it now, the address provided as .text will always be the
first to be set/printed, even if the user provides a "-s .text" later.
This maintains compatibility with the current way of doing things.
@@ -1634,14 +1634,13 @@ symbol_file_command (const char *args, int from_tty)
else if (*arg == '-')
error (_("unknown option `%s'"), arg);
else
- {
- symbol_file_add_main_1 (arg, add_flags, flags);
- name = arg;
- }
+ name = arg;
}
if (name == NULL)
error (_("no symbol file name was specified"));
+
+ symbol_file_add_main_1 (name, add_flags, flags);
}
}
@@ -2180,8 +2179,9 @@ add_symbol_file_command (const char *args, int from_tty)
char *arg;
int argcnt = 0;
int sec_num = 0;
- int expecting_sec_name = 0;
- int expecting_sec_addr = 0;
+ bool expecting_sec_name = false;
+ bool expecting_sec_addr = false;
+ bool seen_addr = false;
struct objfile *objf;
objfile_flags flags = OBJF_USERLOADED | OBJF_SHARED;
symfile_add_flags add_flags = 0;
@@ -2208,46 +2208,49 @@ add_symbol_file_command (const char *args, int from_tty)
for (arg = argv[0], argcnt = 0; arg != NULL; arg = argv[++argcnt])
{
- /* Process the argument. */
- if (argcnt == 0)
- {
- /* The first argument is the file name. */
- filename.reset (tilde_expand (arg));
- }
- else if (argcnt == 1)
- {
- /* The second argument is always the text address at which
- to load the program. */
- sect_opt sect = { ".text", arg };
- sect_opts.push_back (sect);
- }
- else
+ 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;
}
else
error (_("Unrecognized argument \"%s\""), arg);
}
+ else if (strcmp (arg, "-readnow") == 0)
+ flags |= OBJF_READNOW;
+ else if (strcmp (arg, "-s") == 0)
+ {
+ expecting_sec_name = true;
+ expecting_sec_addr = true;
+ }
+ else
+ error (_("Unrecognized argument \"%s\""), arg);
}
+ if (filename == NULL)
+ error (_("You must provide a filename to be loaded."));
+
/* This command takes at least two arguments. The first one is a
filename, and the second is the address where this file has been
loaded. Abort now if this address hasn't been provided by the
@@ -37,6 +37,44 @@ foreach x {"-raednow" "readnow" "foo" "-readnow s"} {
"add-symbol-file: unknown option $word"
}
+# 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"
+# 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
+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
+
gdb_test "add-symbol-file ${binfile} 0 -s" \
"Missing section name after .-s." \
"add-symbol-file bare -s"