On Sun, Dec 8, 2013 at 12:13 AM, Albert J. Wong (王重傑) <[email protected]>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 <[email protected]> >> To: Nico Weber <[email protected]> >> Cc: "[email protected]" <[email protected]> >> Subject: Re: [libcxxabi patch] Add Registers_arm class and >> unw_getcontext implementation >> Message-ID: <[email protected]> >> Content-Type: text/plain; charset=windows-1252 >> >> >> On Dec 5, 2013, at 10:32 PM, Nico Weber <[email protected]> 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 <[email protected]> >> To: [email protected], [email protected], [email protected] >> Cc: [email protected] >> Subject: Re: [PATCH] Fixing Alias-Avoidance Padding for MS-ABI Layout >> Message-ID: >> <[email protected]> >> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >> End of cfe-commits Digest, Vol 78, Issue 131 >> ******************************************** >> > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
