[libcxxabi patch] Add Registers_arm class and unw_getcontext implementation
Albert J. Wong (王重傑)
ajwong at chromium.org
Sun Dec 8 00:13:38 PST 2013
Hi Nick,
Thanks for the review. Responses inline...note that I'm not 100% confident
in my understanding so please take anything I say with a couple grains of
salt.
> Date: Fri, 06 Dec 2013 11:37:33 -0800
> From: Nick Kledzik <kledzik at apple.com>
> To: Nico Weber <thakis at chromium.org>
> Cc: "cfe-commits at cs.uiuc.edu" <cfe-commits at cs.uiuc.edu>
> Subject: Re: [libcxxabi patch] Add Registers_arm class and
> unw_getcontext implementation
> Message-ID: <E521D5D8-9671-4B4E-AA81-CFC5F91889EB at apple.com>
> Content-Type: text/plain; charset=windows-1252
>
>
> 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.
>
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).
>
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.
>
Noted...might add a FIXME first...need to look closer at code.
> +@
> > +@ 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?
>
Likely an oversight. Again, will look when I can get back to a terminal
with real code.
> +
> > +@
> > +@ 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.
>
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.
Thoughts?
-Albert
> -Nick
>
>
>
>
>
> ------------------------------
>
> Message: 5
> Date: Fri, 6 Dec 2013 11:55:49 -0800
> From: Warren Hunt <whunt at google.com>
> To: david.majnemer at gmail.com, richard at metafoo.co.uk, whunt at google.com
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Fixing Alias-Avoidance Padding for MS-ABI Layout
> Message-ID:
> <5d8023b843b655e19ee644de3b9c2ce6 at llvm-reviews.chandlerc.com>
> Content-Type: text/plain; charset="utf-8"
>
> Addressing Richard's comments and further cleanup. This will be the
> version committed.
>
> Hi majnemer, rsmith,
>
> http://llvm-reviews.chandlerc.com/D2258
>
> CHANGE SINCE LAST DIFF
> http://llvm-reviews.chandlerc.com/D2258?vs=5741&id=5961#toc
>
> Files:
> include/clang/AST/RecordLayout.h
> lib/AST/RecordLayout.cpp
> lib/AST/RecordLayoutBuilder.cpp
> test/Layout/ms-x86-alias-avoidance-padding.cpp
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: D2258.2.patch
> Type: text/x-patch
> Size: 20277 bytes
> Desc: not available
> URL: <
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20131206/bd3f6909/attachment.bin
> >
>
> ------------------------------
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> End of cfe-commits Digest, Vol 78, Issue 131
> ********************************************
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131208/f31752bd/attachment.html>
More information about the cfe-commits
mailing list