[PATCH] Implement ARM EHABI exception handling.

Logan Chien tzuhsiang.chien at gmail.com
Tue May 6 11:42:57 PDT 2014


Thanks for your comments.  Will be fixed in next revision.

================
Comment at: include/unwind.h:28
@@ +27,3 @@
+#define LIBCXXABI_ARM_EHABI 1
+#endif
+
----------------
Jon Roelofs wrote:
> #else
> #define LIBCXXABI_ARM_EHABI 0
> #endif
Done.

================
Comment at: include/unwind.h:67
@@ +66,3 @@
+struct _Unwind_Control_Block {
+  uint64_t exception_class;
+  void (*exception_cleanup)(_Unwind_Reason_Code, _Unwind_Control_Block*);
----------------
Jon Roelofs wrote:
> #7.2 says this should be:
> char exception_class[8];
> 
> Maybe add a FIXME here for that?
I am not sure whether we are going to fix this.  There has been several pieces of code using integer arithmetic to determinate whether this is a native exception or foreign exception.  I can add the comment if you insist.

================
Comment at: src/cxa_exception.cpp:311
@@ +310,3 @@
+*/
+__attribute__((visibility("hidden"))) _Unwind_Exception *
+__cxa_end_cleanup_impl()
----------------
Jon Roelofs wrote:
> Is this a public interface? or should it be static?
No this is not a public interface.  When I was writing this code, I have tried to use `static`.  However, this function will be ignored by compiler because there is no function call to this static function.

But now, I have found `__attribute__((used))`, which can solve the issue. I will change this in the next revision.

================
Comment at: src/cxa_exception.cpp:340
@@ +339,3 @@
+asm (
+    "	.pushsection	.text.__cxa_end_cleanup\n"
+    "	.globl	__cxa_end_cleanup\n"
----------------
Jon Roelofs wrote:
> Is _Unwind_Resume going to need a manual entry for the .ARM.exidx section for this function so that _Unwind_Resume can find it?
> 
> I think it just needs to be a Su16 entry with 3 Finish opcodes.
`__cxa_end_cleanup()` is a bridge for the cleanup handler to return to unwinder.  Thus, we don't have to unwind the stack containing from the cleanup handler.  In fact, `_Unwind_Resume` will load the information from `_Unwind_Control_Block`, which contains the actual stack to be unwinded.

Thus, it is fine for not having `.fnstart` and `.fnend` directives here.

================
Comment at: src/cxa_exception.cpp:343
@@ +342,3 @@
+    "	.type	__cxa_end_cleanup,%function\n"
+    "__cxa_end_cleanup:\n"
+    "	push	{r1, r2, r3, r4}\n"
----------------
Jon Roelofs wrote:
> Does this need a
> ".globl __cxa_end_cleanup\n"
> line to make it visible to ld (or does the .type line take care of that)?
I am not sure about this, but this is what clang generates for `extern` functions.  I guess .globl is for the linkage and .type is for the ELF symbol type, thus we need both of them.

================
Comment at: src/cxa_exception.cpp:344
@@ +343,3 @@
+    "__cxa_end_cleanup:\n"
+    "	push	{r1, r2, r3, r4}\n"
+    "	bl	__cxa_end_cleanup_impl\n"
----------------
Jon Roelofs wrote:
> I'm not sure from reading #8.4.1 and #7.4.6 why this is r1-r4 and not r0-r3... r0 is callee save and likely to get clobbered by the call to isOurExceptionClass in __cxa_end_cleanup_impl, no?
r0-r3 are caller save registers.  However, r0 will (and should) be covered by the return value, thus we don't have to push them to stack.  On the other hand, r4 is only pushed for stack alignment.

================
Comment at: src/cxa_personality.cpp:324
@@ +323,3 @@
+{
+    if (classInfo == 0)
+    {
----------------
Jon Roelofs wrote:
> Should we assert that the ttypeEncoding is DW_EH_PE_absptr?
Done.

================
Comment at: src/cxa_personality.cpp:396
@@ +395,3 @@
+    //    adjustments to adjustedPtr are ignored.
+    for (const void** ttypePtr = reinterpret_cast<const void**>(table); *ttypePtr; ++ttypePtr)
+    {
----------------
Jon Roelofs wrote:
> Maybe add a comment here to highlight the difference between ARM EHABI and the Itanium ABI, and explain why the implementation needs to be different. On ARM, these are 0-delimited runs of TARGET2 relocation pointers to the typeinfos, whereas in the rest of the Itanium EH ABI, these are 0-delimited runs of uleb128 indexes into the type table.
Done.

================
Comment at: src/cxa_personality.cpp:712
@@ -658,3 +711,3 @@
                             // If this is a type 3 search and _UA_FORCE_UNWIND, ignore handler and continue scan
-                            if (actions & _UA_SEARCH_PHASE)
+                            if ((actions & _UA_SEARCH_PHASE) || (actions & _UA_HANDLER_FRAME))
                             {
----------------
Jon Roelofs wrote:
> Maybe the part added here should go under #if LIBCXXABI_ARM_EHABI ?
These won't be needed and will be removed in next revision.  (I have fixed the TODO in the phase 2 search.  Thus, we don't have to perform the search again.)  Thanks.

http://reviews.llvm.org/D3613






More information about the cfe-commits mailing list