[libcxxabi patch] Add Registers_arm class and unw_getcontext implementation

[Nico Weber]

ping


On Sat, Dec 14, 2013 at 12:25 PM, Nico Weber <thakis at chromium.org> wrote:

> 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/20131217/4d4f312c/attachment.html>