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

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. 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

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

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

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

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