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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >> >> On Thu, Jun 5, 2014 at 7:52 AM, Nick Kledzik <[email protected]> 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 <[email protected]> wrote: >>> >>> Is this new patch fine for now? (Everything else depends on it.) >>> >>> >>> On Wed, Jun 4, 2014 at 3:27 AM, Dana Jansens <[email protected]> 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 <[email protected]> wrote: >>>> >>>>> On Wed, Jun 4, 2014 at 2:09 AM, Nick Kledzik <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> On Jun 3, 2014, at 4:58 PM, Dana Jansens <[email protected]> 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 >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>> >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
