PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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.

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)

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Andrew Pinski]

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

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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.


-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Richard Guenther]

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.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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 *)?

-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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.

-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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.


-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

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

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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

On Mon, Mar 21, 2011 at 2:55 PM, Ian Lance Taylor <iant@google.com> wrote:
> 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
>
Here is the updated patch. It has

@@ -243,7 +250,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].ref = p;
 }

Is that OK to take the address of a union member?


-- 
H.J.
--
2011-03-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR other/48007
	* unwind-dw2.c (_Unwind_Register): New.
	(_Unwind_Context): Save call frame hard registers as
	_Unwind_Register.
	(_Unwind_GetGR): Updated.
	(_Unwind_SetGR): Likewise.
	(_Unwind_SetGRValue): Likewise.
	(_Unwind_GetGRPtr): Likewise.
	(_Unwind_SetGRPtr): Likewise.
	(uw_install_context_1): Likewise.

[-- Attachment #2: gcc-pr48007-1.patch --]
[-- Type: text/plain, Size: 4140 bytes --]

2011-03-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR other/48007
	* unwind-dw2.c (_Unwind_Register): New.
	(_Unwind_Context): Save call frame hard registers as
	_Unwind_Register.
	(_Unwind_GetGR): Updated.
	(_Unwind_SetGR): Likewise.
	(_Unwind_SetGRValue): Likewise.
	(_Unwind_GetGRPtr): Likewise.
	(_Unwind_SetGRPtr): Likewise.
	(uw_install_context_1): Likewise.

diff --git a/gcc/unwind-dw2.c b/gcc/unwind-dw2.c
index 25990b4..1cb4b47 100644
--- a/gcc/unwind-dw2.c
+++ b/gcc/unwind-dw2.c
@@ -59,12 +59,19 @@
 #define DWARF_REG_TO_UNWIND_COLUMN(REGNO) (REGNO)
 #endif
 
+/* A reister is stored either by reference or by value.  */
+union _Unwind_Register
+{
+  void *ref;
+  _Unwind_Word val;
+};
+
 /* This is the register and unwind state for a particular frame.  This
    provides the information necessary to unwind up past a frame and return
    to its caller.  */
 struct _Unwind_Context
 {
-  void *reg[DWARF_FRAME_REGISTERS+1];
+  union _Unwind_Register reg[DWARF_FRAME_REGISTERS+1];
   void *cfa;
   void *ra;
   void *lsda;
@@ -156,7 +163,7 @@ inline _Unwind_Word
 _Unwind_GetGR (struct _Unwind_Context *context, int index)
 {
   int size;
-  void *ptr;
+  union _Unwind_Register reg;
 
 #ifdef DWARF_ZERO_REG
   if (index == DWARF_ZERO_REG)
@@ -166,18 +173,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];
+  reg = context->reg[index];
 
   if (_Unwind_IsExtendedContext (context) && context->by_value[index])
-    return (_Unwind_Word) (_Unwind_Internal_Ptr) ptr;
+    return reg.val;
 
   /* This will segfault if the register hasn't been saved.  */
   if (size == sizeof(_Unwind_Ptr))
-    return * (_Unwind_Ptr *) ptr;
+    return * (_Unwind_Ptr *) reg.ref;
   else
     {
       gcc_assert (size == sizeof(_Unwind_Word));
-      return * (_Unwind_Word *) ptr;
+      return * (_Unwind_Word *) reg.ref;
     }
 }
 
@@ -209,11 +216,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 = val;
       return;
     }
 
-  ptr = context->reg[index];
+  ptr = context->reg[index].ref;
 
   if (size == sizeof(_Unwind_Ptr))
     * (_Unwind_Ptr *) ptr = val;
@@ -231,8 +238,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 *) &context->reg[index].val;
+  return context->reg[index].ref;
 }
 
 /* Set the pointer to a register INDEX as saved in CONTEXT.  */
@@ -243,7 +250,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].ref = p;
 }
 
 /* Overwrite the saved value for register INDEX in CONTEXT with VAL.  */
@@ -254,10 +261,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 = val;
 }
 
 /* Return nonzero if register INDEX is stored by value rather than
@@ -1534,8 +1541,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 = current->reg[i].ref;
+      void *t = target->reg[i].ref;
 
       gcc_assert (current->by_value[i] == 0);
       if (target->by_value[i] && c)

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> Here is the updated patch. It has

This patch is OK if it bootstraps and passes tests.

Thanks.

Ian

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ulrich Weigand]

Ian Lance Taylor wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> 
> > Here is the updated patch. It has
> 
> This patch is OK if it bootstraps and passes tests.

I thought the _Unwind_Context structure was part of the libgcc ABI
and should only be changed by appending to the end and/or updating
the version field?

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Jakub Jelinek]

On Tue, Mar 22, 2011 at 04:19:46PM +0100, Ulrich Weigand wrote:
> Ian Lance Taylor wrote:
> > "H.J. Lu" <hjl.tools@gmail.com> writes:
> > 
> > > Here is the updated patch. It has
> > 
> > This patch is OK if it bootstraps and passes tests.
> 
> I thought the _Unwind_Context structure was part of the libgcc ABI
> and should only be changed by appending to the end and/or updating
> the version field?

It is, so a change like this should be guarded by some preprocessor
conditionals if it makes any difference on any target (not sure if
we have any targets where a union containing void * and uintptr_t
would be layed out differently in a structure from void *, but
certainly on targets where _Unwind_Word is larger than void *
and they aren't new targets we risk ABI issues).
The problem is when some binaries link against older libgcc.a
and are used together with dynamically linked libgcc_s.so.

	Jakub

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> On Tue, Mar 22, 2011 at 04:19:46PM +0100, Ulrich Weigand wrote:
>> Ian Lance Taylor wrote:
>> > "H.J. Lu" <hjl.tools@gmail.com> writes:
>> > 
>> > > Here is the updated patch. It has
>> > 
>> > This patch is OK if it bootstraps and passes tests.
>> 
>> I thought the _Unwind_Context structure was part of the libgcc ABI
>> and should only be changed by appending to the end and/or updating
>> the version field?
>
> It is, so a change like this should be guarded by some preprocessor
> conditionals if it makes any difference on any target (not sure if
> we have any targets where a union containing void * and uintptr_t
> would be layed out differently in a structure from void *, but
> certainly on targets where _Unwind_Word is larger than void *
> and they aren't new targets we risk ABI issues).
> The problem is when some binaries link against older libgcc.a
> and are used together with dynamically linked libgcc_s.so.

Any target on which _Unwind_Word is larger than void * is broken today,
so I don't think we need to care about that case.

But, yes, if there is a target in which the union would be laid out
differently than the simple field, then there is a problem.  I think
this is an unlikely case.  Does anybody know of a target where that
might occur?

Ian

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Andrew Pinski]

On Tue, Mar 22, 2011 at 9:42 AM, Ian Lance Taylor <iant@google.com> wrote:
>
> Any target on which _Unwind_Word is larger than void * is broken today,
> so I don't think we need to care about that case.

So a MIPS N32 is broken?  Lots of people use that target already and
nothing like this has showed up yet.

Thanks,
Andrew Pinski

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

Andrew Pinski <pinskia@gmail.com> writes:

> On Tue, Mar 22, 2011 at 9:42 AM, Ian Lance Taylor <iant@google.com> wrote:
>>
>> Any target on which _Unwind_Word is larger than void * is broken today,
>> so I don't think we need to care about that case.
>
> So a MIPS N32 is broken?  Lots of people use that target already and
> nothing like this has showed up yet.

That is a fair question.  It does seem to me that it must be broken in
some cases.  _Unwind_GetGRPtr will return &context->reg[index], which is
a void** cast to void*.  We will then pass that to _Unwind_SetGRPtr.  If
we later call _Unwind_SetGR on that register, it will write a value of
size _Unwind_Word through that pointer.  Similarly if we call
_Unwind_GetGR, it will read a value of size _Unwind_Word.  In both
cases, we will be accessing a 4-byte field as an 8-byte value.

If MIPS N32 works today, then something must be ensuring that that
sequence can never occur, or that for some reason it never matters.

Does anybody disagree with this analysis?

Ian

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ulrich Weigand]

Ian Lance Taylor wrote:
> Andrew Pinski <pinskia@gmail.com> writes:
> > On Tue, Mar 22, 2011 at 9:42 AM, Ian Lance Taylor <iant@google.com> wrote:
> >>
> >> Any target on which _Unwind_Word is larger than void * is broken today,
> >> so I don't think we need to care about that case.
> >
> > So a MIPS N32 is broken?  Lots of people use that target already and
> > nothing like this has showed up yet.
> 
> That is a fair question.  It does seem to me that it must be broken in
> some cases.  _Unwind_GetGRPtr will return &context->reg[index], which is
> a void** cast to void*.  We will then pass that to _Unwind_SetGRPtr.  If
> we later call _Unwind_SetGR on that register, it will write a value of
> size _Unwind_Word through that pointer.  Similarly if we call
> _Unwind_GetGR, it will read a value of size _Unwind_Word.  In both
> cases, we will be accessing a 4-byte field as an 8-byte value.
> 
> If MIPS N32 works today, then something must be ensuring that that
> sequence can never occur, or that for some reason it never matters.

Well, the whole problem only occurs when using the "by value" mechanism,
which is only triggered by DW_CFA_val_... CFI statements, which are new
in Dwarf-3 and seem to be very rarely used: they are apparently never
generated by GCC, and the only files I could find in current glibc that
create such CFI by hand are Intel-specific.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:

> Ian Lance Taylor wrote:
>> Andrew Pinski <pinskia@gmail.com> writes:
>> > On Tue, Mar 22, 2011 at 9:42 AM, Ian Lance Taylor <iant@google.com> wrote:
>> >>
>> >> Any target on which _Unwind_Word is larger than void * is broken today,
>> >> so I don't think we need to care about that case.
>> >
>> > So a MIPS N32 is broken?  Lots of people use that target already and
>> > nothing like this has showed up yet.
>> 
>> That is a fair question.  It does seem to me that it must be broken in
>> some cases.  _Unwind_GetGRPtr will return &context->reg[index], which is
>> a void** cast to void*.  We will then pass that to _Unwind_SetGRPtr.  If
>> we later call _Unwind_SetGR on that register, it will write a value of
>> size _Unwind_Word through that pointer.  Similarly if we call
>> _Unwind_GetGR, it will read a value of size _Unwind_Word.  In both
>> cases, we will be accessing a 4-byte field as an 8-byte value.
>> 
>> If MIPS N32 works today, then something must be ensuring that that
>> sequence can never occur, or that for some reason it never matters.
>
> Well, the whole problem only occurs when using the "by value" mechanism,
> which is only triggered by DW_CFA_val_... CFI statements, which are new
> in Dwarf-3 and seem to be very rarely used: they are apparently never
> generated by GCC, and the only files I could find in current glibc that
> create such CFI by hand are Intel-specific.

I see, thanks.

H.J., what is the failure mode that you are seeing?

This suggests that, at least, when sizeof(_Unwind_Word) > sizeof(void*),
we need to extend _Unwind_Context with a new array at the end, and use
that new array when by_value[REG] is true.

Ian

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

On Tue, Mar 22, 2011 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote:
> "Ulrich Weigand" <uweigand@de.ibm.com> writes:
>
>> Ian Lance Taylor wrote:
>>> Andrew Pinski <pinskia@gmail.com> writes:
>>> > On Tue, Mar 22, 2011 at 9:42 AM, Ian Lance Taylor <iant@google.com> wrote:
>>> >>
>>> >> Any target on which _Unwind_Word is larger than void * is broken today,
>>> >> so I don't think we need to care about that case.
>>> >
>>> > So a MIPS N32 is broken?  Lots of people use that target already and
>>> > nothing like this has showed up yet.
>>>
>>> That is a fair question.  It does seem to me that it must be broken in
>>> some cases.  _Unwind_GetGRPtr will return &context->reg[index], which is
>>> a void** cast to void*.  We will then pass that to _Unwind_SetGRPtr.  If
>>> we later call _Unwind_SetGR on that register, it will write a value of
>>> size _Unwind_Word through that pointer.  Similarly if we call
>>> _Unwind_GetGR, it will read a value of size _Unwind_Word.  In both
>>> cases, we will be accessing a 4-byte field as an 8-byte value.
>>>
>>> If MIPS N32 works today, then something must be ensuring that that
>>> sequence can never occur, or that for some reason it never matters.
>>
>> Well, the whole problem only occurs when using the "by value" mechanism,
>> which is only triggered by DW_CFA_val_... CFI statements, which are new
>> in Dwarf-3 and seem to be very rarely used: they are apparently never
>> generated by GCC, and the only files I could find in current glibc that
>> create such CFI by hand are Intel-specific.
>
> I see, thanks.
>
> H.J., what is the failure mode that you are seeing?

Yes, those come from hand written CFI statements for signal handler.

> This suggests that, at least, when sizeof(_Unwind_Word) > sizeof(void*),
> we need to extend _Unwind_Context with a new array at the end, and use
> that new array when by_value[REG] is true.

Will there be a case where 2 copies of unwind-dw2.c are within the same
process?

-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

"H.J. Lu" <hjl.tools@gmail.com> writes:

>> This suggests that, at least, when sizeof(_Unwind_Word) > sizeof(void*),
>> we need to extend _Unwind_Context with a new array at the end, and use
>> that new array when by_value[REG] is true.
>
> Will there be a case where 2 copies of unwind-dw2.c are within the same
> process?

Sure, that's easy.  I think what you are really asking is: will there be
a case where we could call functions from one copy and then pass the
context to the functions from a different copy.  I don't know the answer
to that, but it doesn't seem impossible.

Ian

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

On Tue, Mar 22, 2011 at 9:52 PM, Ian Lance Taylor <iant@google.com> wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>>> This suggests that, at least, when sizeof(_Unwind_Word) > sizeof(void*),
>>> we need to extend _Unwind_Context with a new array at the end, and use
>>> that new array when by_value[REG] is true.
>>
>> Will there be a case where 2 copies of unwind-dw2.c are within the same
>> process?
>
> Sure, that's easy.  I think what you are really asking is: will there be
> a case where we could call functions from one copy and then pass the
> context to the functions from a different copy.  I don't know the answer
> to that, but it doesn't seem impossible.
>

Then. will this be acceptable?

Thanks.

-- 
H.J.
---
#ifndef UNWIND_REGISTER_AS_UNION
#if defined __x86_64 && !defined __LP64__
# define UNWIND_REGISTER_AS_UNION
#endif
#endif

#ifdef UNWIND_REGISTER_AS_UNION
typedef union
{
  void *ref;
  _Unwind_Word val;
} _Unwind_Register;
# define UNWIND_REGISTER_GET_VAL_FROM_REF(TYPE, REG) \
   * (TYPE *) (REG).ref
# define UNWIND_REGISTER_SET_VAL(INDEX, VAL) \
   context->reg[INDEX].val = (VAL)
# define UNWIND_REGISTER_GET_REF(INDEX) \
   context->reg[INDEX].ref
# define UNWIND_REGISTER_GET_REF_FROM_VAL(INDEX) \
   &context->reg[INDEX].val
#else
typedef void *_Unwind_Register;
# define UNWIND_REGISTER_GET_VAL_FROM_REF(TYPE, REG) \
   * (TYPE *) (REG)
# define UNWIND_REGISTER_SET_VAL(INDEX, VAL) \
   context->reg[INDEX] = (void *) (_Unwind_Internal_Ptr) (VAL)
# define UNWIND_REGISTER_GET_REF(INDEX) \
   context->reg[INDEX]
# define UNWIND_REGISTER_GET_REF_FROM_VAL(INDEX) \
   &context->reg[INDEX]
#endif

/* This is the register and unwind state for a particular frame.  This
   provides the information necessary to unwind up past a frame and return
   to its caller.  */
struct _Unwind_Context
{
  _Unwind_Register reg[DWARF_FRAME_REGISTERS+1];
  void *cfa;

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ian Lance Taylor]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> On Tue, Mar 22, 2011 at 9:52 PM, Ian Lance Taylor <iant@google.com> wrote:
>> "H.J. Lu" <hjl.tools@gmail.com> writes:
>>
>>>> This suggests that, at least, when sizeof(_Unwind_Word) > sizeof(void*),
>>>> we need to extend _Unwind_Context with a new array at the end, and use
>>>> that new array when by_value[REG] is true.
>>>
>>> Will there be a case where 2 copies of unwind-dw2.c are within the same
>>> process?
>>
>> Sure, that's easy.  I think what you are really asking is: will there be
>> a case where we could call functions from one copy and then pass the
>> context to the functions from a different copy.  I don't know the answer
>> to that, but it doesn't seem impossible.
>>
>
> Then. will this be acceptable?

It's an interesting idea, but following this approach I think we clearly
want UNWIND_REGISTER_AS_UNION to be defined for any target for which
sizeof (_Unwind_Word) > sizeof (void *).  There is no reason to design a
system which will break for any such target.

Also, N32 is apparently broken right now if anybody adds some
hand-written CFI of the right sort, and this approach does not fix it.

Does anybody know of any existing target for which sizeof (_Unwind_Word)
> sizeof (void *) other than MIPS N32?

Do the MIPS maintainers have any opinion on whether we can theoretically
break the N32 exception handling ABI?  The only potential problem I've
been able to construct is a shared library linked against libgcc_eh.a
with -Bsymbolic which tries to unwind an exception through the
executable.  This is an unlikely scenario as we normally tell everybody
to link against libgcc_s.so.

Ian

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Richard Henderson]

On 03/22/2011 10:01 PM, H.J. Lu wrote:
>>> Will there be a case where 2 copies of unwind-dw2.c are within the same
>>> process?
>>
>> Sure, that's easy.  I think what you are really asking is: will there be
>> a case where we could call functions from one copy and then pass the
>> context to the functions from a different copy.  I don't know the answer
>> to that, but it doesn't seem impossible.

It really shouldn't be, but judging from the existance of _Unwind_IsExtendedContext
it appears that it really is.  Which, frankly, I assume is some sort of strange
packaging problem between glibc and libgcc_s.so.

Jakub, do you recall anything more specific about this?

2007-01-03  Jakub Jelinek  <jakub@redhat.com>

        * unwind-dw2.c (SIGNAL_FRAME_BIT, EXTENDED_CONTEXT_BIT): Define.
        (struct _Unwind_Context): Rename args_size to flags, remove
        signal_frame field, add a new args_size field and version field.
        (_Unwind_IsSignalFrame, _Unwind_SetSignalFrame,
        _Unwind_IsExtendedContext): New inline functions.
        (_Unwind_GetGR, _Unwind_SetGR, _Unwind_GetGRPtr, _Unwind_SetGRPtr):
        Assume by_value array is only present if _Unwind_IsExtendedContext.
        (_Unwind_GetIPInfo, execute_cfa_program, uw_frame_state_for): Use
        _Unwind_IsSignalFrame.
	...

> Then. will this be acceptable?

No.

Ideally we'd remove the by_value array, but I guess that's now versioned
so it'll have to stay even though unused.

Add a new

	_Unwind_Word data[DWARF_FRAME_REGISTERS+1];

at the end.  Increment the context version number.  If the version is
sufficiently high, implement _Unwind_SetGRValue via

	context->data[index] = val;
	context->reg[index] = &context->data[index];

and do not set by_value at all.


r~

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Jakub Jelinek]

On Wed, Mar 23, 2011 at 10:50:35AM -0700, Richard Henderson wrote:
> >>
> >> Sure, that's easy.  I think what you are really asking is: will there be
> >> a case where we could call functions from one copy and then pass the
> >> context to the functions from a different copy.  I don't know the answer
> >> to that, but it doesn't seem impossible.
> 
> It really shouldn't be, but judging from the existance of _Unwind_IsExtendedContext
> it appears that it really is.  Which, frankly, I assume is some sort of strange
> packaging problem between glibc and libgcc_s.so.
> 
> Jakub, do you recall anything more specific about this?

http://gcc.gnu.org/ml/gcc-patches/2006-12/msg01769.html
has some info.

> Ideally we'd remove the by_value array, but I guess that's now versioned
> so it'll have to stay even though unused.
> 
> Add a new
> 
> 	_Unwind_Word data[DWARF_FRAME_REGISTERS+1];
> 
> at the end.  Increment the context version number.  If the version is

For several targets that would grow the context significantly, do you think
we really have to do that for all targets rather than just for the
sizeof (_Unwind_Word) > sizeof (void *) ones?  The accessors are wrapped
in inline functions anyway.

> sufficiently high, implement _Unwind_SetGRValue via
> 
> 	context->data[index] = val;
> 	context->reg[index] = &context->data[index];
> 
> and do not set by_value at all.

	Jakub

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Richard Henderson]

On 03/23/2011 11:04 AM, Jakub Jelinek wrote:
> For several targets that would grow the context significantly, do you think
> we really have to do that for all targets rather than just for the
> sizeof (_Unwind_Word) > sizeof (void *) ones?  The accessors are wrapped
> in inline functions anyway.

Given that ia64 doesn't use this file, we're talking about 512 bytes for
most 64-bit targets.  I suppose that's 512 bytes we needn't allocate for
others, but...

The problem is that we suddenly have a version mismatch.  Suppose we 
conditionally add this, then next decade we need to add some other field
unconditionally.  Now determining what version X means is non-trivial.

What targets are you worried most about?


r~

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

On Wed, Mar 23, 2011 at 11:20 AM, Richard Henderson <rth@redhat.com> wrote:
> On 03/23/2011 11:04 AM, Jakub Jelinek wrote:
>> For several targets that would grow the context significantly, do you think
>> we really have to do that for all targets rather than just for the
>> sizeof (_Unwind_Word) > sizeof (void *) ones?  The accessors are wrapped
>> in inline functions anyway.
>
> Given that ia64 doesn't use this file, we're talking about 512 bytes for
> most 64-bit targets.  I suppose that's 512 bytes we needn't allocate for
> others, but...
>
> The problem is that we suddenly have a version mismatch.  Suppose we
> conditionally add this, then next decade we need to add some other field
> unconditionally.  Now determining what version X means is non-trivial.
>
> What targets are you worried most about?
>

FWIW, I ran into this on x32, which is totally new and doesn't have
backward compatibility issue.



-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Richard Henderson]

On 03/23/2011 11:24 AM, H.J. Lu wrote:
> FWIW, I ran into this on x32, which is totally new and doesn't have
> backward compatibility issue.

I don't care.  I'm not going to totally destroy maintainability for
the sake of one target.  There shall be a common solution.


r~

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Jakub Jelinek]

On Wed, Mar 23, 2011 at 11:20:24AM -0700, Richard Henderson wrote:
> On 03/23/2011 11:04 AM, Jakub Jelinek wrote:
> > For several targets that would grow the context significantly, do you think
> > we really have to do that for all targets rather than just for the
> > sizeof (_Unwind_Word) > sizeof (void *) ones?  The accessors are wrapped
> > in inline functions anyway.
> 
> Given that ia64 doesn't use this file, we're talking about 512 bytes for
> most 64-bit targets.  I suppose that's 512 bytes we needn't allocate for
> others, but...
> 
> The problem is that we suddenly have a version mismatch.  Suppose we 
> conditionally add this, then next decade we need to add some other field
> unconditionally.  Now determining what version X means is non-trivial.
> 
> What targets are you worried most about?

E.g. for ppc64 that's 1160 bytes, mmix 2104 bytes, ...
But if you think the 6 or so conditionals would be too much burden, just
ignore me.

	Jakub

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Richard Henderson]

On 03/23/2011 11:37 AM, Jakub Jelinek wrote:
> E.g. for ppc64 that's 1160 bytes, mmix 2104 bytes, ...
> But if you think the 6 or so conditionals would be too much burden, just
> ignore me.

I suppose we could interpret (a part of?) "version" as a bitmap of features,
rather than a serial number...

Guh.  The entire point of making _Unwind_Context opaque, and inventing libgcc_s,
was so that we didn't have to think about these things.  There's one and only one
copy of the unwind library and all uses of the structure are through accessors.
I hate hate hate that I got the export thing wrong at the very beginning.

Because, really, if we consider the structure truly public, we can't even
change the number of registers for a given port to support new features of
the cpu.  I don't suppose there's any way that we can declare these old
programs Just Broken, and drop this compatibility stuff?


r~

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Jakub Jelinek]

On Wed, Mar 23, 2011 at 12:01:37PM -0700, Richard Henderson wrote:
> Guh.  The entire point of making _Unwind_Context opaque, and inventing libgcc_s,
> was so that we didn't have to think about these things.  There's one and only one
> copy of the unwind library and all uses of the structure are through accessors.
> I hate hate hate that I got the export thing wrong at the very beginning.

I think the situation now is much better than it used to be when
libgcc_s/libgcc_eh was added, at that point there was no --as-needed support
and we wanted to avoid linking -lgcc_s into every program when it didn't
actually need it.  So currently that is mostly compatibility with very old
stuff or with people doing weird things (e.g. linking -lgcc_eh in), or
with people on less capable targets, or if we were to add new _Unwind_*
entrypoints (e.g. _Unwind_GETIPInfo was problematic, as older unwinder
implementations didn't have that symbol and thus it could be picked from
a different unwinder that had it).

	Jakub

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[Ulrich Weigand]

Richard Henderson wrote:
> Because, really, if we consider the structure truly public, we can't even
> change the number of registers for a given port to support new features of
> the cpu.

Indeed, and I remember we got bitten by that a long time ago, which is why
s390.h now has this comment:

/* Number of hardware registers that go into the DWARF-2 unwind info.
   To avoid ABI incompatibility, this number must not change even as
   'fake' hard registers are added or removed.  */
#define DWARF_FRAME_REGISTERS 34

> I don't suppose there's any way that we can declare these old
> programs Just Broken, and drop this compatibility stuff?

I wouldn't like that ... we did run into this problem in the wild, and
some s390 users really run very old programs for some reason.

However, I'm wondering: this bug that leaked the implementation of
_Unwind_Context only ever affected the *original* version of the
structure -- it was fixed before the extended context was ever
added, right?

If this is true, we'd still need to keep the original context format
unchanged, but we'd be free to modify the *extended* format at any
time, without ABI considerations and need for further versioning ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

On Wed, Mar 23, 2011 at 12:22 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Richard Henderson wrote:
>> Because, really, if we consider the structure truly public, we can't even
>> change the number of registers for a given port to support new features of
>> the cpu.
>
> Indeed, and I remember we got bitten by that a long time ago, which is why
> s390.h now has this comment:
>
> /* Number of hardware registers that go into the DWARF-2 unwind info.
>   To avoid ABI incompatibility, this number must not change even as
>   'fake' hard registers are added or removed.  */
> #define DWARF_FRAME_REGISTERS 34
>
>> I don't suppose there's any way that we can declare these old
>> programs Just Broken, and drop this compatibility stuff?
>
> I wouldn't like that ... we did run into this problem in the wild, and
> some s390 users really run very old programs for some reason.
>
> However, I'm wondering: this bug that leaked the implementation of
> _Unwind_Context only ever affected the *original* version of the
> structure -- it was fixed before the extended context was ever
> added, right?
>
> If this is true, we'd still need to keep the original context format
> unchanged, but we'd be free to modify the *extended* format at any
> time, without ABI considerations and need for further versioning ...
>

From what I can tell, the issues are:

1. _Unwind_Context is supposed to be opaque and we are free to
change it.  We should be able to extend DWARF_FRAME_REGISTERS
to support the new hard registers if needed, without breaking binary
compatibility.
2.  _Unwind_Context was leaked and wasn't really opaque.  To
provide backward binary compatibility, we are stuck with what we
had.

Is that possible to implement something along the line:

1. Add some bits to _Unwind_Context so that we can detect
the leaked _Unwind_Context.
2. When a leaked _Unwind_Context is detected at run-time,
as a compile time option, a target can either provide binary
compatibility or issue a run-time error.


-- 
H.J.

Re: PATCH: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

[H.J. Lu]

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

On Thu, Mar 24, 2011 at 12:15 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Mar 23, 2011 at 12:22 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> Richard Henderson wrote:
>>> Because, really, if we consider the structure truly public, we can't even
>>> change the number of registers for a given port to support new features of
>>> the cpu.
>>
>> Indeed, and I remember we got bitten by that a long time ago, which is why
>> s390.h now has this comment:
>>
>> /* Number of hardware registers that go into the DWARF-2 unwind info.
>>   To avoid ABI incompatibility, this number must not change even as
>>   'fake' hard registers are added or removed.  */
>> #define DWARF_FRAME_REGISTERS 34
>>
>>> I don't suppose there's any way that we can declare these old
>>> programs Just Broken, and drop this compatibility stuff?
>>
>> I wouldn't like that ... we did run into this problem in the wild, and
>> some s390 users really run very old programs for some reason.
>>
>> However, I'm wondering: this bug that leaked the implementation of
>> _Unwind_Context only ever affected the *original* version of the
>> structure -- it was fixed before the extended context was ever
>> added, right?
>>
>> If this is true, we'd still need to keep the original context format
>> unchanged, but we'd be free to modify the *extended* format at any
>> time, without ABI considerations and need for further versioning ...
>>
>
> From what I can tell, the issues are:
>
> 1. _Unwind_Context is supposed to be opaque and we are free to
> change it.  We should be able to extend DWARF_FRAME_REGISTERS
> to support the new hard registers if needed, without breaking binary
> compatibility.
> 2.  _Unwind_Context was leaked and wasn't really opaque.  To
> provide backward binary compatibility, we are stuck with what we
> had.
>
> Is that possible to implement something along the line:
>
> 1. Add some bits to _Unwind_Context so that we can detect
> the leaked _Unwind_Context.
> 2. When a leaked _Unwind_Context is detected at run-time,
> as a compile time option, a target can either provide binary
> compatibility or issue a run-time error.

This is the attempt to implement it.  Any comments?

Thanks.

-- 
H.J.
--
2011-04-09  H.J. Lu  <hongjiu.lu@intel.com>

	PR other/48007
	* unwind-dw2.c (UNIQUE_UNWIND_CONTEXT): New.
	(_Unwind_Context): If UNIQUE_UNWIND_CONTEXT is defined, add
	dwarf_reg_size_table and value, remove version and by_value.
	(EXTENDED_CONTEXT_BIT): Don't define if UNIQUE_UNWIND_CONTEXT
	is defined.
	(_Unwind_IsExtendedContext): Likewise.
	(_Unwind_GetGR): Support UNIQUE_UNWIND_CONTEXT.
	(_Unwind_SetGR): Likewise.
	(_Unwind_GetGRPtr): Likewise.
	(_Unwind_SetGRPtr): Likewise.
	(_Unwind_SetGRValue): Likewise.
	(_Unwind_GRByValue): Likewise.
	(__frame_state_for): Initialize dwarf_reg_size_table field if
	UNIQUE_UNWIND_CONTEXT is defined.
	(uw_install_context_1): Likewise.  Support UNIQUE_UNWIND_CONTEXT.

[-- Attachment #2: gcc-pr48007-3.patch --]
[-- Type: text/plain, Size: 6431 bytes --]

2011-04-09  H.J. Lu  <hongjiu.lu@intel.com>

	PR other/48007
	* unwind-dw2.c (UNIQUE_UNWIND_CONTEXT): New.
	(_Unwind_Context): If UNIQUE_UNWIND_CONTEXT is defined, add
	dwarf_reg_size_table and value, remove version and by_value.
	(EXTENDED_CONTEXT_BIT): Don't define if UNIQUE_UNWIND_CONTEXT
	is defined.
	(_Unwind_IsExtendedContext): Likewise.
	(_Unwind_GetGR): Support UNIQUE_UNWIND_CONTEXT.
	(_Unwind_SetGR): Likewise.
	(_Unwind_GetGRPtr): Likewise.
	(_Unwind_SetGRPtr): Likewise.
	(_Unwind_SetGRValue): Likewise.
	(_Unwind_GRByValue): Likewise.
	(__frame_state_for): Initialize dwarf_reg_size_table field if
	UNIQUE_UNWIND_CONTEXT is defined.
	(uw_install_context_1): Likewise.  Support UNIQUE_UNWIND_CONTEXT.

diff --git a/gcc/unwind-dw2.c b/gcc/unwind-dw2.c
index 25990b4..5fa2723 100644
--- a/gcc/unwind-dw2.c
+++ b/gcc/unwind-dw2.c
@@ -59,6 +59,12 @@
 #define DWARF_REG_TO_UNWIND_COLUMN(REGNO) (REGNO)
 #endif
 
+#ifndef UNIQUE_UNWIND_CONTEXT
+#if defined __x86_64 && !defined __LP64__
+# define UNIQUE_UNWIND_CONTEXT
+#endif
+#endif
+
 /* This is the register and unwind state for a particular frame.  This
    provides the information necessary to unwind up past a frame and return
    to its caller.  */
@@ -69,6 +75,15 @@ struct _Unwind_Context
   void *ra;
   void *lsda;
   struct dwarf_eh_bases bases;
+#ifdef UNIQUE_UNWIND_CONTEXT
+  /* Used to check for unique _Unwind_Context.  */
+  void *dwarf_reg_size_table;
+  /* Signal frame context.  */
+#define SIGNAL_FRAME_BIT ((_Unwind_Word) 1 >> 0)
+  _Unwind_Word flags;
+  _Unwind_Word args_size;
+  _Unwind_Word value[DWARF_FRAME_REGISTERS+1];
+#else
   /* Signal frame context.  */
 #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
   /* Context which has version/args_size/by_value fields.  */
@@ -79,6 +94,7 @@ struct _Unwind_Context
   _Unwind_Word version;
   _Unwind_Word args_size;
   char by_value[DWARF_FRAME_REGISTERS+1];
+#endif
 };
 
 /* Byte size of every register managed by these routines.  */
@@ -144,11 +160,13 @@ _Unwind_SetSignalFrame (struct _Unwind_Context *context, int val)
     context->flags &= ~SIGNAL_FRAME_BIT;
 }
 
+#ifndef UNIQUE_UNWIND_CONTEXT
 static inline _Unwind_Word
 _Unwind_IsExtendedContext (struct _Unwind_Context *context)
 {
   return context->flags & EXTENDED_CONTEXT_BIT;
 }
+#endif
 \f
 /* Get the value of register INDEX as saved in CONTEXT.  */
 
@@ -168,8 +186,14 @@ _Unwind_GetGR (struct _Unwind_Context *context, int index)
   size = dwarf_reg_size_table[index];
   ptr = context->reg[index];
 
+#ifdef UNIQUE_UNWIND_CONTEXT
+  gcc_assert (context->dwarf_reg_size_table == &dwarf_reg_size_table);
+  if (context->reg[index] == &context->value[index])
+    return context->value[index];
+#else
   if (_Unwind_IsExtendedContext (context) && context->by_value[index])
     return (_Unwind_Word) (_Unwind_Internal_Ptr) ptr;
+#endif
 
   /* This will segfault if the register hasn't been saved.  */
   if (size == sizeof(_Unwind_Ptr))
@@ -207,11 +231,20 @@ _Unwind_SetGR (struct _Unwind_Context *context, int index, _Unwind_Word val)
   gcc_assert (index < (int) sizeof(dwarf_reg_size_table));
   size = dwarf_reg_size_table[index];
 
+#ifdef UNIQUE_UNWIND_CONTEXT
+  gcc_assert (context->dwarf_reg_size_table == &dwarf_reg_size_table);
+  if (context->reg[index] == &context->value[index])
+    {
+      context->value[index] = val;
+      return;
+    }
+#else
   if (_Unwind_IsExtendedContext (context) && context->by_value[index])
     {
       context->reg[index] = (void *) (_Unwind_Internal_Ptr) val;
       return;
     }
+#endif
 
   ptr = context->reg[index];
 
@@ -230,8 +263,10 @@ static inline void *
 _Unwind_GetGRPtr (struct _Unwind_Context *context, int index)
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
+#ifndef UNIQUE_UNWIND_CONTEXT
   if (_Unwind_IsExtendedContext (context) && context->by_value[index])
     return &context->reg[index];
+#endif
   return context->reg[index];
 }
 
@@ -241,8 +276,10 @@ static inline void
 _Unwind_SetGRPtr (struct _Unwind_Context *context, int index, void *p)
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
+#ifndef UNIQUE_UNWIND_CONTEXT
   if (_Unwind_IsExtendedContext (context))
     context->by_value[index] = 0;
+#endif
   context->reg[index] = p;
 }
 
@@ -254,10 +291,15 @@ _Unwind_SetGRValue (struct _Unwind_Context *context, int index,
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
   gcc_assert (index < (int) sizeof(dwarf_reg_size_table));
+#ifdef UNIQUE_UNWIND_CONTEXT
+  gcc_assert (dwarf_reg_size_table[index] == sizeof (_Unwind_Word));
+  context->value[index] = val;
+  context->reg[index] = &context->value[index];
+#else
   gcc_assert (dwarf_reg_size_table[index] == sizeof (_Unwind_Ptr));
-
   context->by_value[index] = 1;
   context->reg[index] = (void *) (_Unwind_Internal_Ptr) val;
+#endif
 }
 
 /* Return nonzero if register INDEX is stored by value rather than
@@ -267,7 +309,11 @@ static inline int
 _Unwind_GRByValue (struct _Unwind_Context *context, int index)
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
+#ifdef UNIQUE_UNWIND_CONTEXT
+  return context->reg[index] == &context->value[index];
+#else
   return context->by_value[index];
+#endif
 }
 
 /* Retrieve the return address for CONTEXT.  */
@@ -1217,7 +1263,11 @@ __frame_state_for (void *pc_target, struct frame_state *state_in)
   int reg;
 
   memset (&context, 0, sizeof (struct _Unwind_Context));
+#ifdef UNIQUE_UNWIND_CONTEXT
+  context.dwarf_reg_size_table = &dwarf_reg_size_table;
+#else
   context.flags = EXTENDED_CONTEXT_BIT;
+#endif
   context.ra = pc_target + 1;
 
   if (uw_frame_state_for (&context, &fs) != _URC_NO_REASON)
@@ -1455,7 +1505,11 @@ uw_init_context_1 (struct _Unwind_Context *context,
 
   memset (context, 0, sizeof (struct _Unwind_Context));
   context->ra = ra;
+#ifdef UNIQUE_UNWIND_CONTEXT
+  context->dwarf_reg_size_table = &dwarf_reg_size_table;
+#else
   context->flags = EXTENDED_CONTEXT_BIT;
+#endif
 
   code = uw_frame_state_for (context, &fs);
   gcc_assert (code == _URC_NO_REASON);
@@ -1537,8 +1591,13 @@ uw_install_context_1 (struct _Unwind_Context *current,
       void *c = current->reg[i];
       void *t = target->reg[i];
 
+#ifdef UNIQUE_UNWIND_CONTEXT
+      gcc_assert (current->reg[i] != &current->value[i]);
+      if (target->reg[i] == &target->value[i] && c)
+#else
       gcc_assert (current->by_value[i] == 0);
       if (target->by_value[i] && c)
+#endif
 	{
 	  _Unwind_Word w;
 	  _Unwind_Ptr p;