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

Mon Jun 16 10:05:27 PDT 2014

Nick, do you want us to put EHABI in a separate file before upstreaming, 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/7bbdd6cc/attachment.html>