This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.  */
>> +  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.


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


> 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,,,, Using ld: the GNU linker})


>> @@ -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.


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


I'll post the new patch.  Thanks.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]