Re: [ARM/FDPIC v3 06/21] [ARM] FDPIC: Add support for c++ exceptions

[Christophe Lyon <christophe.lyon@linaro.org>]

On Fri, 12 Oct 2018 at 13:37, Richard Earnshaw (lists) <
Richard.Earnshaw@arm.com> wrote:

> On 11/10/18 14:34, Christophe Lyon wrote:
> > The main difference with existing support is that function addresses
> > are function descriptor addresses instead. This means that all code
> > dealing with function pointers now has to cope with function
> > descriptors instead.
> >
> > For the same reason, Linux kernel helpers can no longer be called by
> > dereferencing their address, so we implement the same functionality as
> > a regular function here.
> >
> > When restoring a function address, we also have to restore the FDPIC
> > register value (r9).
> >
> > 2018-XX-XX  Christophe Lyon  <christophe.lyon@st.com>
> >       Mickaël Guêné <mickael.guene@st.com>
> >
> >       gcc/
> >       * ginclude/unwind-arm-common.h (unwinder_cache): Add reserved5
> >       field.
> >       (FDPIC_REGNUM): New define.
> >
> >       libgcc/
> >       * config/arm/linux-atomic.c (__kernel_cmpxchg): Add FDPIC support.
> >       (__kernel_dmb): Likewise.
> >       (__fdpic_cmpxchg): New function.
> >       (__fdpic_dmb): New function.
> >       * config/arm/unwind-arm.h (gnu_Unwind_Find_got): New function.
> >       (_Unwind_decode_typeinfo_ptr): Add FDPIC support.
> >       * unwindo-arm-common.inc (UCB_PR_GOT): New.
> >       (funcdesc_t): New struct.
> >       (get_eit_entry): Add FDPIC support.
> >       (unwind_phase2): Likewise.
> >       (unwind_phase2_forced): Likewise.
> >       (__gnu_Unwind_RaiseException): Likewise.
> >       (__gnu_Unwind_Resume): Likewise.
> >       (__gnu_Unwind_Backtrace): Likewise.
> >       * unwind-pe.h (read_encoded_value_with_base): Likewise.
> >
> >       libstdc++/
> >       * libsupc++/eh_personality.cc (get_ttype_entry): Add FDPIC
> >       support.
> >
> > Change-Id: I517a49ff18fae21c686cd1c6008ea7974515b347
> >
> > diff --git a/gcc/ginclude/unwind-arm-common.h
> b/gcc/ginclude/unwind-arm-common.h
> > index 8a1a919..f663891 100644
> > --- a/gcc/ginclude/unwind-arm-common.h
> > +++ b/gcc/ginclude/unwind-arm-common.h
> > @@ -91,7 +91,7 @@ extern "C" {
> >         _uw reserved2;  /* Personality routine address */
> >         _uw reserved3;  /* Saved callsite address */
> >         _uw reserved4;  /* Forced unwind stop arg */
> > -       _uw reserved5;
> > +       _uw reserved5;  /* Personality routine GOT value in FDPIC mode.
> */
> >       }
> >        unwinder_cache;
> >        /* Propagation barrier cache (valid after phase 1): */
> > @@ -247,4 +247,6 @@ typedef unsigned long _uleb128_t;
> >  }   /* extern "C" */
> >  #endif
> >
> > +#define FDPIC_REGNUM 9
>
> Looking at the rest of this file, I think it can end up being included
> in user code.  So you have to put predefines into the reserved
> namespace.  Why do you need this here anyway?
>
> That was to address a comment I got on v2 of this patch: I was requested
to avoid hardcoding "r9" in unwind-arm.h, and to use FDPIC_REGNUM.
I needed the definition to be visible in both unwind-arm.h and
unwind-arm-common.inc, but I moved it at a too high level. I'll fix this.

> +
> >  #endif /* defined UNWIND_ARM_COMMON_H */
> > diff --git a/libgcc/config/arm/linux-atomic.c
> b/libgcc/config/arm/linux-atomic.c
> > index d334c58..161d1ce 100644
> > --- a/libgcc/config/arm/linux-atomic.c
> > +++ b/libgcc/config/arm/linux-atomic.c
> > @@ -25,11 +25,49 @@ see the files COPYING3 and COPYING.RUNTIME
> respectively.  If not, see
> >
> >  /* Kernel helper for compare-and-exchange.  */
> >  typedef int (__kernel_cmpxchg_t) (int oldval, int newval, int *ptr);
> > +#if __FDPIC__
> > +/* Non-FDPIC ABIs call __kernel_cmpxchg directly by dereferencing its
> > +   address, but under FDPIC we would generate a broken call
> > +   sequence. That's why we have to implement __kernel_cmpxchg and
> > +   __kernel_dmb here: this way, the FDPIC call sequence works.  */
> > +#define __kernel_cmpxchg __fdpic_cmpxchg
> > +#else
> >  #define __kernel_cmpxchg (*(__kernel_cmpxchg_t *) 0xffff0fc0)
> > +#endif
> >
> >  /* Kernel helper for memory barrier.  */
> >  typedef void (__kernel_dmb_t) (void);
> > +#if __FDPIC__
> > +#define __kernel_dmb __fdpic_dmb
> > +#else
> >  #define __kernel_dmb (*(__kernel_dmb_t *) 0xffff0fa0)
> > +#endif
> > +
> > +#if __FDPIC__
> > +static int __fdpic_cmpxchg (int oldval, int newval, int *ptr)
> > +{
> > +  int result;
> > +
> > +  asm volatile ("1: ldrex r3, [%[ptr]]\n\t"
> > +             "subs  r3, r3, %[oldval]\n\t"
> > +             "itt eq\n\t"
> > +             "strexeq r3, %[newval], [%[ptr]]\n\t"
> > +             "teqeq r3, #1\n\t"
> > +             "it eq\n\t"
> > +             "beq 1b\n\t"
> > +             "rsbs  %[result], r3, #0\n\t"
> > +             : [result] "=r" (result)
> > +             : [oldval] "r" (oldval) , [newval] "r" (newval), [ptr] "r"
> (ptr)
> > +             : "r3");
> > +    return result;
> > +}
> > +
> > +static void __fdpic_dmb ()
> > +{
> > +  asm volatile ("dmb\n\t");
> > +}
> > +
>
> The whole point of __kernel_dmb and __kernel_cmpxchg is that the kernel
> can map in a code sequence specific to the runtime CPU.  This ensures
> that the right sequence is used for that CPU.  This breaks that
> principle entirely.
>
> Right, I guess we had v7-only in mind.
I'll check how we can fix that.


> > +#endif
> >
> >  /* Note: we implement byte, short and int versions of atomic operations
> using
> >     the above kernel helpers; see linux-atomic-64bit.c for "long long"
> (64-bit)
> > diff --git a/libgcc/config/arm/unwind-arm.h
> b/libgcc/config/arm/unwind-arm.h
> > index 9f7d3f2..db5dfa2 100644
> > --- a/libgcc/config/arm/unwind-arm.h
> > +++ b/libgcc/config/arm/unwind-arm.h
> > @@ -33,9 +33,31 @@
> >  /* Use IP as a scratch register within the personality routine.  */
> >  #define UNWIND_POINTER_REG 12
> >
> > +#define STR(x) #x
> > +#define XSTR(x) STR(x)
> > +
> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> > +_Unwind_Ptr __attribute__((weak)) __gnu_Unwind_Find_got (_Unwind_Ptr);
> > +
> > +static inline _Unwind_Ptr gnu_Unwind_Find_got (_Unwind_Ptr ptr)
> > +{
> > +    _Unwind_Ptr res;
> > +
> > +    if (__gnu_Unwind_Find_got)
> > +     res =  __gnu_Unwind_Find_got (ptr);
> > +    else
> > +      {
> > +     asm volatile ("mov %[result], r" XSTR(FDPIC_REGNUM)
> > +                   : [result]"=r" (res)
> > +                   :
> > +                   :);
> > +      }
> > +
> > +    return res;
> > +}
> > +
> >    /* Decode an R_ARM_TARGET2 relocation.  */
> >    static inline _Unwind_Word
> >    _Unwind_decode_typeinfo_ptr (_Unwind_Word base __attribute__
> ((unused)),
> > @@ -48,7 +70,12 @@ extern "C" {
> >        if (!tmp)
> >       return 0;
> >
> > -#if (defined(linux) && !defined(__uClinux__)) || defined(__NetBSD__) \
> > +#if __FDPIC__
> > +      /* For FDPIC, we store the offset of the GOT entry.  */
> > +      /* So, first get GOT from dynamic linker and then use indirect
> access.  */
> > +      tmp += gnu_Unwind_Find_got (ptr);
> > +      tmp = *(_Unwind_Word *) tmp;
> > +#elif (defined(linux) && !defined(__uClinux__)) || defined(__NetBSD__) \
> >      || defined(__FreeBSD__) || defined(__fuchsia__)
> >        /* Pc-relative indirect.  */
> >  #define _GLIBCXX_OVERRIDE_TTYPE_ENCODING (DW_EH_PE_pcrel |
> DW_EH_PE_indirect)
> > diff --git a/libgcc/unwind-arm-common.inc b/libgcc/unwind-arm-common.inc
> > index 76f8fc3..7187edd 100644
> > --- a/libgcc/unwind-arm-common.inc
> > +++ b/libgcc/unwind-arm-common.inc
> > @@ -62,6 +62,7 @@ __gnu_Unwind_Find_exidx (_Unwind_Ptr, int *);
> >  #define UCB_PR_ADDR(ucbp) ((ucbp)->unwinder_cache.reserved2)
> >  #define UCB_SAVED_CALLSITE_ADDR(ucbp) ((ucbp)->unwinder_cache.reserved3)
> >  #define UCB_FORCED_STOP_ARG(ucbp) ((ucbp)->unwinder_cache.reserved4)
> > +#define UCB_PR_GOT(ucbp) ((ucbp)->unwinder_cache.reserved5)
> >
> >  /* Unwind descriptors.  */
> >
> > @@ -85,6 +86,15 @@ typedef struct __EIT_entry
> >    _uw content;
> >  } __EIT_entry;
> >
> > +#ifdef __FDPIC__
> > +
> > +/* Only used in FDPIC case.  */
> > +struct funcdesc_t {
> > +    unsigned int ptr;
> > +    unsigned int got;
> > +};
>
> This isn't in GNU style.
>
> OK


> > +#endif
> > +
> >  /* Assembly helper functions.  */
> >
> >  /* Restore core register state.  Never returns.  */
> > @@ -259,7 +269,21 @@ get_eit_entry (_Unwind_Control_Block *ucbp, _uw
> return_address)
> >      {
> >        /* One of the predefined standard routines.  */
> >        _uw idx = (*(_uw *) ucbp->pr_cache.ehtp >> 24) & 0xf;
> > +#if __FDPIC__
> > +      {
> > +     struct funcdesc_t *funcdesc
> > +       = (struct funcdesc_t *) __gnu_unwind_get_pr_addr (idx);
> > +     if (funcdesc)
> > +       {
> > +         UCB_PR_ADDR (ucbp) = funcdesc->ptr;
> > +         UCB_PR_GOT (ucbp) = funcdesc->got;
> > +       }
> > +     else
> > +       UCB_PR_ADDR (ucbp) = 0;
> > +      }
> > +#else
> >        UCB_PR_ADDR (ucbp) = __gnu_unwind_get_pr_addr (idx);
> > +#endif
> >        if (UCB_PR_ADDR (ucbp) == 0)
> >       {
> >         /* Failed */
> > @@ -270,6 +294,10 @@ get_eit_entry (_Unwind_Control_Block *ucbp, _uw
> return_address)
> >      {
> >        /* Execute region offset to PR */
> >        UCB_PR_ADDR (ucbp) = selfrel_offset31 (ucbp->pr_cache.ehtp);
> > +#if __FDPIC__
> > +      UCB_PR_GOT (ucbp)
> > +     = (unsigned int) gnu_Unwind_Find_got ((_Unwind_Ptr) UCB_PR_ADDR
> (ucbp));
> > +#endif
> >      }
> >    return _URC_OK;
> >  }
> > @@ -291,14 +319,29 @@ unwind_phase2 (_Unwind_Control_Block * ucbp,
> phase2_vrs * vrs)
> >        UCB_SAVED_CALLSITE_ADDR (ucbp) = VRS_PC(vrs);
> >
> >        /* Call the pr to decide what to do.  */
> > +#if __FDPIC__
> > +      {
> > +     volatile struct funcdesc_t funcdesc;
> > +     funcdesc.ptr = UCB_PR_ADDR (ucbp);
> > +     funcdesc.got = UCB_PR_GOT (ucbp);
> > +     pr_result = ((personality_routine) &funcdesc)
> > +       (_US_UNWIND_FRAME_STARTING, ucbp, (_Unwind_Context *) vrs);
> > +      }
> > +#else
> >        pr_result = ((personality_routine) UCB_PR_ADDR (ucbp))
> >       (_US_UNWIND_FRAME_STARTING, ucbp, (_Unwind_Context *) vrs);
> > +#endif
> >      }
> >    while (pr_result == _URC_CONTINUE_UNWIND);
> >
> >    if (pr_result != _URC_INSTALL_CONTEXT)
> >      abort();
> >
> > +#if __FDPIC__
> > +  /* r9 could have been lost due to PLT jump.  Restore correct value.
> */
> > +  vrs->core.r[FDPIC_REGNUM] = gnu_Unwind_Find_got (VRS_PC (vrs));
> > +#endif
> > +
> >    uw_restore_core_regs (vrs, &vrs->core);
> >  }
> >
> > @@ -346,8 +389,18 @@ unwind_phase2_forced (_Unwind_Control_Block *ucbp,
> phase2_vrs *entry_vrs,
> >         next_vrs = saved_vrs;
> >
> >         /* Call the pr to decide what to do.  */
> > +#if __FDPIC__
> > +       {
> > +         volatile struct funcdesc_t funcdesc;
> > +         funcdesc.ptr = UCB_PR_ADDR (ucbp);
> > +         funcdesc.got = UCB_PR_GOT (ucbp);
> > +         pr_result = ((personality_routine) &funcdesc)
> > +           (action, ucbp, (void *) &next_vrs);
> > +       }
> > +#else
> >         pr_result = ((personality_routine) UCB_PR_ADDR (ucbp))
> >           (action, ucbp, (void *) &next_vrs);
> > +#endif
> >
> >         saved_vrs.prev_sp = VRS_SP (&next_vrs);
> >       }
> > @@ -384,6 +437,11 @@ unwind_phase2_forced (_Unwind_Control_Block *ucbp,
> phase2_vrs *entry_vrs,
> >        return _URC_FAILURE;
> >      }
> >
> > +#if __FDPIC__
> > +  /* r9 could have been lost due to PLT jump.  Restore correct value.
> */
> > +  saved_vrs.core.r[FDPIC_REGNUM] = gnu_Unwind_Find_got (VRS_PC
> (&saved_vrs));
> > +#endif
> > +
> >    uw_restore_core_regs (&saved_vrs, &saved_vrs.core);
> >  }
> >
> > @@ -429,8 +487,18 @@ __gnu_Unwind_RaiseException (_Unwind_Control_Block
> * ucbp,
> >       return _URC_FAILURE;
> >
> >        /* Call the pr to decide what to do.  */
> > +#if __FDPIC__
> > +      {
> > +     volatile struct funcdesc_t funcdesc;
> > +     funcdesc.ptr = UCB_PR_ADDR (ucbp);
> > +     funcdesc.got = UCB_PR_GOT (ucbp);
> > +     pr_result = ((personality_routine) &funcdesc)
> > +       (_US_VIRTUAL_UNWIND_FRAME, ucbp, (void *) &saved_vrs);
> > +      }
> > +#else
> >        pr_result = ((personality_routine) UCB_PR_ADDR (ucbp))
> >       (_US_VIRTUAL_UNWIND_FRAME, ucbp, (void *) &saved_vrs);
> > +#endif
> >      }
> >    while (pr_result == _URC_CONTINUE_UNWIND);
> >
> > @@ -488,13 +556,27 @@ __gnu_Unwind_Resume (_Unwind_Control_Block * ucbp,
> phase2_vrs * entry_vrs)
> >      }
> >
> >    /* Call the cached PR.  */
> > +#if __FDPIC__
> > +  {
> > +    volatile struct funcdesc_t funcdesc;
> > +    funcdesc.ptr = UCB_PR_ADDR (ucbp);
> > +    funcdesc.got = UCB_PR_GOT (ucbp);
> > +    pr_result = ((personality_routine) &funcdesc)
> > +      (_US_UNWIND_FRAME_RESUME, ucbp, (_Unwind_Context *) entry_vrs);
> > +  }
> > +#else
> >    pr_result = ((personality_routine) UCB_PR_ADDR (ucbp))
> >       (_US_UNWIND_FRAME_RESUME, ucbp, (_Unwind_Context *) entry_vrs);
> > +#endif
> >
> >    switch (pr_result)
> >      {
> >      case _URC_INSTALL_CONTEXT:
> >        /* Upload the registers to enter the landing pad.  */
> > +#if __FDPIC__
> > +      /* r9 could have been lost due to PLT jump.  Restore correct
> value.  */
> > +      entry_vrs->core.r[FDPIC_REGNUM] = gnu_Unwind_Find_got (VRS_PC
> (entry_vrs));
> > +#endif
> >        uw_restore_core_regs (entry_vrs, &entry_vrs->core);
> >
> >      case _URC_CONTINUE_UNWIND:
> > @@ -586,9 +668,20 @@ __gnu_Unwind_Backtrace(_Unwind_Trace_Fn trace, void
> * trace_argument,
> >       }
> >
> >        /* Call the pr to decide what to do.  */
> > +#if __FDPIC__
> > +      {
> > +     volatile struct funcdesc_t funcdesc;
> > +     funcdesc.ptr = UCB_PR_ADDR (ucbp);
> > +     funcdesc.got = UCB_PR_GOT (ucbp);
> > +     code = ((personality_routine) &funcdesc)
> > +       (_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND,
> > +        ucbp, (void *) &saved_vrs);
> > +      }
> > +#else
> >        code = ((personality_routine) UCB_PR_ADDR (ucbp))
> >       (_US_VIRTUAL_UNWIND_FRAME | _US_FORCE_UNWIND,
> >        ucbp, (void *) &saved_vrs);
> > +#endif
> >      }
> >    while (code != _URC_END_OF_STACK
> >        && code != _URC_FAILURE);
> > diff --git a/libgcc/unwind-pe.h b/libgcc/unwind-pe.h
> > index dd5ae95..9c5177f 100644
> > --- a/libgcc/unwind-pe.h
> > +++ b/libgcc/unwind-pe.h
> > @@ -259,10 +259,27 @@ read_encoded_value_with_base (unsigned char
> encoding, _Unwind_Ptr base,
> >
> >        if (result != 0)
> >       {
> > +#if __FDPIC__
> > +       /* FDPIC relative addresses imply taking the GOT address
> > +          into account.  */
> > +       if ((encoding & DW_EH_PE_pcrel) && (encoding &
> DW_EH_PE_indirect))
> > +         {
> > +           result += gnu_Unwind_Find_got ((_Unwind_Ptr) u);
> > +           result = *(_Unwind_Internal_Ptr *) result;
> > +         }
> > +       else
> > +         {
> > +           result += ((encoding & 0x70) == DW_EH_PE_pcrel
> > +                      ? (_Unwind_Internal_Ptr) u : base);
> > +           if (encoding & DW_EH_PE_indirect)
> > +             result = *(_Unwind_Internal_Ptr *) result;
> > +         }
> > +#else
> >         result += ((encoding & 0x70) == DW_EH_PE_pcrel
> >                    ? (_Unwind_Internal_Ptr) u : base);
> >         if (encoding & DW_EH_PE_indirect)
> >           result = *(_Unwind_Internal_Ptr *) result;
> > +#endif
> >       }
> >      }
> >
> > diff --git a/libstdc++-v3/libsupc++/eh_personality.cc
> b/libstdc++-v3/libsupc++/eh_personality.cc
> > index 1b336c7..7b26b8d 100644
> > --- a/libstdc++-v3/libsupc++/eh_personality.cc
> > +++ b/libstdc++-v3/libsupc++/eh_personality.cc
> > @@ -93,7 +93,15 @@ get_ttype_entry (lsda_header_info *info, _uleb128_t i)
> >    _Unwind_Ptr ptr;
> >
> >    i *= size_of_encoded_value (info->ttype_encoding);
> > -  read_encoded_value_with_base (info->ttype_encoding, info->ttype_base,
> > +  read_encoded_value_with_base (
> > +#if __FDPIC__
> > +                             /* Force these flags to nake sure to
> > +                                take the GOT into account.  */
> > +                             (DW_EH_PE_pcrel | DW_EH_PE_indirect),
> > +#else
> > +                             info->ttype_encoding,
> > +#endif
> > +                             info->ttype_base,
> >                               info->TType - i, &ptr);
> >
> >    return reinterpret_cast<const std::type_info *>(ptr);
> >
>
> Nearly all of the above is outside of my comfort zone...  I can't help
> feeling that there ought to be a less invasive way to do this, but
> without being particularly familiar with the code it's difficult to be
> sure.
>
> The last two files patched will need approval from a relevant component
> maintainer.
>