Changes LGTM. Might be a good idea for kledzik or nbjoerg to have a look too,
but post-commit review is probably fine for that.
================
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*);
----------------
Logan Chien wrote:
> 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.
Ok. It's not super important to me.
================
Comment at: src/cxa_exception.cpp:311
@@ +310,3 @@
+*/
+__attribute__((visibility("hidden"))) _Unwind_Exception *
+__cxa_end_cleanup_impl()
----------------
Logan Chien wrote:
> 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.
Ok. Sounds good.
================
Comment at: src/cxa_exception.cpp:340
@@ +339,3 @@
+asm (
+ " .pushsection .text.__cxa_end_cleanup\n"
+ " .globl __cxa_end_cleanup\n"
----------------
Logan Chien wrote:
> 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.
Oh, right.
================
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"
----------------
Logan Chien wrote:
> 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.
Ok. Doing what clang does for `extern`s sounds good to me.
================
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"
----------------
Logan Chien wrote:
> 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.
Ohhhhh, I see now.
================
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))
{
----------------
Logan Chien wrote:
> 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.
Ok.
http://reviews.llvm.org/D3613
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
