[v3] Make '{add-,}symbol-file' not care about the position of command line arguments
Commit Message
Changes from v2:
- Make .text address always be the first set on 'add-symbol-file'
command.
- Add support for "--" on both commands.
- Extend 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. Both commands now
support the "--" option to stop argument processing.
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-30 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-30 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 | 85 ++++++++++++++++++++++---------------
gdb/testsuite/gdb.base/relocate.exp | 60 ++++++++++++++++++++++++++
2 files changed, 110 insertions(+), 35 deletions(-)
Comments
On 11/30/2017 01:33 PM, Sergio Durigan Junior wrote:
> +# 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"
> +clean_restart
> +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_test "symbol-file -- non-existent-file" \
> + "non-existent-file: No such file or directory\." \
> + "symbol-file with -- disables option processing"
This should be "-non-existent-file", with leading "-" ...
Missing the same test with "add-symbol-file".
Another thing I wondered is why do we have the
expecting_sec_name/expecting_sec_addr variables instead of
simply reading the arguments in the argv ahead inline
like maintenance_print_symbols does?
As is, the patch essentially forbids section names
that start with '-':
(gdb) add-symbol-file ./gdb 0 -s -funnysection 0
add symbol table from file "./gdb" at
.text_addr = 0x0
-funnysection_addr = 0x0
(y or n)
=>
(gdb) add-symbol-file ./gdb 0 -s -funnysection 0
Missing section address after "-s"
and I'm not sure that's really a good idea. I don't think
we should really interpret/require section names to be in any
way/format.
(Another thing that I noticed but I'm kind of ignoring is the fact
that gdb_test treats the question as optional gdb output, so
pedantically gdb could stop outputting the question and answer
"n" automatically and the testcase wouldn't notice.)
Thanks,
Pedro Alves
On Thursday, November 30 2017, Pedro Alves wrote:
> On 11/30/2017 01:33 PM, Sergio Durigan Junior wrote:
>
>> +# 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"
>> +clean_restart
>> +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_test "symbol-file -- non-existent-file" \
>> + "non-existent-file: No such file or directory\." \
>> + "symbol-file with -- disables option processing"
>
> This should be "-non-existent-file", with leading "-" ...
>
> Missing the same test with "add-symbol-file".
Right, fixed.
> Another thing I wondered is why do we have the
> expecting_sec_name/expecting_sec_addr variables instead of
> simply reading the arguments in the argv ahead inline
> like maintenance_print_symbols does?
>
> As is, the patch essentially forbids section names
> that start with '-':
>
> (gdb) add-symbol-file ./gdb 0 -s -funnysection 0
> add symbol table from file "./gdb" at
> .text_addr = 0x0
> -funnysection_addr = 0x0
> (y or n)
>
> =>
>
> (gdb) add-symbol-file ./gdb 0 -s -funnysection 0
> Missing section address after "-s"
>
> and I'm not sure that's really a good idea. I don't think
> we should really interpret/require section names to be in any
> way/format.
I agree; I hadn't thought of this case before. I implemented the
argument reading ahead, as proposed.
> (Another thing that I noticed but I'm kind of ignoring is the fact
> that gdb_test treats the question as optional gdb output, so
> pedantically gdb could stop outputting the question and answer
> "n" automatically and the testcase wouldn't notice.)
Would you prefer if I made the tests answer "y" instead? Or maybe I'm
misunderstanding your concern.
Thanks,
On 11/30/2017 05:26 PM, Sergio Durigan Junior wrote:
>> (Another thing that I noticed but I'm kind of ignoring is the fact
>> that gdb_test treats the question as optional gdb output, so
>> pedantically gdb could stop outputting the question and answer
>> "n" automatically and the testcase wouldn't notice.)
>
> Would you prefer if I made the tests answer "y" instead? Or maybe I'm
> misunderstanding your concern.
>
Take a look at how gdb_test implements the question/response. The
question argument is a question that GDB _may_ ask, not one that
GDB _must_ ask.
So pedantically if add-symbol-file's query ever becomes broken in a way
that makes GDB simply automatically assume "n" without GDB printing
the question in the first place, like:
(gdb) add-symbol-file -s .text 0x200 $binfile 0x100
Not confirmed.
(gdb)
then this:
gdb_test "add-symbol-file -s .text 0x200 $binfile 0x100" \
"Not confirmed\." \
"add-symbol-file different -s .text, before file" \
"add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = x100\r\n\t\.text_addr = 0x200\r\n\\(y or n\\) " \
"n"
won't notice it, it'll still PASS.
The usual way to test must-ask questions is to use
one gdb_test_multiple up to the question, and another gdb_test for
answering the question. Something like (untested):
set test "add-symbol-file different -s .text, before file"
gdb_test_multiple "add-symbol-file -s .text 0x200 $binfile 0x100" $test {
-re "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = x100\r\n\t\.text_addr = 0x200\r\n\\(y or n\\) " {
gdb_test "n" "Not confirmed\." $test
}
}
Thanks,
Pedro Alves
On Thursday, November 30 2017, Pedro Alves wrote:
> On 11/30/2017 05:26 PM, Sergio Durigan Junior wrote:
>
>>> (Another thing that I noticed but I'm kind of ignoring is the fact
>>> that gdb_test treats the question as optional gdb output, so
>>> pedantically gdb could stop outputting the question and answer
>>> "n" automatically and the testcase wouldn't notice.)
>>
>> Would you prefer if I made the tests answer "y" instead? Or maybe I'm
>> misunderstanding your concern.
>>
>
> Take a look at how gdb_test implements the question/response. The
> question argument is a question that GDB _may_ ask, not one that
> GDB _must_ ask.
>
> So pedantically if add-symbol-file's query ever becomes broken in a way
> that makes GDB simply automatically assume "n" without GDB printing
> the question in the first place, like:
>
> (gdb) add-symbol-file -s .text 0x200 $binfile 0x100
> Not confirmed.
> (gdb)
>
> then this:
>
> gdb_test "add-symbol-file -s .text 0x200 $binfile 0x100" \
> "Not confirmed\." \
> "add-symbol-file different -s .text, before file" \
> "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = x100\r\n\t\.text_addr = 0x200\r\n\\(y or n\\) " \
> "n"
>
> won't notice it, it'll still PASS.
>
> The usual way to test must-ask questions is to use
> one gdb_test_multiple up to the question, and another gdb_test for
> answering the question. Something like (untested):
>
> set test "add-symbol-file different -s .text, before file"
> gdb_test_multiple "add-symbol-file -s .text 0x200 $binfile 0x100" $test {
> -re "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = x100\r\n\t\.text_addr = 0x200\r\n\\(y or n\\) " {
> gdb_test "n" "Not confirmed\." $test
> }
> }
OK, I see what you mean. You obviously know that I copied the same
pattern present at gdb.base/relocate.exp (and many other tests, which
makes me frown a bit when I read "The usual way to test must-ask...").
I'll rewrite my tests to use the suggested way, then.
Thanks,
On 11/30/2017 05:43 PM, Sergio Durigan Junior wrote:
> OK, I see what you mean. You obviously know that I copied the same
> pattern present at gdb.base/relocate.exp (and many other tests, which
> makes me frown a bit when I read "The usual way to test must-ask...").
I didn't know that, no, I've only been reading the patch without
looking at the original source. I did say I was ignoring this. ;-)
> I'll rewrite my tests to use the suggested way, then.
Thanks,
Pedro Alves
@@ -1622,26 +1622,35 @@ symbol_file_command (const char *args, int from_tty)
objfile_flags flags = OBJF_USERLOADED;
symfile_add_flags add_flags = 0;
char *name = NULL;
+ bool stop_processing_options = false;
+ int idx;
+ char *arg;
if (from_tty)
add_flags |= SYMFILE_VERBOSE;
gdb_argv built_argv (args);
- for (char *arg : built_argv)
+ for (arg = built_argv[0], idx = 0; arg != NULL; arg = built_argv[++idx])
{
- if (strcmp (arg, "-readnow") == 0)
- flags |= OBJF_READNOW;
- else if (*arg == '-')
- error (_("unknown option `%s'"), arg);
- else
+ if (stop_processing_options || *arg != '-')
{
- symbol_file_add_main_1 (arg, add_flags, flags);
- name = arg;
+ if (name == NULL)
+ name = arg;
+ else
+ error (_("Unrecognized argument \"%s\""), arg);
}
+ else if (strcmp (arg, "-readnow") == 0)
+ flags |= OBJF_READNOW;
+ else if (strcmp (arg, "--") == 0)
+ stop_processing_options = true;
+ else
+ error (_("Unrecognized argument \"%s\""), arg);
}
if (name == NULL)
error (_("no symbol file name was specified"));
+
+ symbol_file_add_main_1 (name, add_flags, flags);
}
}
@@ -2180,8 +2189,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;
@@ -2196,7 +2206,8 @@ add_symbol_file_command (const char *args, int from_tty)
};
struct section_addr_info *section_addrs;
- std::vector<sect_opt> sect_opts;
+ std::vector<sect_opt> sect_opts = { { ".text", NULL } };
+ bool stop_processing_options = false;
struct cleanup *my_cleanups = make_cleanup (null_cleanup, NULL);
dont_repeat ();
@@ -2208,51 +2219,55 @@ 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 (stop_processing_options || *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_opts[0].value = arg;
+ 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 if (strcmp (arg, "--") == 0)
+ stop_processing_options = 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
user. */
- if (sect_opts.empty ())
+ if (!seen_addr)
error (_("The address where %s has been loaded is missing"),
filename.get ());
@@ -37,6 +37,66 @@ 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 0x100 -s .bss 0x3" \
+ "Not confirmed\." \
+ "add-symbol-file positionless -readnow" \
+ "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x100\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 0x100" \
+ "Not confirmed\." \
+ "add-symbol-file positionless -s" \
+ "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x100\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
+ "n"
+gdb_test "add-symbol-file $binfile 0x100 -s .bss 0x3" \
+ "Not confirmed\." \
+ "add-symbol-file positionless -s, no -readnow" \
+ "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x100\r\n\t\.bss_addr = 0x3\r\n\\(y or n\\) " \
+ "n"
+# Check that passing "-s .text", no matter the position, always has
+# the same result.
+gdb_test "add-symbol-file $binfile 0x100 -s .text 0x200" \
+ "Not confirmed\." \
+ "add-symbol-file different -s .text, after file" \
+ "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x100\r\n\t\.text_addr = 0x200\r\n\\(y or n\\) " \
+ "n"
+gdb_test "add-symbol-file -s .text 0x200 $binfile 0x100" \
+ "Not confirmed\." \
+ "add-symbol-file different -s .text, before file" \
+ "add symbol table from file \"${binfile}\" at\r\n\t\.text_addr = 0x100\r\n\t\.text_addr = 0x200\r\n\\(y or n\\) " \
+ "n"
+# Test that passing "--" disables option processing.
+gdb_test "add-symbol-file -- $binfile 0x100 -s .bss 0x3" \
+ "Unrecognized argument \"-s\"" \
+ "add-symbol-file with -- disables option processing"
+# 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"
+clean_restart
+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_test "symbol-file -- non-existent-file" \
+ "non-existent-file: No such file or directory\." \
+ "symbol-file with -- disables option processing"
+clean_restart
+gdb_test "symbol-file -readnow -- $binfile" \
+ "Reading symbols from ${binfile}\.\.\.expanding to full symbols\.\.\.done\." \
+ "symbol-file with -- and -readnow"
+gdb_test "symbol-file -- $binfile -readnow" \
+ "Unrecognized argument \"-readnow\"" \
+ "symbol-file with -- and -readnow, invalid option"
+
+clean_restart
+
gdb_test "add-symbol-file ${binfile} 0 -s" \
"Missing section name after .-s." \
"add-symbol-file bare -s"