This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Improve readability of x86's SystemTap SDT probe support & fix bug & add testcase
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>
- Date: Sat, 28 Dec 2013 07:48:34 +0400
- Subject: Re: [PATCH] Improve readability of x86's SystemTap SDT probe support & fix bug & add testcase
- Authentication-results: sourceware.org; auth=none
- References: <m3a9fmhqbe dot fsf at redhat dot com>
> This patch reorganizes code on gdb/i386-tdep.c's SystemTap SDT probe
> support functions. Before it, the code to parse special operands on x86
> lived in a single, big function. This patch creates 2 new functions
> that makes the code more organized and removes a few indentation levels
> (which is always good IMO).
>
> While I was making the patch, I decided to create some testcases for it
> (by tweaking the probe's asm generated by GCC), and found a bug on the
> parsing code for the triplet displacement operand (i.e., things like
> "-4+8-20(%rbp)"). Well, since I was already at it, I created some
> testcases for the two kinds of special operands handled by the code (the
> triplet displacement mentioned above, and the three-argument
> displacement like "(%rbx,%ebx,-8)"). This code is target-dependent so I
> created it under gdb.arch/.
>
> Comments? OK to apply?
First of all, thanks for creating a testcase!
I've only skimmed through the patch so far. Generally speaking,
I definitely agree that simplifying large functions is a useful
thing to do. Fixing bugs too :). It would be better, though, if
we could separate the two into different patches, one that does
the re-org, and nothing else. This one can be large, but we know
the code hasn't really be changed. Perhaps "git diff/show -b"
will even confirm that. And the second can include the fix and
the testcase, and it'll be obvious from the diff what the fix is.
Would you mind doing that?
> gdb/ChangeLog
> 2013-12-27 Sergio Durigan Junior <sergiodj@redhat.com>
>
> * i386-tdep.c (i386_stap_parse_special_token_triplet): New
> function, with code from i386_stap_parse_special_token. Fix bug
> with the parsing of triplet displacement.
> (i386_stap_parse_special_token_three_arg_disp): New function, with
> code from i386_stap_parse_special_token.
> (i386_stap_parse_special_token): Move code to the two functions
> created above.
>
> gdb/testsuite/ChangeLog
> 2013-12-27 Sergio Durigan Junior <sergiodj@redhat.com>
>
> * gdb.arch/amd64-stap-special-operands.exp: New file.
> * gdb.arch/amd64-stap-three-arg-disp.S: Likewise.
> * gdb.arch/amd64-stap-three-arg-disp.c: Likewise.
> * gdb.arch/amd64-stap-triplet.S: Likewise.
> * gdb.arch/amd64-stap-triplet.c: Likewise.
--
Joel