| Message ID | 20110306171830.GA18591@intel.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: > Hi, > > We shouldn't save call frame hard registers as "void *". This patch > changes the unwind library to save call frame hard registers as > _Unwind_Word. OK for 4.7? I think this will break the ABI for the MIPS N32 ABI. Not to mention the MIPS N32 ABI works fine with the unwinding part this way. Does someone use the unwinding library to look at the registers in previous stack frames? -- Pinski
On Sun, Mar 6, 2011 at 1:15 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> Hi, >> >> We shouldn't save call frame hard registers as "void *". This patch >> changes the unwind library to save call frame hard registers as >> _Unwind_Word. OK for 4.7? > > I think this will break the ABI for the MIPS N32 ABI. Not to mention > the MIPS N32 ABI works fine with the unwinding part this way. Does > someone use the unwinding library to look at the registers in previous > stack frames? It may be psABI/implementation specific. X32 glibc force unwind calls _Unwind_SetGRValue to get a 64bit register value.
On Sun, Mar 6, 2011 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Sun, Mar 6, 2011 at 1:15 PM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>> Hi, >>> >>> We shouldn't save call frame hard registers as "void *". This patch >>> changes the unwind library to save call frame hard registers as >>> _Unwind_Word. OK for 4.7? >> >> I think this will break the ABI for the MIPS N32 ABI. Not to mention >> the MIPS N32 ABI works fine with the unwinding part this way. Does >> someone use the unwinding library to look at the registers in previous >> stack frames? > > It may be psABI/implementation specific. X32 glibc force unwind calls > _Unwind_SetGRValue to get a 64bit register value. So fix it on that side? Richard.
On Sun, Mar 6, 2011 at 3:23 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Sun, Mar 6, 2011 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Sun, Mar 6, 2011 at 1:15 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>>> Hi, >>>> >>>> We shouldn't save call frame hard registers as "void *". This patch >>>> changes the unwind library to save call frame hard registers as >>>> _Unwind_Word. OK for 4.7? >>> >>> I think this will break the ABI for the MIPS N32 ABI. Not to mention >>> the MIPS N32 ABI works fine with the unwinding part this way. Does >>> someone use the unwinding library to look at the registers in previous >>> stack frames? >> >> It may be psABI/implementation specific. X32 glibc force unwind calls >> _Unwind_SetGRValue to get a 64bit register value. > > So fix it on that side? > How? One function in question is /* Overwrite the saved value for register INDEX in CONTEXT with VAL. */ static inline void _Unwind_SetGRValue (struct _Unwind_Context *context, int index, _Unwind_Word val) Are you saying it shouldn't be called if UNITS_PER_WORD > sizeof (void *)?
On Sun, Mar 6, 2011 at 3:40 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Sun, Mar 6, 2011 at 3:23 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Sun, Mar 6, 2011 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Sun, Mar 6, 2011 at 1:15 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>>> On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>>>> Hi, >>>>> >>>>> We shouldn't save call frame hard registers as "void *". This patch >>>>> changes the unwind library to save call frame hard registers as >>>>> _Unwind_Word. OK for 4.7? >>>> >>>> I think this will break the ABI for the MIPS N32 ABI. Not to mention >>>> the MIPS N32 ABI works fine with the unwinding part this way. Does >>>> someone use the unwinding library to look at the registers in previous >>>> stack frames? >>> >>> It may be psABI/implementation specific. X32 glibc force unwind calls >>> _Unwind_SetGRValue to get a 64bit register value. >> >> So fix it on that side? >> > > How? One function in question is > > /* Overwrite the saved value for register INDEX in CONTEXT with VAL. */ > > static inline void > _Unwind_SetGRValue (struct _Unwind_Context *context, int index, > _Unwind_Word val) > > > Are you saying it shouldn't be called if UNITS_PER_WORD > sizeof (void *)? > FWIW, the type of GR is _Unwind_Word, not void *. They may not have the same size. Why does the DWARF unwind library use void * to store GR? Can a target have an option to save a _Unwind_Word value in _Unwind_Word, instead of void *? Thanks.
On Sun, Mar 6, 2011 at 4:15 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Sun, Mar 6, 2011 at 3:40 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Sun, Mar 6, 2011 at 3:23 PM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Sun, Mar 6, 2011 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> On Sun, Mar 6, 2011 at 1:15 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>>>> On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>>>>> Hi, >>>>>> >>>>>> We shouldn't save call frame hard registers as "void *". This patch >>>>>> changes the unwind library to save call frame hard registers as >>>>>> _Unwind_Word. OK for 4.7? >>>>> >>>>> I think this will break the ABI for the MIPS N32 ABI. Not to mention >>>>> the MIPS N32 ABI works fine with the unwinding part this way. Does >>>>> someone use the unwinding library to look at the registers in previous >>>>> stack frames? >>>> >>>> It may be psABI/implementation specific. X32 glibc force unwind calls >>>> _Unwind_SetGRValue to get a 64bit register value. >>> >>> So fix it on that side? >>> >> >> How? One function in question is >> >> /* Overwrite the saved value for register INDEX in CONTEXT with VAL. */ >> >> static inline void >> _Unwind_SetGRValue (struct _Unwind_Context *context, int index, >> _Unwind_Word val) >> >> >> Are you saying it shouldn't be called if UNITS_PER_WORD > sizeof (void *)? >> > > FWIW, the type of GR is _Unwind_Word, not void *. They may not > have the same size. Why does the DWARF unwind library use > void * to store GR? Can a target have an option to save a _Unwind_Word > value in _Unwind_Word, instead of void *? > From what I can tell, _Unwind_Context is internal to the unwind library and only one copy unwind library can be used in a process. I don't think changing _Unwind_Context will impact binary compatibility. Is my patch OK for 4.7? Thanks.
On Sun, Mar 6, 2011 at 9:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: > We shouldn't save call frame hard registers as "void *". This patch > changes the unwind library to save call frame hard registers as > _Unwind_Word. OK for 4.7? > + PR other/48007 > + * unwind-dw2.c (_Unwind_Context): Save call frame hard registers > + as _Unwind_Word. > + (_Unwind_GetGR): Get GR value as _Unwind_Word. > + (_Unwind_SetGR): Set GR value as _Unwind_Word. > + (_Unwind_SetGRValue): Likewise. > + (_Unwind_GetGRPtr): Cast return to "void *". > + (_Unwind_SetGRPtr): Cast pointer to _Unwind_Word. > + (uw_install_context_1): Cast pointer to "void *". The approach you are using here looks wrong to me. When looking at the DWARF2 _Unwind_Context, you have to look at the by_value field for the register. If by_value is true, then the reg field for that register holds an _Unwind_Word which is the value of the register. If by_value is false, then the reg field holds a pointer to the place where the actual value is stored. The size of the actual value is determined by dwarf_reg_size_table. In other words, the reg field is a really a union of _Unwind_Word and void*. You have correctly observed that the current code fails for the case where sizeof(_Unwind_Word) > sizeof(void*). However, the answer is not to change the type from void* to _Unwind_Word. That will fail when sizeof(void*) > sizeof(_Unwind_Word). I think you have to make the reg field a union. By the way, don't use intptr_t in the unwind code. Use _Unwind_Internal_Ptr. But I think if you make the field a union, you won't need to use either type. Ian
diff --git a/gcc/ChangeLog.x32 b/gcc/ChangeLog.x32 index 5389f19..022767b 100644 --- a/gcc/ChangeLog.x32 +++ b/gcc/ChangeLog.x32 @@ -1,3 +1,15 @@ +2011-03-06 H.J. Lu <hongjiu.lu@intel.com> + + PR other/48007 + * unwind-dw2.c (_Unwind_Context): Save call frame hard registers + as _Unwind_Word. + (_Unwind_GetGR): Get GR value as _Unwind_Word. + (_Unwind_SetGR): Set GR value as _Unwind_Word. + (_Unwind_SetGRValue): Likewise. + (_Unwind_GetGRPtr): Cast return to "void *". + (_Unwind_SetGRPtr): Cast pointer to _Unwind_Word. + (uw_install_context_1): Cast pointer to "void *". + 2011-03-05 H.J. Lu <hongjiu.lu@intel.com> * config/i386/linux-unwind.h (x86_64_fallback_frame_state): diff --git a/gcc/unwind-dw2.c b/gcc/unwind-dw2.c index 2ea9adb..229c373 100644 --- a/gcc/unwind-dw2.c +++ b/gcc/unwind-dw2.c @@ -60,7 +60,7 @@ to its caller. */ struct _Unwind_Context { - void *reg[DWARF_FRAME_REGISTERS+1]; + _Unwind_Word reg[DWARF_FRAME_REGISTERS+1]; void *cfa; void *ra; void *lsda; @@ -152,7 +152,7 @@ inline _Unwind_Word _Unwind_GetGR (struct _Unwind_Context *context, int index) { int size; - void *ptr; + _Unwind_Word val; #ifdef DWARF_ZERO_REG if (index == DWARF_ZERO_REG) @@ -162,18 +162,18 @@ _Unwind_GetGR (struct _Unwind_Context *context, int index) index = DWARF_REG_TO_UNWIND_COLUMN (index); gcc_assert (index < (int) sizeof(dwarf_reg_size_table)); size = dwarf_reg_size_table[index]; - ptr = context->reg[index]; + val = context->reg[index]; if (_Unwind_IsExtendedContext (context) && context->by_value[index]) - return (_Unwind_Word) (_Unwind_Internal_Ptr) ptr; + return val; /* This will segfault if the register hasn't been saved. */ if (size == sizeof(_Unwind_Ptr)) - return * (_Unwind_Ptr *) ptr; + return * (_Unwind_Ptr *) (intptr_t) val; else { gcc_assert (size == sizeof(_Unwind_Word)); - return * (_Unwind_Word *) ptr; + return * (_Unwind_Word *) (intptr_t) val; } } @@ -205,11 +205,11 @@ _Unwind_SetGR (struct _Unwind_Context *context, int index, _Unwind_Word val) if (_Unwind_IsExtendedContext (context) && context->by_value[index]) { - context->reg[index] = (void *) (_Unwind_Internal_Ptr) val; + context->reg[index] = val; return; } - ptr = context->reg[index]; + ptr = (void *) (intptr_t) context->reg[index]; if (size == sizeof(_Unwind_Ptr)) * (_Unwind_Ptr *) ptr = val; @@ -227,8 +227,8 @@ _Unwind_GetGRPtr (struct _Unwind_Context *context, int index) { index = DWARF_REG_TO_UNWIND_COLUMN (index); if (_Unwind_IsExtendedContext (context) && context->by_value[index]) - return &context->reg[index]; - return context->reg[index]; + return (void *) (intptr_t) &context->reg[index]; + return (void *) (intptr_t) context->reg[index]; } /* Set the pointer to a register INDEX as saved in CONTEXT. */ @@ -239,7 +239,7 @@ _Unwind_SetGRPtr (struct _Unwind_Context *context, int index, void *p) index = DWARF_REG_TO_UNWIND_COLUMN (index); if (_Unwind_IsExtendedContext (context)) context->by_value[index] = 0; - context->reg[index] = p; + context->reg[index] = (_Unwind_Word) (intptr_t) p; } /* Overwrite the saved value for register INDEX in CONTEXT with VAL. */ @@ -250,10 +250,10 @@ _Unwind_SetGRValue (struct _Unwind_Context *context, int index, { index = DWARF_REG_TO_UNWIND_COLUMN (index); gcc_assert (index < (int) sizeof(dwarf_reg_size_table)); - gcc_assert (dwarf_reg_size_table[index] == sizeof (_Unwind_Ptr)); + gcc_assert (dwarf_reg_size_table[index] == sizeof (_Unwind_Word)); context->by_value[index] = 1; - context->reg[index] = (void *) (_Unwind_Internal_Ptr) val; + context->reg[index] = val; } /* Return nonzero if register INDEX is stored by value rather than @@ -1524,8 +1524,8 @@ uw_install_context_1 (struct _Unwind_Context *current, for (i = 0; i < DWARF_FRAME_REGISTERS; ++i) { - void *c = current->reg[i]; - void *t = target->reg[i]; + void *c = (void *) (intptr_t) current->reg[i]; + void *t = (void *) (intptr_t) target->reg[i]; gcc_assert (current->by_value[i] == 0); if (target->by_value[i] && c)
Hi, We shouldn't save call frame hard registers as "void *". This patch changes the unwind library to save call frame hard registers as _Unwind_Word. OK for 4.7? Thanks. H.J. ---- commit 4163355471bfb192fdacc5da1ceb9aec5569ff94 Author: H.J. Lu <hjl.tools@gmail.com> Date: Sun Mar 6 08:19:25 2011 -0800 Save call frame hard registers as _Unwind_Word.