Re: [PATCH, v17] Add support for setting disassembler-options in GDB for POWER, ARM and S390

[Peter Bergner <bergner@vnet.ibm.com>]

On 2/23/17 6:26 AM, Pedro Alves wrote:
> On 02/22/2017 09:23 PM, Peter Bergner wrote:
>> (gdb) set architecture powerpc:common64 
>> The target architecture is assumed to be powerpc:common64
>> (gdb) show disassembler-options 
>> The current disassembler options are 'power8'
>> [snip]
> 
> Given this was broken but went unnoticed before, something like
> the above should be added as a test.

Done.  I added tests for ARM, PowerPC and S390 to verify their options
are preserved over a set architecture call.



>> +/* A macro for iterating over each comma separated option in OPTIONS.  */
>> +#define FOR_EACH_DISASSEMBLER_OPTION(OPT, OPTIONS) \
>> +  for ((OPT) = (typeof (OPT))(OPTIONS); \
> 
> typeof is a GNU extension.  I don't see any other use of it in the
> tree.  I don't know whether all compilers that binutils intents to
> support accept it.
> 
> But why do you need the cast?

I added it because I got a build error due to a cast, but looking
closer, I added an uneeded const on a variable and removing that
obviates the need for the cast.  It's removed now.  Thanks.



>> +      size_t i, num_options = sizeof (ppc_opts) / sizeof (ppc_opts[0]);
> 
> While at it, all new occurrences of this
> "sizeof (array) / sizeof (array[0])" idiom should really be
> replaced by "ARRAY_SIZE (array)" instead.

Done.



>> +manual and/or the output of @kbd{objdump --help}.  The default value
>> +is empty string.
> 
> Missing "the" in "is the empty string".

Fixed.


> While at it, I think it'd be nice to add a ref to the section in binutils
> manual directly.  We have a few like that already in the manual.  For
> example, we have:
> 
> (@pxref{Overlay Description,,, ld.info, Using ld: the GNU linker})

Done.



>> @@ -780,6 +787,7 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
>>    m_di.endian = gdbarch_byte_order (gdbarch);
>>    m_di.endian_code = gdbarch_byte_order_for_code (gdbarch);
>>    m_di.application_data = this;
>> +  m_di.disassembler_options = *gdbarch_disassembler_options (gdbarch);
> 
> Isn't this going to crash on archs that don't set gdbarch_disassembler_options?

Ahh, I forgot to test this on one of the unsupported architectures after
I made the change of the gdbarch_disassembler_options to char **.
Yes, it dies.  I added a new function get_disassembler_options() that
we can use, which will do the dereference for us, but only if the
arch is supported.  Otherwise, it returns NULL.  That fixes the SEGVs.



> Do the "gdb.base/all-architectures-*.exp" tests pass on an
> --enable-targets=all build of gdb with this patch?

I think I cleaned out my last build, so I can't check.
With the issues above and below fixed, they do all pass.




>> +      for (i = 0, col = 0; valid_options->name[i] != NULL; i++)
>> +	{
>> +	  /* Include the " " and "," we print below.  */
>> +	  size_t len = strlen (valid_options->name[i]) + 2;
>> +	  if (col + len > 80)
> 
> Is this 80 here supposed to be the screen width?  Do we
> really need this manual wrapping?

The only architecture that uses this is the ppc arch and this mimics
what print_ppc_disassembler_options() does.  We have ~60 options that
we're emitting, so I think wrapping does make things more readable.




>> +/* Holds the current set of options to be passed to the disassembler.  */
>> +static char *disassembler_options;
> 
> Even though these are going to be statics, can you please name
> them $cpu_disassembler_options, etc.  It'll make debugging
> gdb easier.  "(gdb) print arm_disassembler_options", etc.

Done.



>> +	  len = strcspn(style, ",");
> 
> missing space before parens.

Fixed.


I'll post the new patch.  Thanks.

Peter