diff mbox

PATCH [8/n]: Prepare x32: PR other/48007: Unwind library doesn't work with UNITS_PER_WORD > sizeof (void *)

Message ID BANLkTimw8O0UnnVATPKgrRCVT=82sO48Dw@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu June 28, 2011, 6:53 p.m. UTC 
On Mon, Jun 27, 2011 at 7:58 AM, Jason Merrill <jason@redhat.com> wrote:
> On 06/26/2011 05:58 PM, H.J. Lu wrote:
>>
>> The current unwind library scheme provides only one unwind
>> context and is backward compatible with multiple different unwind
>> contexts from multiple unwind libraries:
>>
>> http://gcc.gnu.org/ml/gcc-patches/2006-12/msg01769.html
>>
>> My patch fixes UNITS_PER_WORD > sizeof (void *) and
>> enforces single unwind context when backward compatibility
>> isn't needed.
>
> OK, there seem to be two things going on in this patch:
>
> 1) Handle registers larger than pointers.
> 2) Require that all code share a single copy of the unwinder.
>
> For #2, how are you avoiding the issues Jakub describes in that message?
>  Isn't his scenario 2 still possible?  Are you deciding that it's better to
> abort at run-time in that case?
>
> It seems to me that for targets newer than Jakub's patch we can hard-wire
> _Unwind_IsExtendedContext to true, but making further assumptions would be a
> mistake.
>
> Then, if we're still trying to handle versioning, I think your earlier patch
> for #1 (r170716) that just changes the type of the reg array is a better way
> to go.  But that change should be dependent on a target macro to avoid ABI
> changes for existing targets.
>

This updated patch.  It allows multiple unwind contexts.  It replaces

char by_value[DWARF_FRAME_REGISTERS+1];

with

_Unwind_Word value[DWARF_FRAME_REGISTERS+1];

The code is cleaner than conditionally replacing

void *reg[DWARF_FRAME_REGISTERS+1];

with

_Unwind_Word reg[DWARF_FRAME_REGISTERS+1];

with a bigger unwind context.  But it is more flexible if we
want to extend unwind context later, like saving/restoring
128bit or vector registers which may be bigger than the current
_Unwind_Word.

Thanks.

Comments

Jason Merrill June 30, 2011, 2:08 p.m. UTC | #1 
On 06/28/2011 02:53 PM, H.J. Lu wrote:
> This updated patch.  It allows multiple unwind contexts.  It replaces
>
> char by_value[DWARF_FRAME_REGISTERS+1];
>
> with
>
> _Unwind_Word value[DWARF_FRAME_REGISTERS+1];
>
> The code is cleaner than conditionally replacing
>
> void *reg[DWARF_FRAME_REGISTERS+1];
>
> with
>
> _Unwind_Word reg[DWARF_FRAME_REGISTERS+1];
>
> with a bigger unwind context.

It doesn't seem cleaner to me.

> But it is more flexible if we
> want to extend unwind context later, like saving/restoring
> 128bit or vector registers which may be bigger than the current
> _Unwind_Word.

I don't see that, either; with either approach, supporting larger 
registers would require a change to the the unwind context.

Jason
H.J. Lu June 30, 2011, 2:42 p.m. UTC | #2 
On Thu, Jun 30, 2011 at 7:08 AM, Jason Merrill <jason@redhat.com> wrote:
> On 06/28/2011 02:53 PM, H.J. Lu wrote:
>>
>> This updated patch.  It allows multiple unwind contexts.  It replaces
>>
>> char by_value[DWARF_FRAME_REGISTERS+1];
>>
>> with
>>
>> _Unwind_Word value[DWARF_FRAME_REGISTERS+1];
>>
>> The code is cleaner than conditionally replacing
>>
>> void *reg[DWARF_FRAME_REGISTERS+1];
>>
>> with
>>
>> _Unwind_Word reg[DWARF_FRAME_REGISTERS+1];
>>
>> with a bigger unwind context.
>
> It doesn't seem cleaner to me.

Register may be saved/restored either by address or value. My patch
doesn't change the reg field.  The other way will be

#ifdef USE_UNWIND_WORD
    _Unwind_Word reg[DWARF_FRAME_REGISTERS+1];
#else
     void *reg[DWARF_FRAME_REGISTERS+1];
#endif

We need it so that we are binary compatible with the existing
unwind context.  Once we do that we need many

#ifdef USE_UNWIND_WORD
#endif

whenever the reg field is accessed since the reg field is changed.
Jason Merrill June 30, 2011, 3:03 p.m. UTC | #3 
On 06/30/2011 10:42 AM, H.J. Lu wrote:
> Register may be saved/restored either by address or value. My patch
> doesn't change the reg field.  The other way will be
>
> #ifdef USE_UNWIND_WORD
>      _Unwind_Word reg[DWARF_FRAME_REGISTERS+1];
> #else
>       void *reg[DWARF_FRAME_REGISTERS+1];
> #endif
>
> We need it so that we are binary compatible with the existing
> unwind context.  Once we do that we need many
>
> #ifdef USE_UNWIND_WORD
> #endif
>
> whenever the reg field is accessed since the reg field is changed.

But your patch already changes all but one place where reg is accessed. 
  And we can avoid lots of ifdefs by abstraction with macros/inlines so 
there's one interface.

Also, why change SIGNAL_FRAME_BIT?

Jason
H.J. Lu June 30, 2011, 4:08 p.m. UTC | #4 
On Thu, Jun 30, 2011 at 8:03 AM, Jason Merrill <jason@redhat.com> wrote:
> On 06/30/2011 10:42 AM, H.J. Lu wrote:
>>
>> Register may be saved/restored either by address or value. My patch
>> doesn't change the reg field.  The other way will be
>>
>> #ifdef USE_UNWIND_WORD
>>     _Unwind_Word reg[DWARF_FRAME_REGISTERS+1];
>> #else
>>      void *reg[DWARF_FRAME_REGISTERS+1];
>> #endif
>>
>> We need it so that we are binary compatible with the existing
>> unwind context.  Once we do that we need many
>>
>> #ifdef USE_UNWIND_WORD
>> #endif
>>
>> whenever the reg field is accessed since the reg field is changed.
>
> But your patch already changes all but one place where reg is accessed.  And
> we can avoid lots of ifdefs by abstraction with macros/inlines so there's
> one interface.

I can do that.

> Also, why change SIGNAL_FRAME_BIT?
>

The current one is

  /* Signal frame context.  */
#define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)

It is defined such a strange way to be binary backward compatible.
Since there is no such a problem with if REG_VALUE_IN_UNWIND_CONTEXT
is defined, I simply define it as

  /* Signal frame context.  */
#define SIGNAL_FRAME_BIT ((_Unwind_Word) 1 >> 0)

Thanks.
diff mbox

Patch

diff --git a/gcc/config.gcc b/gcc/config.gcc
index a1dbd1a..c9867a2 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -2627,6 +2648,7 @@  esac
 case ${target} in
 i[34567]86-*-linux* | x86_64-*-linux*)
 	tmake_file="${tmake_file} i386/t-pmm_malloc i386/t-i386"
+	libgcc_tm_file="${libgcc_tm_file} i386/value-unwind.h"
 	;;
 i[34567]86-*-* | x86_64-*-*)
 	tmake_file="${tmake_file} i386/t-gmm_malloc i386/t-i386"
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 341628b..2666716 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -3701,6 +3701,14 @@  return @code{@var{regno}}.
 
 @end defmac
 
+@defmac REG_VALUE_IN_UNWIND_CONTEXT
+
+Define this macro if the target stores register values as
+@code{_Unwind_Word} type in unwind context.  The default is to
+store register values as @code{void *} type.
+
+@end defmac
+
 @node Elimination
 @subsection Eliminating Frame Pointer and Arg Pointer
 
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index f7c16e9..690fa52 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -3687,6 +3687,14 @@  return @code{@var{regno}}.
 
 @end defmac
 
+@defmac REG_VALUE_IN_UNWIND_CONTEXT
+
+Define this macro if the target stores register values as
+@code{_Unwind_Word} type in unwind context.  The default is to
+store register values as @code{void *} type.
+
+@end defmac
+
 @node Elimination
 @subsection Eliminating Frame Pointer and Arg Pointer
 
diff --git a/gcc/system.h b/gcc/system.h
index e02cbcd..ed39d93 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -764,7 +764,7 @@  extern void fancy_abort (const char *, int, const char *) ATTRIBUTE_NORETURN;
 /* Target macros only used for code built for the target, that have
    moved to libgcc-tm.h or have never been present elsewhere.  */
  #pragma GCC poison DECLARE_LIBRARY_RENAMES LIBGCC2_GNU_PREFIX		\
-	MD_UNWIND_SUPPORT ENABLE_EXECUTE_STACK
+	MD_UNWIND_SUPPORT ENABLE_EXECUTE_STACK REG_VALUE_IN_UNWIND_CONTEXT
 
 /* Other obsolete target macros, or macros that used to be in target
    headers and were not used, and may be obsolete or may never have
diff --git a/gcc/unwind-dw2.c b/gcc/unwind-dw2.c
index 19da299..8014a1b 100644
--- a/gcc/unwind-dw2.c
+++ b/gcc/unwind-dw2.c
@@ -69,16 +69,25 @@  struct _Unwind_Context
   void *ra;
   void *lsda;
   struct dwarf_eh_bases bases;
+#ifdef REG_VALUE_IN_UNWIND_CONTEXT
+  /* Signal frame context.  */
+#define SIGNAL_FRAME_BIT ((_Unwind_Word) 1 >> 0)
+#else
   /* Signal frame context.  */
 #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
   /* Context which has version/args_size/by_value fields.  */
 #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
+#endif
   _Unwind_Word flags;
   /* 0 for now, can be increased when further fields are added to
      struct _Unwind_Context.  */
   _Unwind_Word version;
   _Unwind_Word args_size;
+#ifdef REG_VALUE_IN_UNWIND_CONTEXT
+  _Unwind_Word value[DWARF_FRAME_REGISTERS+1];
+#else
   char by_value[DWARF_FRAME_REGISTERS+1];
+#endif
 };
 
 /* Byte size of every register managed by these routines.  */
@@ -144,11 +153,13 @@  _Unwind_SetSignalFrame (struct _Unwind_Context *context, int val)
     context->flags &= ~SIGNAL_FRAME_BIT;
 }
 
+#ifndef REG_VALUE_IN_UNWIND_CONTEXT
 static inline _Unwind_Word
 _Unwind_IsExtendedContext (struct _Unwind_Context *context)
 {
   return context->flags & EXTENDED_CONTEXT_BIT;
 }
+#endif
 
 /* Get the value of register INDEX as saved in CONTEXT.  */
 
@@ -168,8 +179,13 @@  _Unwind_GetGR (struct _Unwind_Context *context, int index)
   size = dwarf_reg_size_table[index];
   ptr = context->reg[index];
 
+#ifdef REG_VALUE_IN_UNWIND_CONTEXT
+  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 +223,19 @@  _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 REG_VALUE_IN_UNWIND_CONTEXT
+  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 +254,10 @@  static inline void *
 _Unwind_GetGRPtr (struct _Unwind_Context *context, int index)
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
+#ifndef REG_VALUE_IN_UNWIND_CONTEXT
   if (_Unwind_IsExtendedContext (context) && context->by_value[index])
     return &context->reg[index];
+#endif
   return context->reg[index];
 }
 
@@ -241,8 +267,10 @@  static inline void
 _Unwind_SetGRPtr (struct _Unwind_Context *context, int index, void *p)
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
+#ifndef REG_VALUE_IN_UNWIND_CONTEXT
   if (_Unwind_IsExtendedContext (context))
     context->by_value[index] = 0;
+#endif
   context->reg[index] = p;
 }
 
@@ -254,10 +282,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 REG_VALUE_IN_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 +300,11 @@  static inline int
 _Unwind_GRByValue (struct _Unwind_Context *context, int index)
 {
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
+#ifdef REG_VALUE_IN_UNWIND_CONTEXT
+  return context->reg[index] == &context->value[index];
+#else
   return context->by_value[index];
+#endif
 }
 
 /* Retrieve the return address for CONTEXT.  */
@@ -1215,7 +1252,9 @@  __frame_state_for (void *pc_target, struct frame_state *state_in)
   int reg;
 
   memset (&context, 0, sizeof (struct _Unwind_Context));
+#ifndef REG_VALUE_IN_UNWIND_CONTEXT
   context.flags = EXTENDED_CONTEXT_BIT;
+#endif
   context.ra = pc_target + 1;
 
   if (uw_frame_state_for (&context, &fs) != _URC_NO_REASON)
@@ -1453,7 +1492,9 @@  uw_init_context_1 (struct _Unwind_Context *context,
 
   memset (context, 0, sizeof (struct _Unwind_Context));
   context->ra = ra;
+#ifndef REG_VALUE_IN_UNWIND_CONTEXT
   context->flags = EXTENDED_CONTEXT_BIT;
+#endif
 
   code = uw_frame_state_for (context, &fs);
   gcc_assert (code == _URC_NO_REASON);
@@ -1535,8 +1576,13 @@  uw_install_context_1 (struct _Unwind_Context *current,
       void *c = current->reg[i];
       void *t = target->reg[i];
 
+#ifdef REG_VALUE_IN_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;
diff --git a/libgcc/config/i386/value-unwind.h b/libgcc/config/i386/value-unwind.h
new file mode 100644
index 0000000..0dceb5c
--- /dev/null
+++ b/libgcc/config/i386/value-unwind.h
@@ -0,0 +1,26 @@ 
+/* Store register values as _Unwind_Word type in DWARF2 EH unwind context.
+   Copyright (C) 2011
+   Free Software Foundation, Inc.
+
+   This file is part of GCC.
+
+   GCC is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published
+   by the Free Software Foundation; either version 3, or (at your
+   option) any later version.
+
+   GCC is distributed in the hope that it will be useful, but WITHOUT
+   ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+   or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
+   License for more details.
+
+   You should have received a copy of the GNU General Public License and
+   a copy of the GCC Runtime Library Exception along with this program;
+   see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Define this macro if the target stores register values as _Unwind_Word
+   type in unwind context.  Only enable it for x32.  */
+#if defined __x86_64 && !defined __LP64__
+# define REG_VALUE_IN_UNWIND_CONTEXT
+#endif