[PATCH] Split unwind register save/restore files per-function, and appease the assembler.

Jonathan Roelofs jonathan at codesourcery.com
Fri Jan 30 12:56:29 PST 2015


================
Comment at: Unwind/CMakeLists.txt:25
@@ -15,1 +24,3 @@
+  UnwindRegistersSave_saveiWMMX.S
+  UnwindRegistersSave_unw_getcontext.S
 )
----------------
compnerd wrote:
> It would be nice to split these on architecture boundary and keep the variants in a single file.  Something like:
> 
>     UnwindRegistersRestore-${ARCH}.S
>     UnwindRegistersSave-${ARCH}.S
> 
> Might be nice to do that for the current save/restore file as a setup change.
I split them per-function so that there was no possible way a `.fpu` directive accidentally applied to something it shouldn't. If we could push & pop fpu states, I'd be less worried about it.

================
Comment at: Unwind/CMakeLists.txt:46
@@ -34,3 +45,3 @@
 
-append_if(LIBCXXABI_HEADERS APPLE ../../include/mach-o/compact_unwind_encoding.h)
+#append_if(LIBCXXABI_HEADERS APPLE ../../include/mach-o/compact_unwind_encoding.h)
 
----------------
compnerd wrote:
> I think you didn't mean for this to be part of the change.
oops, yeah, I didn't.

================
Comment at: Unwind/UnwindRegistersRestore_restoreCoreAndJumpTo.S:14
@@ +13,3 @@
+
+#if __arm__ && !__APPLE__
+
----------------
compnerd wrote:
> Since we effectively never assembly this file in the `__APPLE__` case anyways, why not change the build system to only assembly this file if APPLE (and similar through out which won't be needed if we do the single file per arch).
The "right" fix is to use the defines which say whether unwind is sjlj on apple or not. Spewing that assumption around in the form of `__APPLE__` probably isn't right, agreed.

================
Comment at: Unwind/UnwindRegistersRestore_restoreCoreAndJumpTo.S:16
@@ +15,3 @@
+
+#if !defined(__ARM_ARCH_ISA_ARM)
+  .thumb
----------------
compnerd wrote:
> Why not make this more explicit:
> 
>     #if defined(__ARM_ARCH_ISA_THUMB)
>       .thumb
>     #elif defined(__ARM_ARCH_ISA_ARM)
>       .arm
>     #else
>       #error unsupported ARM ISA
>     #endif
> 
> And similar through out which won't be needed if we do the single file per arch.
sounds reasonable to me.

http://reviews.llvm.org/D7258

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list