[libcxxabi patch] Add Registers_arm class and unw_getcontext implementation
Nico Weber
thakis at chromium.org
Mon Dec 9 11:15:53 PST 2013
On Sun, Dec 8, 2013 at 12:13 AM, Albert J. Wong (王重傑)
<ajwong at chromium.org>wrote:
> 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.
>>
>
We can add an assembly.h file like compiler-rt has it. Do you just want a
macro that prepends __USER_LABEL_PREFIX__, or do you want more macros from
that file too? Do you want this in a prerequisite to the patch here, as
part of the patch here, or as a follow-up?
>
> 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.
>
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.
>>
>
> 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
>> ********************************************
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131209/e03fe98f/attachment.html>
More information about the cfe-commits
mailing list