[libcxxabi patch] Add Registers_arm class and unw_getcontext implementation

Nico Weber thakis at chromium.org
Sat Dec 14 12:25:25 PST 2013


Hi Nick,

I just realized we didn't cc you on our replies here:
Albert:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131202/095019.html
Me:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131209/095089.html

Sorry!

Adding Albert to this branch of the thread, to hopefully bring us all on
the same page again. I'm also attaching a new version of the patch, with
the following changes:

* Add an assembly.h file, use that to hide the leading '_' difference in
the two .S files
* Change the arm 32bit register enum in libunwind.h
* Change the float register stuff in Registers_arm

On Fri, Dec 6, 2013 at 11:37 AM, Nick Kledzik <kledzik at apple.com> wrote:

>
> On Dec 5, 2013, at 10:32 PM, Nico Weber <thakis at chromium.org> wrote:
>
> > Hi,
> >
> > the attached patch adds Registers_arm and assembly for unw_getcontext.
> VFP support is still missing. The class isn't used by anything yet.
>
>
> > +// 32-bit ARM64 registers
> > +enum {
> > +  UNW_ARM_R0  = 0,
> > +  UNW_ARM_R1  = 1,
> > ...
> > +  UNW_ARM_D0  = 64,
> > +  UNW_ARM_D1  = 65,
> This register numbering needs some comments.  I’m not even sure what the
> right numbering should be.  The numbers do need to match what the compiler
> thinks the numbering is (e.g. the dwarf register numbering).  I’ve heard
> that for arm dwarf, 64+ is legacy for single precision registers, and that
> when they were widened to double, new register numbers (256+) were used.
>

Albert: """Hmm...section 3.1 of the DWARF for ARM spec seems to agree with
you.

http://infocenter.arm.com/help/topic/com.arm.doc.ihi0040b/IHI0040B_aadwarf.pdf

I think that implies that the ARM64 enum constants are also incorrect.
 AFAICT though, these enums aren't actually evaluated for their integer
representation in either libc++abi or libc++. Is the need to match the
DWARF register numbering just part of the libunwind.h API or am I missing
some usage somewhere?
"""


>
> We should also comment/document how arm’s model of s/d/q overlapping
> registers works with the unwinder.   Since this unwinder’s main goal is to
> support C++ exception unwinding, and the ABI says only d8-d15 need to be
> preserved (that is other double in q registers are volatile), the unwinder
> really only needs to support saving and restoring d8-d15.  That is, even
> though arm has “vector” registers, validVectorRegister() will always
>  return false (as you have done in you patch).
>

Albert: """My current understanding is that this structure is meant to back
the
implementation of the VRS functions like:

  _Unwind_VRS_Get()

that are defined in section 7.5 of the EHABI spec:


http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038a/IHI0038A_ehabi.pdf

>From the tables, it looks like we actually should support R0-R15, S0-S31,
D0-D31, and WMMX {data,control}."""


>
> > Index: src/Unwind/UnwindRegistersRestore.S
> > ===================================================================
> > --- src/Unwind/UnwindRegistersRestore.S       (revision 196555)
> > +++ src/Unwind/UnwindRegistersRestore.S       (working copy)
> > @@ -316,8 +316,27 @@
> >    ldp    x0, x1,  [x0, #0x000]  ; restore x0,x1
> >    ret    lr            ; jump to pc
> >
> > +#elif __arm__
> >
> > +  .text
> > +  .globl _ZN9libunwind13Registers_arm6jumptoEv
> > +  .hidden _ZN9libunwind13Registers_arm6jumptoEv
> We really need to wrap these directives and symbols names in macros to be
> portable.
> For instance, mach-o needs and extra leading underscore added by the macro.
> The compiler_rt project has some such macros.


>
> > +@
> > +@ void libunwind::Registers_arm::jumpto()
> > +@
> > +@ On entry:
> > +@  thread_state pointer is in r0
> > +@
> > +_ZN9libunwind13Registers_arm6jumptoEv:
> > +  @ Use lr as base so that r0 can be restored.
> > +  mov lr, r0
> > +  @ 32bit thumb-2 restrictions for ldm:
> > +  @ . the sp (r13) cannot be in the list
> > +  @ . the pc (r15) and lr (r14) cannot both be in the list in an LDM
> instruction
> > +  ldm lr, {r0-r12}
> > +  ldr sp, [lr, #52]
> > +  ldr lr, [lr, #60]  @ restore pc into lr
> > +  mov pc, lr
> >
> > -
> >  #endif
> >
> > Index: src/Unwind/UnwindRegistersSave.S
> > ===================================================================
> > --- src/Unwind/UnwindRegistersSave.S  (revision 196555)
> > +++ src/Unwind/UnwindRegistersSave.S  (working copy)
> > @@ -282,5 +282,26 @@
> >    ldr    x0, #0      ; return UNW_ESUCCESS
> >    ret
> >
> > +#elif __arm__ && !__APPLE__
> Why is this one !__APPLE, but the other is not?
>

Me: """This was actually intentional. The Registers classes are marked
_LIBUNWIND_HIDDEN so defining them unconditionally should be fine as long
as there are no callers. unw_getcontext is
only __attribute__((unavailable)) which I think is only a warning. I could
make the other !__APPLE__ too if you want, but that's the reasoning."""


>
> > +
> > +@
> > +@ extern int unw_getcontext(unw_context_t* thread_state)
> > +@
> > +@ On entry:
> > +@  thread_state pointer is in r0
> > +@
> > +  .globl unw_getcontext
> > +unw_getcontext:
> > +  @ 32bit thumb-2 restrictions for stm:
> > +  @ . the sp (r13) cannot be in the list
> > +  @ . the pc (r15) cannot be in the list in an STM instruction
> > +  stm r0, {r0-r12}
> > +  str sp, [r0, #52]
> > +  str lr, [r0, #56]
> > +  str lr, [r0, #60]  @ store return address as pc
> > +  mov r0, #0      @ return UNW_ESUCCESS
> > +  mov pc, lr
> > +  @ FIXME: VFP registers
> > +
> >  #endif
> >
>
>
>
> >
> > One thing we were wondering about: Registers_arm64::validRegister()
> returns true for UNW_ARM64_D0 - UNW_ARM64_D31, but
> Registers_arm64::getRegister() aborts if one of these is passed in, which
> suggests that unw_get_reg() will abort if called for one of these
> registers. Should validRegister() return false for the D registers? (The
> 32bit implementation does the same thing as the 64bit implementation for
> now.)
> That looks like a bug in Registers_arm64::validRegister().  Perhaps we
> should rename validRegister() to be validIntegerRegister() to help avoid
> confusion like that.  It is supposed to just check if the register number
> is an integer register number.
>

Albert: """By inspection, the Register_XXX classes all seem to follow a
contract where
the getRegister() allows access to the general purpose and control
registers. I'm tempted to think we can clarify this via comments. If we
were to change this convention for Register_arm, I'd be tempted to suggest
changing it throughout all the platforms since the Register_XXX instances
are conceptually a type family."""

Either way, I'd like to do this in a follow-up if possible.


>
> -Nick
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131214/34e2f8f3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: armregs.patch
Type: application/octet-stream
Size: 20783 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131214/34e2f8f3/attachment.obj>


More information about the cfe-commits mailing list