The unwinder changes should be a different patch than the other libcxxabi changes. Post-commit review is fine.
Some other details I just noticed: Are the changes to UnwindLevel1.c to cast logging parameters to long-long still needed now that ARM is no longer compiling that file? It looks like the definition of static_assert() is commented out. -Nick On Jun 24, 2014, at 3:19 PM, Nico Weber <[email protected]> wrote: > On Tue, Jun 24, 2014 at 2:18 PM, Nick Kledzik <[email protected]> wrote: > Nico, > > This looks good now. > > Cool, thanks! What does this mean in practice? > > a) Check in Dana's patch on this thread, post next patch, wait for review > b) Check in everything we have, in logical chunks, and use post-commit review > where necessary? > > Thanks, > Nico > > > -Nick > > On Jun 22, 2014, at 2:02 PM, Nico Weber <[email protected]> wrote: >> On Mon, Jun 16, 2014 at 1:15 PM, Nick Kledzik <[email protected]> wrote: >> >> 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. >> >> Please take another look: >> https://github.com/awong-dev/ndk/compare/upstream-llvm-ndkpatched...master#files_bucket >> >> I renamed Unwind-arm.cpp to Unwind-EHABI.cpp, and moved all new code from >> UnwindLevel1.c to there instead. I also reformatted the 80col lines. The >> file still calls the unw_ functions, let me know if you want us to move off >> those before upstreaming. >> >> > * 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. >> >> Registers.hpp is currently free of preprocessor defines and relies on the >> linker throwing out unused functions, so I didn't do this. Are you worried >> about jumpTo() having different behavior? Right now, Registers_arm is only >> used for EHABI unwinding, so it seems waiting with these ifdefs until >> someone wants to add itanium unwinding for arm might be ok? >> >> Thanks, >> Nico >> >> (ps: Please ignore the first change in UnwindRegistersSave.S, I think that's >> some merge bug. https://github.com/awong-dev/ndk/pull/152 fixes it.) > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
