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

[Pedro Alves <palves@redhat.com>]

On 02/15/2017 11:14 PM, Peter Bergner wrote:
> On 2/14/17 2:01 PM, Pedro Alves wrote:
>>  - Maintain the intended git commit log as an integral part
>>    of the patch, and include it in patch re-posts, so that
>>    any revision of the patch can be reviewed as a
>>    self-contained entity.  There's probably some rationale for
>>    some changes to the tests that is written down in some
>>    earlier intro, but was lost meanwhile.
> 
> I'm not a git expert, as I'm paid to work on gcc and we still use
> subversion.  I am willing to learn though, so can you explain what
> you mean by the above?

Play with "git log gdb/" a bit and you'll get a feel.  You'll notice
that we put the rationale for changes in the commit log, unlike in gcc,
where folks just tend to copy the ChangeLog entry to the svn log at
commit time.  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.  The commit log is reviewed as a part of the patch as well.

More info here http://sourceware.org/gdb/wiki/ContributionChecklist.

I'd recommend reading Linux patch submission guidelines, there are
many such documents, and tend to explain these things very
nicely.  For example:
  https://kernelnewbies.org/PatchPhilosophy

>>  - For each new revision of the patch, bump a v2, v3, etc.
>>    revision number in the email subject, so that's easier
>>    to find specific revisions, and to identify whic
>>    email contains the latest version.
> 
> 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.  :-)

> 
>>> +  char *options = remove_whitespace_and_extra_commas
>>> (prospective_options);
>>> +  char *iter, opt[256];
>>
>> Can we get rid of the hardcoded (and not enforced) limit?
>> Maybe just use strtok_r instead of FOR_EACH_DISASSEMBLER_OPTION?
>>
>>> +  /* Verify we have valid disassembler options.  */
>>> +  FOR_EACH_DISASSEMBLER_OPTION (opt, iter, options)
> 
> The problem with strtok{,_r} is that it is destructive to the option
> string, so we'd have to dup the string before scanning it, which
> doesn't seem very elegant either.  We would also need to remember
> to free the dup'd string as well when we were done with it.
> 
> The reason I copied the parsed option into a char array was that I
> needed a null terminated string that I could use with strcmp.
> Unfortunately, the POWER port has several options that have a
> common prefix:
> 
>   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)

and adjust the macro to return the len up to the comma
(minus whitespace) in optlen, and make opt be a pointer that
points directly to the input string.

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

Or something like that.

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

>> I believe 'word' points past the comma already?
> 
> 'word' does point past the last comma, but sometimes, it points well
> past the last comma.  For example, if I type:
> 
>   set disassembler-options force-thumb, reg-name-g<tab>
> 
> ...then 'text' will equal "force-thumb, reg-name-g" and 'word'
> will equal "g".  To get the completer to match "reg-names-gcc",
> I have to modify 'text' to be "reg-names-g".

Ah, that's because "-" is a word break character too.  That'd be
fixable by implementing the set_cmd_completer_handle_brkchars hook,
but let's not bother.  Please just check that

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

does the right thing.

>> I understand that you're largely copying the mechanism
>> from an existing test, but, I should mention that this extracting
>> the function disassembly in one go seems fragile -- at some point
>> this can grow enough to overflow expect's buffer.
> 
> I'm just using the mechanism that the original HUGE test case used.
> The fact that I'm breaking that test case up into many much smaller
> test cases is an improvement and makes overflowing expect's buffer
> less likely with the patch than before the patch.

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

> That's sounds great, but I'm afraid I have no idea how to do the
> above and I don't see any examples like it in the testsuite to copy.
> Unless you can show me a lot more context to your idea, I'm afraid
> I'm going to have to leave the test cases as they are.

OK, let's leave it.

Thanks,
Pedro Alves