Re: [PATCH 0/7] Refactor shared code in {i386,amd64}-linux-nat.c

[Pedro Alves <palves@redhat.com>]

On 06/27/2014 10:28 AM, Mark Kettenis wrote:

> Sorry, no.  Perhaps more code can be shared between i386-linux-nat.c
> and amd64-linux-nat.c, but this goes too far and turns things into
> #ifdef spagetthi.  It also breaks established naming conventions.

I think you may have stopped looking before patch #6.  :-)

Please also keep in mind that gdbserver also has its own copy of
this code, and on the gdbserver side, we also started out with i386 and
amd64 split in different files, and then they got merged:

 commit d0722149ad594a7d3892bb2fd53a72c6d4933793
 Author: Doug Evans <dje@google.com>
 Date:   Tue May 12 22:25:00 2009 +0000

        Biarch support for i386/amd64 gdbserver.
        * Makefile.in (SFILES): Remove linux-i386-low.c, linux-x86-64-low.c.
        Add linux-x86-low.c.
        (linux-i386-low.o, linux-x86-64-low.o): Delete.
        (linux-x86-low.o): Add.
        ...

Please look at the resulting x86-linux-nat.c, and gdbserver's
existing linux-x86-low.c, the interim steps.
(The latter we'll eventually want to merge gdb's code with.)

This whole series can be found in

  git@github.com:gbenson/binutils-gdb.git gbenson/i386-dregs

for convenience.

I can't agree that this series went too far, on the contrary - gdbserver
has everything in the linux-x86-low.c file, while Gary only moved the truly
shared code.  That's a fine incremental step IMNSHO.

The resulting #ifdefery quite reasonable, IMNSHO -- it'd be bad if there
were lots of different combinations of symbols we'd have to #ifdef on,
but there's really only a few #ifdef __x86_64__.  So if it's
spaghetti, it's still dry, raw, straight and neatly lined up
in the package.  :-)

As for naming, I suggested x86 myself.  I think x86 for shared 32-bit/64-bit
code, is quite an established convention itself.  It's what we already use
in gdbserver, and at least glibc and the Linux kernel use x86 for shared
code too.  E.g.:

 $ ls glibc/src/sysdeps/ | grep 86
 i386
 x86
 x86_64

I'd _really_, _really_ love to see this go forward (really), and reduce
the amount of places one has to touch when hacking on watchpoint stuff.
and when adding new target descriptions, etc., from 3 places, to 2 places.
See https://sourceware.org/ml/gdb-patches/2014-06/msg00773.html for a patch
that just recently still had to touch 3 places with the same code.
And going forward, I'd like to reduce that to 1 place only, even.

Please take a look at the resulting code past patch #5, I believe you'll
be pleasantly surprised! :-)

-- 
Pedro Alves