Re: [RFC-v2] Mingw Windows 64-bit gdbserver

[Pedro Alves <pedro at codesourcery dot com>]

On Sunday 18 April 2010 00:17:59, Pierre Muller wrote:
> 
> > -----Message d'origine-----
> > De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> > owner@sourceware.org] De la part de Pedro Alves
> > Envoyé : Saturday, April 17, 2010 12:58 PM
> > À : Pierre Muller
> > Cc : gdb-patches@sourceware.org
> > Objet : Re: [RFC] Mingw Windows 64-bit gdbserver
> > 
> > On Saturday 17 April 2010 06:40:02, Pierre Muller wrote:
> > 
> > > > How about instead merging the files, like
> > > > linux-x86-low.c handles both 64-bit and 32-bit?  There's
> > > > a lot of common stuff between both archs support, it
> > > > seems.
> > >
> > >   Of course, I agree with you that the two files
> > > share a very large common portion that is identical.
> > >   There are only two places where they really differ:
> > >   For the call to the init_registers_XXX
> > > and for the register mappings array.
> > >
> > >   The main question is how should we split these parts
> > > off if we want to keep a common part:
> > >
> > >   I would propose this:
> > >   rename win32-i386-low.c to win-x86-low.c
> > 
> > Lets avoid someone reading this and getting religious
> > against "win", and go with windows-*-low.c, just
> > like gdb/windows-nat.c was renamed from win32-nat.c, and
> > gdb has i386-windows-nat.c and amd64-windows-nat.c.
> > 
> > >   Create win32-i386-low.h and win64-amd64-low.h
> > > that would have the register mappings and
> > > a macro to define their local init_registers.
> > 
> > Yes, much better, if nothing else because that's how
> > gdb handles this as well.  It's always good to have the
> > code bases solve the same problem in the same way, so
> > that we can more easily keep them in sync or merge them.
> > 
> > Take a look at gdb/amd64-windows-nat.c, it also does something
> > similar to handle the common stuff, though since we have
> > a win32_target_ops in gdbserver, we can put the register mappings
> > array pointer directly in win32_target_ops instead of making it
> > a global.
> > 
> > Let's avoid macros.  Use for example the `arch_setup' callback
> > in the win32_target_ops vector for this, keeping the arrays
> > defined in the corresponding arch specific .c files.
> 
>   Here is a new version of the patch:
> I tried to minimize code duplication, but I
> still needed one macro to decide which file
> from win32-i386-low.c or win64-amd64-low.c 
> should be read.

Sorry, this is a no-no.  Please reread what I said.  Do
not include ".c" files.  If you want to take a route like
that (have a define to select between 64-bit and 32-bit),
might as well merge the i386 and amd64 bits all within the
same file, just like I suggested from the beggining, and
use `ifdef __x86_64__'.

>   The ChangeLog is a bit messy because I renamed a file
> and reused the old name to create a new file...
> but I didn't find another name that would fit better...
> Unless I use windows- as a prefix everywhere...

I think is would be simpler for both of us if you split out this
patch into smaller pieces.  Fix the type casts in win32-low.c,
one piece.  Renaming a file, and adjusting
configure/makefile, another piece.  Splitting out i?86 bits
and amd64 windows bits to their own files, another piece
(though I still wonder why not merge all of that into
one file, but I admit that I no longer know what
is best).

This change:

> -srv_amd64_regobj="amd64.o x86-64-avx.o"
> +srv_amd64_regobj="amd64.o amd64-avx.o"

looks like another piece.  Is it just a cosmetic change because
we use amd64 instead of x86-64 throughout?  Please explain
it and post it separately, if possible.

Etc., etc.

-- 
Pedro Alves