[PATCH,HURD] Fix reading core

[PATCH,HURD] Fix reading core

[Samuel Thibault]

The i386 GNU/Hurd ELF core format actually follows the uaccess gregset_t
array format, not the Mach thread state format.  This fixes gdb reading
it.

* gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
(creg_offset): New array.
(CREG_ADDR): Use creg_offset instead of reg_offset.

--- a/gdb/i386gnu-nat.c.original	2013-02-11 00:46:02.000000000 +0000
+++ b/gdb/i386gnu-nat.c	2013-02-11 00:48:09.000000000 +0000
@@ -56,8 +56,21 @@
   REG_OFFSET (ds), REG_OFFSET (es), REG_OFFSET (fs), REG_OFFSET (gs)
 };
 
+/* Offset to the greg_t location where REG is stored.  */
+#define CREG_OFFSET(reg) (REG_##reg * 4)
+
+/* At CREG_OFFSET[N] is the offset to the greg_t location where
+   the GDB register N is stored.  */
+static int creg_offset[] =
+{
+  CREG_OFFSET (EAX), CREG_OFFSET (ECX), CREG_OFFSET (EDX), CREG_OFFSET (EBX),
+  CREG_OFFSET (UESP), CREG_OFFSET (EBP), CREG_OFFSET (ESI), CREG_OFFSET (EDI),
+  CREG_OFFSET (EIP), CREG_OFFSET (EFL), CREG_OFFSET (CS), CREG_OFFSET (SS),
+  CREG_OFFSET (DS), CREG_OFFSET (ES), CREG_OFFSET (FS), CREG_OFFSET (GS)
+};
+
 #define REG_ADDR(state, regnum) ((char *)(state) + reg_offset[regnum])
-#define CREG_ADDR(state, regnum) ((const char *)(state) + reg_offset[regnum])
+#define CREG_ADDR(state, regnum) ((const char *)(state) + creg_offset[regnum])
 
 \f
 /* Get the whole floating-point state of THREAD and record the values

Re: [PATCH,HURD] Fix reading core

[Joel Brobecker]

> The i386 GNU/Hurd ELF core format actually follows the uaccess gregset_t
> array format, not the Mach thread state format.  This fixes gdb reading
> it.
> 
> * gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
> (creg_offset): New array.
> (CREG_ADDR): Use creg_offset instead of reg_offset.

Did anyone review this? I don't know GNU/Hurd, but the patch looks
very plausible.

Do you have write access to the GDB repository? If not, do you have
copyright assignment papers on file? This patch is small enough that
we can take it as a tiny patch, but if you think you're going to
send more patches, we should probably get you started on those.

The ChangeLog entry above needs to be indented properly (just to be
sure). Another tiny style nitpick below...

> --- a/gdb/i386gnu-nat.c.original	2013-02-11 00:46:02.000000000 +0000
> +++ b/gdb/i386gnu-nat.c	2013-02-11 00:48:09.000000000 +0000
> @@ -56,8 +56,21 @@
>    REG_OFFSET (ds), REG_OFFSET (es), REG_OFFSET (fs), REG_OFFSET (gs)
>  };
>  
> +/* Offset to the greg_t location where REG is stored.  */
> +#define CREG_OFFSET(reg) (REG_##reg * 4)
> +
> +/* At CREG_OFFSET[N] is the offset to the greg_t location where
> +   the GDB register N is stored.  */
> +static int creg_offset[] =
> +{
> +  CREG_OFFSET (EAX), CREG_OFFSET (ECX), CREG_OFFSET (EDX), CREG_OFFSET (EBX),
> +  CREG_OFFSET (UESP), CREG_OFFSET (EBP), CREG_OFFSET (ESI), CREG_OFFSET (EDI),
> +  CREG_OFFSET (EIP), CREG_OFFSET (EFL), CREG_OFFSET (CS), CREG_OFFSET (SS),
> +  CREG_OFFSET (DS), CREG_OFFSET (ES), CREG_OFFSET (FS), CREG_OFFSET (GS)
> +};

Unless it was done on purpose, we try to limit the size of lines
to 70 characters, only extending it to up to 80 when it makes
a difference....


> +
>  #define REG_ADDR(state, regnum) ((char *)(state) + reg_offset[regnum])
> -#define CREG_ADDR(state, regnum) ((const char *)(state) + reg_offset[regnum])
> +#define CREG_ADDR(state, regnum) ((const char *)(state) + creg_offset[regnum])
>  
>  \f
>  /* Get the whole floating-point state of THREAD and record the values

-- 
Joel

Re: [PATCH,HURD] Fix reading core

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

Hi!

On Tue, 9 Apr 2013 16:41:17 -0700, Joel Brobecker <brobecker@adacore.com> wrote:
> > The i386 GNU/Hurd ELF core format actually follows the uaccess gregset_t
> > array format, not the Mach thread state format.  This fixes gdb reading
> > it.
> > 
> > * gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
> > (creg_offset): New array.
> > (CREG_ADDR): Use creg_offset instead of reg_offset.
> 
> Did anyone review this? I don't know GNU/Hurd, but the patch looks
> very plausible.

Not yet, but it's on my list... (together with other Hurd GDB work).
Anyway, Joel, thanks for following up on Samuel's patch, and not letting
it fall through the cracks!


> Do you have write access to the GDB repository? If not, do you have
> copyright assignment papers on file? This patch is small enough that
> we can take it as a tiny patch, but if you think you're going to
> send more patches, we should probably get you started on those.

I'd support all that; Samuel's not on file for GDB (but is for other
packages, such as Hurd proper).


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [PATCH,HURD] Fix reading core

[Samuel Thibault]

Thomas Schwinge, le Thu 11 Apr 2013 00:35:15 +0200, a écrit :
> > Do you have write access to the GDB repository? If not, do you have
> > copyright assignment papers on file? This patch is small enough that
> > we can take it as a tiny patch, but if you think you're going to
> > send more patches, we should probably get you started on those.
> 
> I'd support all that; Samuel's not on file for GDB (but is for other
> packages, such as Hurd proper).

My other patch about x86 hardware watchpoint support might need it
indeed. Which template should I use?

Samuel

Re: [PATCH,HURD] Fix reading core

[Thomas Schwinge]

[-- Attachment #1.1: Type: text/plain, Size: 836 bytes --]

Hi!

On Thu, 11 Apr 2013 00:54:32 +0200, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Thomas Schwinge, le Thu 11 Apr 2013 00:35:15 +0200, a écrit :
> > > Do you have write access to the GDB repository? If not, do you have
> > > copyright assignment papers on file? This patch is small enough that
> > > we can take it as a tiny patch, but if you think you're going to
> > > send more patches, we should probably get you started on those.
> > 
> > I'd support all that; Samuel's not on file for GDB (but is for other
> > packages, such as Hurd proper).
> 
> My other patch about x86 hardware watchpoint support might need it
> indeed. Which template should I use?

Going by your other assignments (and by knowing you) ;-), you'll want the
generic »past and future changes«; attached.


Grüße,
 Thomas



[-- Attachment #1.2: request-assign.future --]
[-- Type: text/plain, Size: 965 bytes --]

Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]


[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]


[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]


[For the copyright registration, what country are you a citizen of?]


[What year were you born?]


[Please write your email address here.]


[Please write your postal address here.]





[Which files have you changed so far, and which new files have you written
so far?]

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [PATCH,HURD] Fix reading core

[Samuel Thibault]

Joel Brobecker, le Tue 09 Apr 2013 16:41:17 -0700, a écrit :
> > The i386 GNU/Hurd ELF core format actually follows the uaccess gregset_t
> > array format, not the Mach thread state format.  This fixes gdb reading
> > it.
> > 
> > * gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
> > (creg_offset): New array.
> > (CREG_ADDR): Use creg_offset instead of reg_offset.
> 
> Did anyone review this? I don't know GNU/Hurd, but the patch looks
> very plausible.
> 
> Do you have write access to the GDB repository?

I don't.

> If not, do you have copyright assignment papers on file?

It's on file now.

Samuel

Re: [PATCH,HURD] Fix reading core

[Joel Brobecker]

> > > * gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
> > > (creg_offset): New array.
> > > (CREG_ADDR): Use creg_offset instead of reg_offset.
> > 
> > Did anyone review this? I don't know GNU/Hurd, but the patch looks
> > very plausible.
> > 
> > Do you have write access to the GDB repository?
> 
> I don't.
> 
> > If not, do you have copyright assignment papers on file?
> 
> It's on file now.

OK - if you have other patches in the pipeline, let's give you write
access to the repository as soon as this patch is approved. I am not
clear whether I should re-review, or let Thomas do it.

Thank you,
-- 
Joel

Re: [PATCH,HURD] Fix reading core

[Alfred M. Szmidt]

   > > > * gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
   > > > (creg_offset): New array.
   > > > (CREG_ADDR): Use creg_offset instead of reg_offset.
   > > 
   > > Did anyone review this? I don't know GNU/Hurd, but the patch looks
   > > very plausible.
   > > 
   > > Do you have write access to the GDB repository?
   > 
   > I don't.
   > 
   > > If not, do you have copyright assignment papers on file?
   > 
   > It's on file now.

   OK - if you have other patches in the pipeline, let's give you write
   access to the repository as soon as this patch is approved. 

Consider it approved.

Re: [PATCH,HURD] Fix reading core

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

Hi!

On Tue, 9 Apr 2013 16:41:17 -0700, Joel Brobecker <brobecker@adacore.com> wrote:
> > The i386 GNU/Hurd ELF core format actually follows the uaccess gregset_t
> > array format, not the Mach thread state format.  This fixes gdb reading
> > it.
> > 
> > * gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
> > (creg_offset): New array.
> > (CREG_ADDR): Use creg_offset instead of reg_offset.

In context with the related Hurd patch,
<http://git.savannah.gnu.org/cgit/hurd/hurd.git/commit/?id=c48e6ec36da97d514488f88fa4b9c268fd0d6284>,
I have now reviewed, tested, and committed this patch.

> Do you have write access to the GDB repository?

Samuel asked me to commit it on his behalf (and likewise his other patch,
once reviewed).

> do you have
> copyright assignment papers on file?

This has been completed.

> > --- a/gdb/i386gnu-nat.c.original	2013-02-11 00:46:02.000000000 +0000
> > +++ b/gdb/i386gnu-nat.c	2013-02-11 00:48:09.000000000 +0000
> > @@ -56,8 +56,21 @@
> >    REG_OFFSET (ds), REG_OFFSET (es), REG_OFFSET (fs), REG_OFFSET (gs)
> >  };
> >  
> > +/* Offset to the greg_t location where REG is stored.  */
> > +#define CREG_OFFSET(reg) (REG_##reg * 4)
> > +
> > +/* At CREG_OFFSET[N] is the offset to the greg_t location where
> > +   the GDB register N is stored.  */
> > +static int creg_offset[] =
> > +{
> > +  CREG_OFFSET (EAX), CREG_OFFSET (ECX), CREG_OFFSET (EDX), CREG_OFFSET (EBX),
> > +  CREG_OFFSET (UESP), CREG_OFFSET (EBP), CREG_OFFSET (ESI), CREG_OFFSET (EDI),
> > +  CREG_OFFSET (EIP), CREG_OFFSET (EFL), CREG_OFFSET (CS), CREG_OFFSET (SS),
> > +  CREG_OFFSET (DS), CREG_OFFSET (ES), CREG_OFFSET (FS), CREG_OFFSET (GS)
> > +};
> 
> Unless it was done on purpose, we try to limit the size of lines
> to 70 characters, only extending it to up to 80 when it makes
> a difference....

I left this as is; the idea is to keep it visually comparable to the
reg_offset definition just above (same sequence of registers; see the
first line of the patch's context).


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]