ping
On Sat, Dec 14, 2013 at 12:25 PM, Nico Weber <[email protected]> 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 <[email protected]> wrote: > >> >> 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. >> > > 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 >> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
