[PATCH] libcxxabi/unwind: Add support for ARM EHABI in AddressSpace.hpp

Nick Kledzik kledzik at apple.com
Mon Jun 16 13:15:50 PDT 2014


On Jun 16, 2014, at 10:05 AM, Nico Weber <thakis at chromium.org> wrote:

> Nick, do you want us to put EHABI in a separate file before upstreaming,
Yes.   It should be easy to de-conditionalize your current changes to UnwindLevel1.c and merge it with your Unwind-arm.c to make the new EHABI-only file.

-Nick



> or is it ok to upstream things mostly as-is and then restructure in-tree?
> 
> 
> On Tue, Jun 10, 2014 at 8:17 AM, Dana Jansens <danakj at google.com> wrote:
> Hi Nick,
> 
> Thanks for the feedback. There's definitely style and #if cleanup to be done as you mentioned. We were worried about just getting everything functional rather than perfect style for everything in our repo so please take it with a grain of salt. We've been intending to clean things up for each patch that we send upstream, as hopefully the AddressSpace patch in this thread demonstrates.
> 
> On Mon, Jun 9, 2014 at 5:51 PM, Nick Kledzik <kledzik at apple.com> wrote:
> Nico,
> 
> After looking over the sources in github, there are enough conditionals added to UnwindLevel1.c that I think it would be more readable to put all the EHABI unwinding stuff into a separate file.  You already have a new Unwind-arm.cpp file.  Rename it to Unwind-EHABI.cpp and add all the changed _Unwind_RaiseException and friends functions to it.  I expect that once you have Unwind-EHABI.cpp only doing EHABI unwinding, you’ll rearrange the algorithms to better match what EHABI does (e.g. remove call to unw_step()).  
> 
> We can certainly do this, and it's something we wanted to address in the longer term, but it's going to push our timeline for upstreaming out -- probably by months. We all took time off our regular tasks to get this working and rewriting it separately won't be as trivial for us. Is there a path for getting the EHABI implementation out in the world without blocking on a full rewrite?
>  
> Some other comments:
> 
> * Some lines over 80 chars.  Consider using clang-format
> 
> * In general, the conditionals should consider the possibility of three unwinders for arm: SJLJ, EHABI, and (someday maybe) Itanium.   For instance, the new functions in UnwindRegisters{Save|Restore}.S should be conditionalized with some EHABI conditional (not just __arm__ && !__APPLE), and in Registers.hpp, all the new EHABI specific stuff in Registers_arm should be conditionalized with ARM_EHABI.
> 
> * There are some uses of:
>    extern “C”
> that can be removed because there should already be a prototype for them in unwind.h that marks them “C”.
> 
> * The new Unwind-arm.cpp has a #include of :../private_typeinfo.h”.  What is it using?  If we move the unwinder to compiler-rt, it won’t have access to that header.
> 
> This was added in f5bcc3af for the call to __cxxabiv1::__cxa_type_match, which we left unimplemented currently as it doesn't appear to be needed to support current clang/g++ compilers. However, __cxa_type_match is officially part of the EHABI and needs deep knowledge of the type_info shims. This means that EHABI is hard to implement fully in compiler_rt.
> 
> Cheers,
> Dana
>  
> -Nick
> 
> 
> On Jun 5, 2014, at 8:39 AM, Nico Weber <thakis at chromium.org> wrote:
>> On Thu, Jun 5, 2014 at 7:52 AM, Nick Kledzik <kledzik at apple.com> wrote:
>> I’ve been at WWDC this week, so I’ll be slow on responding...
>> 
>> Do you have what the final code will look like after all the incremental patches you plan?
>> 
>> Yes, see https://github.com/awong-dev/ndk/compare/upstream-llvm-ndkpatched...master#files_bucket . To see the relevant diffs, click on "Files changed" tab and search for "libcxxabi/"
>> 
>> Also see http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140526/106670.html which gave an outline of our upstreaming plan.
>>  
>>  
>> 
>> I’d like to understand how much overlap there is between the Itanium unwinder and the EHABI unwinder.  If there is not much, then trying to jam the two together with lots of conditionals will just make for hard to read code.  And it is not like the algorithm can be improved in the future (so keeping them together would allow both unwinders to improve at once), because the algorithm just follows what the steps of the spec (Itanium and EHABI).
>> 
>> -Nick
>> 
>> On Jun 4, 2014, at 2:34 PM, Nico Weber <thakis at chromium.org> wrote:
>>> Is this new patch fine for now? (Everything else depends on it.)
>>> 
>>> 
>>> On Wed, Jun 4, 2014 at 3:27 AM, Dana Jansens <danakj at google.com> wrote:
>>> Here's an updated patch that uses LIBCXX_API_EHABI. Since this is in AddressSpace.hpp which is part of the "libunwind" layer, I've duplicated the #define for this from the unwind.h which is part of the "_Unwind" layer to avoid including the unwind.h header which doesn't belong in AddressSpace.hpp.
>>> 
>>> 
>>> On Wed, Jun 4, 2014 at 2:29 AM, Dana Jansens <danakj at google.com> wrote:
>>> On Wed, Jun 4, 2014 at 2:09 AM, Nick Kledzik <kledzik at apple.com> wrote:
>>> 
>>> On Jun 3, 2014, at 4:58 PM, Dana Jansens <danakj at google.com> wrote:
>>> 
>>>> 
>>>> The setjump-longjump is similar to this.  It does not use the lower level libunwind APIs and it has its own phase1/phase2 unwinding code separate from the Itanium style.  So, it makes sense to me for the ARM EHABI  implementation to be in its own Unwind-ehabi.c file and do not use libunwind under it.  This was part of why I thought of EHABI as being a different unwinder than the zero-cost unwinder in terms of _LIBUNWIND_BUILD_blah.
>>>> 
>>>> We discussed making a change like that, but we're more concerned with upstreaming first right now, rather than keeping this all on a private repo. Since the way we developed this was sharing code with the itanium implementation as much as possible, are you okay with upstreaming it in this fashion and then looking at moving it away in the future?
>>>>  
>>> Can you be more specific about what you mean by “in this fashion” and “moving it away in the future”.
>>> 
>>> Sure! What we have in our repo[1] is an implementation of ARM EHABI on top of the Itanium APIs. Initially we felt that made a lot of sense, though more recently we've started thinking about doing something different to fit the ARM EHABI requirements better. So, currently we are sharing code in unwind_level1, and AddressSpace and so on, as much as possible. This also helps keep our diffs smaller, I think.
>>> 
>>> In the future (maybe 6 months out) we could consider moving the implementation away from sharing code with itanium with #ifdefs, and moving to something more separate like SJLJ. But this isn't something we can realistically commit to doing right now, so it would make upstreaming a lot more difficult.
>>> 
>>> What we have is a functioning implementation that passes the tests, so I think it's not unreasonable. Concretely, this means using #if LIBCXX_ARM_EHABI throughout each of the three cxxabi, Unwind, and libunwind layers.
>>> 
>>> [1] https://github.com/awong-dev/ndk/compare/upstream-llvm-ndkpatched...master#commit_comments_bucket
>>>  
>>> 
>>> -Nick
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>> 
>>> 
>> 
>> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140616/585bf2d2/attachment.html>


More information about the cfe-commits mailing list