<div dir="ltr">On Sun, Dec 8, 2013 at 12:13 AM, Albert J. Wong (王重傑) <span dir="ltr"><<a href="mailto:ajwong@chromium.org" target="_blank">ajwong@chromium.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>Hi Nick,</div><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Date: Fri, 06 Dec 2013 11:37:33 -0800<br>
From: Nick Kledzik <<a href="mailto:kledzik@apple.com" target="_blank">kledzik@apple.com</a>><br>
To: Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>><br>
Cc: "<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>" <<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>><br>
Subject: Re: [libcxxabi patch] Add Registers_arm class and<br>
unw_getcontext implementation<br>
Message-ID: <<a href="mailto:E521D5D8-9671-4B4E-AA81-CFC5F91889EB@apple.com" target="_blank">E521D5D8-9671-4B4E-AA81-CFC5F91889EB@apple.com</a>><br>
Content-Type: text/plain; charset=windows-1252<br>
<br>
<br>
On Dec 5, 2013, at 10:32 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
<br>
> Hi,<br>
><br>
> the attached patch adds Registers_arm and assembly for unw_getcontext. VFP support is still missing. The class isn't used by anything yet.<br>
<br>
<br>
> +// 32-bit ARM64 registers<br>
> +enum {<br>
> + UNW_ARM_R0 = 0,<br>
> + UNW_ARM_R1 = 1,<br>
> ...<br>
> + UNW_ARM_D0 = 64,<br>
> + UNW_ARM_D1 = 65,<br>
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.<br>
</blockquote><div><br></div><div>Hmm...section 3.1 of the DWARF for ARM spec seems to agree with you. </div><div><br></div><div><a href="http://infocenter.arm.com/help/topic/com.arm.doc.ihi0040b/IHI0040B_aadwarf.pdf" target="_blank">http://infocenter.arm.com/help/topic/com.arm.doc.ihi0040b/IHI0040B_aadwarf.pdf</a><br>
</div><div><br></div><div>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?</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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).<br>
</blockquote><div><br></div><div>My current understanding is that this structure is meant to back the implementation of the VRS functions like:</div><div><br></div><div> _Unwind_VRS_Get()</div><div><br></div><div>that are defined in section 7.5 of the EHABI spec:</div>
<div><br></div><div> <a href="http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038a/IHI0038A_ehabi.pdf" target="_blank">http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038a/IHI0038A_ehabi.pdf</a></div><div><br>
</div><div>From the tables, it looks like we actually should support R0-R15, S0-S31, D0-D31, and WMMX {data,control}.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> Index: src/Unwind/UnwindRegistersRestore.S<br>
> ===================================================================<br>
> --- src/Unwind/UnwindRegistersRestore.S (revision 196555)<br>
> +++ src/Unwind/UnwindRegistersRestore.S (working copy)<br>
> @@ -316,8 +316,27 @@<br>
> ldp x0, x1, [x0, #0x000] ; restore x0,x1<br>
> ret lr ; jump to pc<br>
><br>
> +#elif __arm__<br>
><br>
> + .text<br>
> + .globl _ZN9libunwind13Registers_arm6jumptoEv<br>
> + .hidden _ZN9libunwind13Registers_arm6jumptoEv<br>
We really need to wrap these directives and symbols names in macros to be portable.<br>
For instance, mach-o needs and extra leading underscore added by the macro.<br>
The compiler_rt project has some such macros.<br></blockquote></div></div></div></blockquote><div><br></div><div>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?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"></blockquote><div><br></div><div>Noted...might add a FIXME first...need to look closer at code.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> +@<br>
> +@ void libunwind::Registers_arm::jumpto()<br>
> +@<br>
> +@ On entry:<br>
> +@ thread_state pointer is in r0<br>
> +@<br>
> +_ZN9libunwind13Registers_arm6jumptoEv:<br>
> + @ Use lr as base so that r0 can be restored.<br>
> + mov lr, r0<br>
> + @ 32bit thumb-2 restrictions for ldm:<br>
> + @ . the sp (r13) cannot be in the list<br>
> + @ . the pc (r15) and lr (r14) cannot both be in the list in an LDM instruction<br>
> + ldm lr, {r0-r12}<br>
> + ldr sp, [lr, #52]<br>
> + ldr lr, [lr, #60] @ restore pc into lr<br>
> + mov pc, lr<br>
><br>
> -<br>
> #endif<br>
><br>
> Index: src/Unwind/UnwindRegistersSave.S<br>
> ===================================================================<br>
> --- src/Unwind/UnwindRegistersSave.S (revision 196555)<br>
> +++ src/Unwind/UnwindRegistersSave.S (working copy)<br>
> @@ -282,5 +282,26 @@<br>
> ldr x0, #0 ; return UNW_ESUCCESS<br>
> ret<br>
><br>
> +#elif __arm__ && !__APPLE__<br>
Why is this one !__APPLE, but the other is not?<br></blockquote><div><br></div><div>Likely an oversight. Again, will look when I can get back to a terminal with real code.</div></div></div></div></blockquote><div><br></div>
<div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> +<br>
> +@<br>
> +@ extern int unw_getcontext(unw_context_t* thread_state)<br>
> +@<br>
> +@ On entry:<br>
> +@ thread_state pointer is in r0<br>
> +@<br>
> + .globl unw_getcontext<br>
> +unw_getcontext:<br>
> + @ 32bit thumb-2 restrictions for stm:<br>
> + @ . the sp (r13) cannot be in the list<br>
> + @ . the pc (r15) cannot be in the list in an STM instruction<br>
> + stm r0, {r0-r12}<br>
> + str sp, [r0, #52]<br>
> + str lr, [r0, #56]<br>
> + str lr, [r0, #60] @ store return address as pc<br>
> + mov r0, #0 @ return UNW_ESUCCESS<br>
> + mov pc, lr<br>
> + @ FIXME: VFP registers<br>
> +<br>
> #endif<br>
><br>
<br>
<br>
<br>
><br>
> 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.)<br>
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.<br>
</blockquote><div><br></div><div>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.</div>
<div><br></div><div>Thoughts?</div><div><br></div><div>-Albert</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
-Nick<br>
<br>
<br>
<br>
<br>
<br>
------------------------------<br>
<br>
Message: 5<br>
Date: Fri, 6 Dec 2013 11:55:49 -0800<br>
From: Warren Hunt <<a href="mailto:whunt@google.com" target="_blank">whunt@google.com</a>><br>
To: <a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>, <a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>, <a href="mailto:whunt@google.com" target="_blank">whunt@google.com</a><br>
Cc: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
Subject: Re: [PATCH] Fixing Alias-Avoidance Padding for MS-ABI Layout<br>
Message-ID:<br>
<<a href="mailto:5d8023b843b655e19ee644de3b9c2ce6@llvm-reviews.chandlerc.com" target="_blank">5d8023b843b655e19ee644de3b9c2ce6@llvm-reviews.chandlerc.com</a>><br>
Content-Type: text/plain; charset="utf-8"<br>
<br>
Addressing Richard's comments and further cleanup. This will be the version committed.<br>
<br>
Hi majnemer, rsmith,<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2258" target="_blank">http://llvm-reviews.chandlerc.com/D2258</a><br>
<br>
CHANGE SINCE LAST DIFF<br>
<a href="http://llvm-reviews.chandlerc.com/D2258?vs=5741&id=5961#toc" target="_blank">http://llvm-reviews.chandlerc.com/D2258?vs=5741&id=5961#toc</a><br>
<br>
Files:<br>
include/clang/AST/RecordLayout.h<br>
lib/AST/RecordLayout.cpp<br>
lib/AST/RecordLayoutBuilder.cpp<br>
test/Layout/ms-x86-alias-avoidance-padding.cpp<br>
-------------- next part --------------<br>
A non-text attachment was scrubbed...<br>
Name: D2258.2.patch<br>
Type: text/x-patch<br>
Size: 20277 bytes<br>
Desc: not available<br>
URL: <<a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20131206/bd3f6909/attachment.bin" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20131206/bd3f6909/attachment.bin</a>><br>
<br>
------------------------------<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
<br>
End of cfe-commits Digest, Vol 78, Issue 131<br>
********************************************<br>
</blockquote></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>