* AVX and unavailable registers, fix system-gcore.exp
@ 2012-01-26 19:16 Pedro Alves
2012-02-27 18:49 ` H.J. Lu
2012-03-05 14:32 ` i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL Pedro Alves
0 siblings, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2012-01-26 19:16 UTC (permalink / raw)
To: GDB Patches, Mark Kettenis
I now have access to a machine with AVX, and I'm seeing a few core related
failures on native x86_64 Fedora 16. GDBserver doesn't show the failures because
it treats this issue already like the patch below proposes.
This was discussed a while ago, in the "RFC: partially available registers"
thread:
http://old.nabble.com/RFC%3A-partially-available-registers-td32055573i20.html
The failures I'm seeing look like, e.g.:
FAIL: gdb.arch/system-gcore.exp: corefile restored all registers
because before the core, the program showed:
...
ymm0 *value not available*
...
and when we load back the core we see:
...
ymm0 {v8_float = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_double = {0x0, 0x0, 0x0, 0x0}, v32_int8 = {0x0 <repeats 32 time
s>}, v16_int16 = {0x0 <repeats 16 times>}, v8_int32 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_int64 = {0x0, 0x0, 0x0, 0x0}, v2_int
128 = {0x00000000000000000000000000000000, 0x00000000000000000000000000000000}}
...
I think we should make live debugging present the still-not-touched x87
states as zero, as described in the comment in the patch:
+ /* With the delayed xsave mechanism, in between the program
+ starting, and the program accessing the vector registers for the
+ first time, the register's values are invalid. The kernel
+ initializes register states to zero when they are set the first
+ time in a program. This means that from the user-space programs'
+ perspective, it's the same as if the registers have always been
+ zero from the start of the program. Therefore, the debugger
+ should provide the same illusion to the user.
This is already handled this way with gdbserver, hence we don't see
these failures there (the gcore tests now work against gdbserver).
The kernel is also putting the registers touched as zero
in the core, otherwise we'd see *unavailable* when we loaded the
core file back.
I have a doubt in the xsave-in-corefile support bits. There's code in place to
handle a NULL regs (as in no xsave contents to work with), so I'm handling it
as presently:
+
+ Note however, the case when REGS is NULL is a different case.
+ That case means we do not have access to the x87 states, so we
+ should mark the registers as unavailable (by supplying NULL). */
+
but I can't figure out how would we ever get a NULL REGS there. Is there a
convoluted path I missed? amd64-linux-tdep.c unconditionally installs
amd64_linux_regset_sections as gdbarch_core_regset_sections
callback, and this includes the .reg-xstate section.
However, corelow.c:get_core_register_section bails early if
a section is not found in the core, never reaching regset->supply_regset
with a NULL `contents'.
Mark, maybe you have a clue?
--
Pedro Alves
2012-01-26 Pedro Alves <palves@redhat.com>
* i387-tdep.c (i387_supply_xsave): If we have an xsave buffer, and
the register state is clear, supply explicit zero, instead of
marking the register unavailable.
---
gdb/i387-tdep.c | 87 ++++++++++++++++++++++++++++++++++---------------------
1 files changed, 54 insertions(+), 33 deletions(-)
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index 450b8f2..b3adb74 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -725,6 +725,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
const gdb_byte *regs = xsave;
int i;
unsigned int clear_bv;
+ const gdb_byte zero[MAX_REGISTER_SIZE] = { 0 };
const gdb_byte *p;
enum
{
@@ -764,6 +765,19 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
else
clear_bv = I386_XSTATE_AVX_MASK;
+ /* With the delayed xsave mechanism, in between the program
+ starting, and the program accessing the vector registers for the
+ first time, the register's values are invalid. The kernel
+ initializes register states to zero when they are set the first
+ time in a program. This means that from the user-space programs'
+ perspective, it's the same as if the registers have always been
+ zero from the start of the program. Therefore, the debugger
+ should provide the same illusion to the user.
+
+ Note however, the case when REGS is NULL is a different case.
+ That case means we do not have access to the x87 states, so we
+ should mark the registers as unavailable (by supplying NULL). */
+
switch (regclass)
{
case none:
@@ -771,26 +785,26 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
case avxh:
if ((clear_bv & I386_XSTATE_AVX))
- p = NULL;
+ regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
else
- p = XSAVE_AVXH_ADDR (tdep, regs, regnum);
- regcache_raw_supply (regcache, regnum, p);
+ regcache_raw_supply (regcache, regnum,
+ XSAVE_AVXH_ADDR (tdep, regs, regnum));
return;
case sse:
if ((clear_bv & I386_XSTATE_SSE))
- p = NULL;
+ regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
else
- p = FXSAVE_ADDR (tdep, regs, regnum);
- regcache_raw_supply (regcache, regnum, p);
+ regcache_raw_supply (regcache, regnum,
+ FXSAVE_ADDR (tdep, regs, regnum));
return;
case x87:
if ((clear_bv & I386_XSTATE_X87))
- p = NULL;
+ regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
else
- p = FXSAVE_ADDR (tdep, regs, regnum);
- regcache_raw_supply (regcache, regnum, p);
+ regcache_raw_supply (regcache, regnum,
+ FXSAVE_ADDR (tdep, regs, regnum));
return;
case all:
@@ -798,16 +812,19 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
if ((tdep->xcr0 & I386_XSTATE_AVX))
{
if ((clear_bv & I386_XSTATE_AVX))
- p = NULL;
+ {
+ for (i = I387_YMM0H_REGNUM (tdep);
+ i < I387_YMMENDH_REGNUM (tdep);
+ i++)
+ regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+ }
else
- p = regs;
-
- for (i = I387_YMM0H_REGNUM (tdep);
- i < I387_YMMENDH_REGNUM (tdep); i++)
{
- if (p != NULL)
- p = XSAVE_AVXH_ADDR (tdep, regs, i);
- regcache_raw_supply (regcache, i, p);
+ for (i = I387_YMM0H_REGNUM (tdep);
+ i < I387_YMMENDH_REGNUM (tdep);
+ i++)
+ regcache_raw_supply (regcache, i,
+ XSAVE_AVXH_ADDR (tdep, regs, i));
}
}
@@ -815,16 +832,18 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
if ((tdep->xcr0 & I386_XSTATE_SSE))
{
if ((clear_bv & I386_XSTATE_SSE))
- p = NULL;
+ {
+ for (i = I387_XMM0_REGNUM (tdep);
+ i < I387_MXCSR_REGNUM (tdep);
+ i++)
+ regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+ }
else
- p = regs;
-
- for (i = I387_XMM0_REGNUM (tdep);
- i < I387_MXCSR_REGNUM (tdep); i++)
{
- if (p != NULL)
- p = FXSAVE_ADDR (tdep, regs, i);
- regcache_raw_supply (regcache, i, p);
+ for (i = I387_XMM0_REGNUM (tdep);
+ i < I387_MXCSR_REGNUM (tdep); i++)
+ regcache_raw_supply (regcache, i,
+ FXSAVE_ADDR (tdep, regs, i));
}
}
@@ -832,16 +851,18 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
if ((tdep->xcr0 & I386_XSTATE_X87))
{
if ((clear_bv & I386_XSTATE_X87))
- p = NULL;
+ {
+ for (i = I387_ST0_REGNUM (tdep);
+ i < I387_FCTRL_REGNUM (tdep);
+ i++)
+ regcache_raw_supply (regcache, i, regs != NULL ? zero : NULL);
+ }
else
- p = regs;
-
- for (i = I387_ST0_REGNUM (tdep);
- i < I387_FCTRL_REGNUM (tdep); i++)
{
- if (p != NULL)
- p = FXSAVE_ADDR (tdep, regs, i);
- regcache_raw_supply (regcache, i, p);
+ for (i = I387_ST0_REGNUM (tdep);
+ i < I387_FCTRL_REGNUM (tdep);
+ i++)
+ regcache_raw_supply (regcache, i, FXSAVE_ADDR (tdep, regs, i));
}
}
break;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: AVX and unavailable registers, fix system-gcore.exp
2012-01-26 19:16 AVX and unavailable registers, fix system-gcore.exp Pedro Alves
@ 2012-02-27 18:49 ` H.J. Lu
2012-02-27 19:23 ` Pedro Alves
2012-03-05 14:32 ` i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL Pedro Alves
1 sibling, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2012-02-27 18:49 UTC (permalink / raw)
To: Pedro Alves; +Cc: GDB Patches, Mark Kettenis
On Thu, Jan 26, 2012 at 11:07 AM, Pedro Alves <palves@redhat.com> wrote:
> I now have access to a machine with AVX, and I'm seeing a few core related
> failures on native x86_64 Fedora 16. GDBserver doesn't show the failures because
> it treats this issue already like the patch below proposes.
>
> This was discussed a while ago, in the "RFC: partially available registers"
> thread:
>
> http://old.nabble.com/RFC%3A-partially-available-registers-td32055573i20.html
>
> The failures I'm seeing look like, e.g.:
>
> FAIL: gdb.arch/system-gcore.exp: corefile restored all registers
>
> because before the core, the program showed:
>
> ...
> ymm0 *value not available*
> ...
>
> and when we load back the core we see:
>
> ...
> ymm0 {v8_float = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_double = {0x0, 0x0, 0x0, 0x0}, v32_int8 = {0x0 <repeats 32 time
> s>}, v16_int16 = {0x0 <repeats 16 times>}, v8_int32 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_int64 = {0x0, 0x0, 0x0, 0x0}, v2_int
> 128 = {0x00000000000000000000000000000000, 0x00000000000000000000000000000000}}
> ...
>
>
> I think we should make live debugging present the still-not-touched x87
> states as zero, as described in the comment in the patch:
>
> + /* With the delayed xsave mechanism, in between the program
> + starting, and the program accessing the vector registers for the
> + first time, the register's values are invalid. The kernel
> + initializes register states to zero when they are set the first
> + time in a program. This means that from the user-space programs'
> + perspective, it's the same as if the registers have always been
> + zero from the start of the program. Therefore, the debugger
> + should provide the same illusion to the user.
>
> This is already handled this way with gdbserver, hence we don't see
> these failures there (the gcore tests now work against gdbserver).
> The kernel is also putting the registers touched as zero
> in the core, otherwise we'd see *unavailable* when we loaded the
> core file back.
>
>
> I have a doubt in the xsave-in-corefile support bits. There's code in place to
> handle a NULL regs (as in no xsave contents to work with), so I'm handling it
> as presently:
>
> +
> + Note however, the case when REGS is NULL is a different case.
> + That case means we do not have access to the x87 states, so we
> + should mark the registers as unavailable (by supplying NULL). */
> +
>
> but I can't figure out how would we ever get a NULL REGS there. Is there a
> convoluted path I missed? amd64-linux-tdep.c unconditionally installs
> amd64_linux_regset_sections as gdbarch_core_regset_sections
> callback, and this includes the .reg-xstate section.
> However, corelow.c:get_core_register_section bails early if
> a section is not found in the core, never reaching regset->supply_regset
> with a NULL `contents'.
>
> Mark, maybe you have a clue?
>
> --
> Pedro Alves
>
> 2012-01-26 Pedro Alves <palves@redhat.com>
>
> * i387-tdep.c (i387_supply_xsave): If we have an xsave buffer, and
> the register state is clear, supply explicit zero, instead of
> marking the register unavailable.
This fixes 7.3/7.4 regression:
http://www.sourceware.org/bugzilla/show_bug.cgi?id=13766
> ---
>
> gdb/i387-tdep.c | 87 ++++++++++++++++++++++++++++++++++---------------------
> 1 files changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index 450b8f2..b3adb74 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -725,6 +725,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
> const gdb_byte *regs = xsave;
> int i;
> unsigned int clear_bv;
> + const gdb_byte zero[MAX_REGISTER_SIZE] = { 0 };
Shouldn't it be static since it is never modified?
> const gdb_byte *p;
> enum
> {
> @@ -764,6 +765,19 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
> else
> clear_bv = I386_XSTATE_AVX_MASK;
>
> + /* With the delayed xsave mechanism, in between the program
> + starting, and the program accessing the vector registers for the
> + first time, the register's values are invalid. The kernel
> + initializes register states to zero when they are set the first
> + time in a program. This means that from the user-space programs'
> + perspective, it's the same as if the registers have always been
> + zero from the start of the program. Therefore, the debugger
> + should provide the same illusion to the user.
> +
> + Note however, the case when REGS is NULL is a different case.
> + That case means we do not have access to the x87 states, so we
> + should mark the registers as unavailable (by supplying NULL). */
> +
> switch (regclass)
> {
> case none:
> @@ -771,26 +785,26 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
>
> case avxh:
> if ((clear_bv & I386_XSTATE_AVX))
> - p = NULL;
> + regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
> else
> - p = XSAVE_AVXH_ADDR (tdep, regs, regnum);
> - regcache_raw_supply (regcache, regnum, p);
> + regcache_raw_supply (regcache, regnum,
> + XSAVE_AVXH_ADDR (tdep, regs, regnum));
> return;
>
> case sse:
> if ((clear_bv & I386_XSTATE_SSE))
> - p = NULL;
> + regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
> else
> - p = FXSAVE_ADDR (tdep, regs, regnum);
> - regcache_raw_supply (regcache, regnum, p);
> + regcache_raw_supply (regcache, regnum,
> + FXSAVE_ADDR (tdep, regs, regnum));
> return;
>
> case x87:
> if ((clear_bv & I386_XSTATE_X87))
> - p = NULL;
> + regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
> else
> - p = FXSAVE_ADDR (tdep, regs, regnum);
> - regcache_raw_supply (regcache, regnum, p);
> + regcache_raw_supply (regcache, regnum,
> + FXSAVE_ADDR (tdep, regs, regnum));
> return;
>
> case all:
> @@ -798,16 +812,19 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
> if ((tdep->xcr0 & I386_XSTATE_AVX))
> {
> if ((clear_bv & I386_XSTATE_AVX))
> - p = NULL;
> + {
> + for (i = I387_YMM0H_REGNUM (tdep);
> + i < I387_YMMENDH_REGNUM (tdep);
> + i++)
> + regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
> + }
> else
> - p = regs;
> -
> - for (i = I387_YMM0H_REGNUM (tdep);
> - i < I387_YMMENDH_REGNUM (tdep); i++)
> {
> - if (p != NULL)
> - p = XSAVE_AVXH_ADDR (tdep, regs, i);
> - regcache_raw_supply (regcache, i, p);
> + for (i = I387_YMM0H_REGNUM (tdep);
> + i < I387_YMMENDH_REGNUM (tdep);
> + i++)
> + regcache_raw_supply (regcache, i,
> + XSAVE_AVXH_ADDR (tdep, regs, i));
> }
> }
>
> @@ -815,16 +832,18 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
> if ((tdep->xcr0 & I386_XSTATE_SSE))
> {
> if ((clear_bv & I386_XSTATE_SSE))
> - p = NULL;
> + {
> + for (i = I387_XMM0_REGNUM (tdep);
> + i < I387_MXCSR_REGNUM (tdep);
> + i++)
> + regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
> + }
> else
> - p = regs;
> -
> - for (i = I387_XMM0_REGNUM (tdep);
> - i < I387_MXCSR_REGNUM (tdep); i++)
> {
> - if (p != NULL)
> - p = FXSAVE_ADDR (tdep, regs, i);
> - regcache_raw_supply (regcache, i, p);
> + for (i = I387_XMM0_REGNUM (tdep);
> + i < I387_MXCSR_REGNUM (tdep); i++)
> + regcache_raw_supply (regcache, i,
> + FXSAVE_ADDR (tdep, regs, i));
> }
> }
>
> @@ -832,16 +851,18 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
> if ((tdep->xcr0 & I386_XSTATE_X87))
> {
> if ((clear_bv & I386_XSTATE_X87))
> - p = NULL;
> + {
> + for (i = I387_ST0_REGNUM (tdep);
> + i < I387_FCTRL_REGNUM (tdep);
> + i++)
> + regcache_raw_supply (regcache, i, regs != NULL ? zero : NULL);
> + }
> else
> - p = regs;
> -
> - for (i = I387_ST0_REGNUM (tdep);
> - i < I387_FCTRL_REGNUM (tdep); i++)
> {
> - if (p != NULL)
> - p = FXSAVE_ADDR (tdep, regs, i);
> - regcache_raw_supply (regcache, i, p);
> + for (i = I387_ST0_REGNUM (tdep);
> + i < I387_FCTRL_REGNUM (tdep);
> + i++)
> + regcache_raw_supply (regcache, i, FXSAVE_ADDR (tdep, regs, i));
> }
> }
> break;
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: AVX and unavailable registers, fix system-gcore.exp
2012-02-27 18:49 ` H.J. Lu
@ 2012-02-27 19:23 ` Pedro Alves
2012-03-02 21:49 ` H.J. Lu
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2012-02-27 19:23 UTC (permalink / raw)
To: H.J. Lu; +Cc: GDB Patches, Mark Kettenis
On 02/27/2012 06:04 PM, H.J. Lu wrote:
> On Thu, Jan 26, 2012 at 11:07 AM, Pedro Alves <palves@redhat.com> wrote:
>> 2012-01-26 Pedro Alves <palves@redhat.com>
>>
>> * i387-tdep.c (i387_supply_xsave): If we have an xsave buffer, and
>> the register state is clear, supply explicit zero, instead of
>> marking the register unavailable.
>
> This fixes 7.3/7.4 regression:
>
> http://www.sourceware.org/bugzilla/show_bug.cgi?id=13766
Thanks for confirming. Since you agree with the patch, I'll check it in in
a few days unless I hear otherwise. I'll post a separate RFC patch proposing
to remove the NULL REGS handling:
>> I have a doubt in the xsave-in-corefile support bits. There's code in place to
>> handle a NULL regs (as in no xsave contents to work with), so I'm handling it
>> as presently:
>>
>>
>> +
>> + Note however, the case when REGS is NULL is a different case.
>> + That case means we do not have access to the x87 states, so we
>> + should mark the registers as unavailable (by supplying NULL). */
>> +
>> but I can't figure out how would we ever get a NULL REGS there. Is there a
>> convoluted path I missed? amd64-linux-tdep.c unconditionally installs
>> amd64_linux_regset_sections as gdbarch_core_regset_sections
>> callback, and this includes the .reg-xstate section.
>> However, corelow.c:get_core_register_section bails early if
>> a section is not found in the core, never reaching regset->supply_regset
>> with a NULL `contents'.
>> + const gdb_byte zero[MAX_REGISTER_SIZE] = { 0 };
>
> Shouldn't it be static since it is never modified?
I'll do that change.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: AVX and unavailable registers, fix system-gcore.exp
2012-02-27 19:23 ` Pedro Alves
@ 2012-03-02 21:49 ` H.J. Lu
2012-03-05 14:24 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2012-03-02 21:49 UTC (permalink / raw)
To: Pedro Alves; +Cc: GDB Patches, Mark Kettenis
On Mon, Feb 27, 2012 at 10:48 AM, Pedro Alves <palves@redhat.com> wrote:
> On 02/27/2012 06:04 PM, H.J. Lu wrote:
>> On Thu, Jan 26, 2012 at 11:07 AM, Pedro Alves <palves@redhat.com> wrote:
>
>>> 2012-01-26 Pedro Alves <palves@redhat.com>
>>>
>>> * i387-tdep.c (i387_supply_xsave): If we have an xsave buffer, and
>>> the register state is clear, supply explicit zero, instead of
>>> marking the register unavailable.
>>
>> This fixes 7.3/7.4 regression:
>>
>> http://www.sourceware.org/bugzilla/show_bug.cgi?id=13766
>
> Thanks for confirming. Since you agree with the patch, I'll check it in in
> a few days unless I hear otherwise. I'll post a separate RFC patch proposing
> to remove the NULL REGS handling:
Has this fix been checked in?
Thanks.
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: AVX and unavailable registers, fix system-gcore.exp
2012-03-02 21:49 ` H.J. Lu
@ 2012-03-05 14:24 ` Pedro Alves
2012-03-06 13:22 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2012-03-05 14:24 UTC (permalink / raw)
To: H.J. Lu; +Cc: GDB Patches, Mark Kettenis
On 03/02/2012 09:49 PM, H.J. Lu wrote:
> On Mon, Feb 27, 2012 at 10:48 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 02/27/2012 06:04 PM, H.J. Lu wrote:
>>> On Thu, Jan 26, 2012 at 11:07 AM, Pedro Alves <palves@redhat.com> wrote:
>>
>>>> 2012-01-26 Pedro Alves <palves@redhat.com>
>>>>
>>>> * i387-tdep.c (i387_supply_xsave): If we have an xsave buffer, and
>>>> the register state is clear, supply explicit zero, instead of
>>>> marking the register unavailable.
>>>
>>> This fixes 7.3/7.4 regression:
>>>
>>> http://www.sourceware.org/bugzilla/show_bug.cgi?id=13766
>>
>> Thanks for confirming. Since you agree with the patch, I'll check it in in
>> a few days unless I hear otherwise. I'll post a separate RFC patch proposing
>> to remove the NULL REGS handling:
>
> Has this fix been checked in?
It's in now.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL.
2012-01-26 19:16 AVX and unavailable registers, fix system-gcore.exp Pedro Alves
2012-02-27 18:49 ` H.J. Lu
@ 2012-03-05 14:32 ` Pedro Alves
2012-03-05 15:51 ` Mark Kettenis
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2012-03-05 14:32 UTC (permalink / raw)
To: GDB Patches; +Cc: Mark Kettenis, H.J. Lu
On 01/26/2012 07:07 PM, Pedro Alves wrote:
>
> I have a doubt in the xsave-in-corefile support bits. There's code in place to
> handle a NULL regs (as in no xsave contents to work with), so I'm handling it
> as presently:
>
> +
> + Note however, the case when REGS is NULL is a different case.
> + That case means we do not have access to the x87 states, so we
> + should mark the registers as unavailable (by supplying NULL). */
> +
>
> but I can't figure out how would we ever get a NULL REGS there. Is there a
> convoluted path I missed? amd64-linux-tdep.c unconditionally installs
> amd64_linux_regset_sections as gdbarch_core_regset_sections
> callback, and this includes the .reg-xstate section.
> However, corelow.c:get_core_register_section bails early if
> a section is not found in the core, never reaching regset->supply_regset
> with a NULL `contents'.
On 02/27/2012 06:48 PM, Pedro Alves wrote:
> I'll post a separate RFC patch proposing to remove the NULL REGS handling
Here's what I'd like to check in, given the analysis quoted above.
2012-03-05 Pedro Alves <palves@redhat.com>
* i387-tdep.c (i387_supply_xsave): Assert the xsave section buffer
is not NULL, and remove resulting dead code.
---
gdb/i387-tdep.c | 33 +++++++++++----------------------
1 files changed, 11 insertions(+), 22 deletions(-)
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index 3fc344d..26e4e50 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -790,6 +790,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
all = x87 | sse | avxh
} regclass;
+ gdb_assert (regs != NULL);
gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
gdb_assert (tdep->num_xmm_regs > 0);
@@ -807,7 +808,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
else
regclass = none;
- if (regs != NULL && regclass != none)
+ if (regclass != none)
{
/* Get `xstat_bv'. */
const gdb_byte *xstate_bv_p = XSAVE_XSTATE_BV_ADDR (regs);
@@ -826,11 +827,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
time in a program. This means that from the user-space programs'
perspective, it's the same as if the registers have always been
zero from the start of the program. Therefore, the debugger
- should provide the same illusion to the user.
-
- Note however, the case when REGS is NULL is a different case.
- That case means we do not have access to the x87 states, so we
- should mark the registers as unavailable (by supplying NULL). */
+ should provide the same illusion to the user. */
switch (regclass)
{
@@ -839,7 +836,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
case avxh:
if ((clear_bv & I386_XSTATE_AVX))
- regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
+ regcache_raw_supply (regcache, regnum, zero);
else
regcache_raw_supply (regcache, regnum,
XSAVE_AVXH_ADDR (tdep, regs, regnum));
@@ -847,7 +844,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
case sse:
if ((clear_bv & I386_XSTATE_SSE))
- regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
+ regcache_raw_supply (regcache, regnum, zero);
else
regcache_raw_supply (regcache, regnum,
FXSAVE_ADDR (tdep, regs, regnum));
@@ -855,7 +852,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
case x87:
if ((clear_bv & I386_XSTATE_X87))
- regcache_raw_supply (regcache, regnum, regs == NULL ? NULL : zero);
+ regcache_raw_supply (regcache, regnum, zero);
else
regcache_raw_supply (regcache, regnum,
FXSAVE_ADDR (tdep, regs, regnum));
@@ -870,7 +867,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
for (i = I387_YMM0H_REGNUM (tdep);
i < I387_YMMENDH_REGNUM (tdep);
i++)
- regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+ regcache_raw_supply (regcache, i, zero);
}
else
{
@@ -890,7 +887,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
for (i = I387_XMM0_REGNUM (tdep);
i < I387_MXCSR_REGNUM (tdep);
i++)
- regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+ regcache_raw_supply (regcache, i, zero);
}
else
{
@@ -909,7 +906,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
for (i = I387_ST0_REGNUM (tdep);
i < I387_FCTRL_REGNUM (tdep);
i++)
- regcache_raw_supply (regcache, i, regs == NULL ? NULL : zero);
+ regcache_raw_supply (regcache, i, zero);
}
else
{
@@ -926,12 +923,6 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
for (i = I387_FCTRL_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
if (regnum == -1 || regnum == i)
{
- if (regs == NULL)
- {
- regcache_raw_supply (regcache, i, NULL);
- continue;
- }
-
/* Most of the FPU control registers occupy only 16 bits in
the xsave extended state. Give those a special treatment. */
if (i != I387_FIOFF_REGNUM (tdep)
@@ -982,10 +973,8 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
}
if (regnum == I387_MXCSR_REGNUM (tdep) || regnum == -1)
- {
- p = regs == NULL ? NULL : FXSAVE_MXCSR_ADDR (regs);
- regcache_raw_supply (regcache, I387_MXCSR_REGNUM (tdep), p);
- }
+ regcache_raw_supply (regcache, I387_MXCSR_REGNUM (tdep),
+ FXSAVE_MXCSR_ADDR (regs));
}
/* Similar to i387_collect_fxsave, but use XSAVE extended state. */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL.
2012-03-05 14:32 ` i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL Pedro Alves
@ 2012-03-05 15:51 ` Mark Kettenis
2012-03-05 16:19 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Mark Kettenis @ 2012-03-05 15:51 UTC (permalink / raw)
To: palves; +Cc: gdb-patches, hjl.tools
> Date: Mon, 05 Mar 2012 14:32:32 +0000
> From: Pedro Alves <palves@redhat.com>
>
> On 01/26/2012 07:07 PM, Pedro Alves wrote:
>
> >
> > I have a doubt in the xsave-in-corefile support bits. There's
> > code in place to handle a NULL regs (as in no xsave contents to
> > work with), so I'm handling it as presently:
> >
> > +
> > + Note however, the case when REGS is NULL is a different case.
> > + That case means we do not have access to the x87 states, so we
> > + should mark the registers as unavailable (by supplying NULL). */
> > +
> >
> > but I can't figure out how would we ever get a NULL REGS there. Is there a
> > convoluted path I missed? amd64-linux-tdep.c unconditionally installs
> > amd64_linux_regset_sections as gdbarch_core_regset_sections
> > callback, and this includes the .reg-xstate section.
> > However, corelow.c:get_core_register_section bails early if
> > a section is not found in the core, never reaching regset->supply_regset
> > with a NULL `contents'.
>
>
> On 02/27/2012 06:48 PM, Pedro Alves wrote:
> > I'll post a separate RFC patch proposing to remove the NULL REGS handling
>
> Here's what I'd like to check in, given the analysis quoted above.
I agree with your analysis.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL.
2012-03-05 15:51 ` Mark Kettenis
@ 2012-03-05 16:19 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2012-03-05 16:19 UTC (permalink / raw)
To: Mark Kettenis; +Cc: gdb-patches, hjl.tools
Hi Mark,
On 03/05/2012 03:51 PM, Mark Kettenis wrote:
> I agree with your analysis.
Thanks for looking it over. I've now checked the patch in.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: AVX and unavailable registers, fix system-gcore.exp
2012-03-05 14:24 ` Pedro Alves
@ 2012-03-06 13:22 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2012-03-06 13:22 UTC (permalink / raw)
To: H.J. Lu; +Cc: GDB Patches
On 03/05/2012 02:24 PM, Pedro Alves wrote:
>>> On 02/27/2012 06:04 PM, H.J. Lu wrote:
>>>> This fixes 7.3/7.4 regression:
>>>>
>>>> http://www.sourceware.org/bugzilla/show_bug.cgi?id=13766
>
> It's in now.
... and now applied to 7.4 and 7.3 as well.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-03-06 13:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-26 19:16 AVX and unavailable registers, fix system-gcore.exp Pedro Alves
2012-02-27 18:49 ` H.J. Lu
2012-02-27 19:23 ` Pedro Alves
2012-03-02 21:49 ` H.J. Lu
2012-03-05 14:24 ` Pedro Alves
2012-03-06 13:22 ` Pedro Alves
2012-03-05 14:32 ` i387-tdep.c:i387_supply_xsave: Assert the xsave section buffer, is not NULL Pedro Alves
2012-03-05 15:51 ` Mark Kettenis
2012-03-05 16:19 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox