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, updated] Add support for setting disassembler-options in GDB for POWER, ARM and S390

On 2/15/17 6:21 PM, Pedro Alves wrote:
In practice, what this means is just that you use
"git commit --ammend", and edit the commit message to include the
rationale for the patch, and update it / maintain it whenever the patch
changes in a way that might change the description/rationale for
the patch.

Ah, the --ammend is the part I was missing.  Thanks.

Easily done, as I've been doing just that internally.
I'm frightened to say that I'm at v25 and counting. :-(

Internal revisions don't count, only public submissions.  :-)

Well the number of public submissions isn't too far off my
internal patches. :-)

  Eg: "e500" & "e500mc", "ppc" & "ppc32" and "ppc64", etc.

...which strncmp cannot disambiguate, because it doesn't enforce the
two strings have the same length.

You could handle that with:

if (optlen == strlen (valid_options->name[i])
    && strncmp (opt, optlen, valid_options->name[i]) == 0)

Yes, but that involves two scans over the string, so...

I have two ideas, one is to write our own strcmp that treats option
delimiters like ',' just like '\0'.

Or something like that.

... I will attempt to try this first, since it seems like the easiest
solution.  Is there a preferred location for a function like this to
go into?  Looking around, it seems maybe common/common-utils.c?

The other idea would be to
modify the disassembler_options string so that we use '\0' as the
delimiter between the different options.  We'd need an extra '\0'
at the end to know when we've run out of options though.  If we
did this, then we could just use standard strcmp on the options.

Can't see how this would work without an interning step,
given the delimiters come from user input.

To be honest, I'd prefer an option string like this, since it
would allow for an easy FOR_EACH_DISASSEMBLER_OPTION macro and
we could use standard strcmp, but I too was wondering how I could
easily add 1 extra char to the string to hold the extra null byte
which is needed to identify the end of the option strings.
That's why I was going to try my first suggestion first.

Please just check that

 (gdb) complete set disassembler-options force-thumb, reg-name-g

does the right thing.

It does:

(gdb) complete set disassembler-options force-thumb, reg-names-g
set disassembler-options force-thumb, reg-names-gcc

Ditto for the ARM specific set arm disassembler option (which uses
short option names):

(gdb) complete set arm disassembler g
set arm disassembler gcc

This mention of "breaking the test cases up in many smaller
test cases" is the sort of rationale that would be nice to put in the
commit log.  :-)  I wasn't actually sure that that's what you're
doing, and why.  Seemed like you've changed the tests to avoid
hardcoding offsets too?  (it would probabably be clearer to do that
with a separate preparatory patch, with the added advantage that
that part could probably be merged to master quickly, but fine
to keep it together if you prefer.)

Well that was the whole rational for this patch.  I had made some
changes to GAS to modify the support for our POWER9 cpu and thought
I'd be a good citizen and update the GDB testsuite while I was at it.
Unfortunately, the file was incredibly large and hard to update due to
the hard coded file offsets.  Also, the test case tested for power7,
power8 and power9 all in the same test case.  I thought it best if
we test each cpu separately and that we should be able to enforce
the disassembler options when doing it (in case we deprecate some
instruction(s) on newer cpus).  And this is what led to the patch
we have now. :-)


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