On Jun 16, 2014, at 10:05 AM, Nico Weber <[email protected]> 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 <[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
