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

Pedro Alves
2017-11-30T10:57:56Z

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