Page MenuHomePhabricator

[libunwind] Implement the Get/SetTopOfFunctionStack functions via a __thread TLS variable
AbandonedPublic

Authored by mstorsjo on Sep 25 2017, 12:57 PM.

Details

Reviewers
compnerd
Summary

When targeting apple platforms, these functions are implemented in
Unwind_AppleExtras.cpp, but there's previously no implementation for
other platforms.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 25 2017, 12:57 PM
compnerd requested changes to this revision.Sep 25 2017, 9:05 PM

I have a complete implementation for this which is known to work on Windows at least. The only thing that __thread requires is a working linker (which does not include binutils -- I do have a local patch to make that work though). Are these two the only ones that are missing? At the very least, the implementation of SetTopOfFunctionStack is incorrect.

This revision now requires changes to proceed.Sep 25 2017, 9:05 PM

I have a complete implementation for this which is known to work on Windows at least.

Patches for libunwind, or standalone? In any case, please do share if you think it's relevant.

The only thing that __thread requires is a working linker (which does not include binutils -- I do have a local patch to make that work though).

Ok, thanks for clarifying. (If I read wikipedia right, it requires Vista if built into a DLL that isn't linked to by an executable but loaded via LoadLibrary though.)

Are these two the only ones that are missing?

As far as I've seen in my tests, these are the only ones missing yes. Most of the rest of Unwind_AppleExtras.cpp contains things not relating to SJLJ.

At the very least, the implementation of SetTopOfFunctionStack is incorrect.

In which way? My implementation does pretty much exactly the same as the apple version of SetTopOfFunctionStack, but using a __thread variable instead of _pthread_setspecific_direct(__PTK_LIBC_DYLD_Unwind_SjLj_Key, fc);.

Ugh, I was mixing up _Unwind_SjLj_Register with this function.

src/Unwind-sjlj.c
468

I would prefer:

#if !defined(__APPLE__)
469

Please make this static.

481

Can't both of these also be static?

mstorsjo added inline comments.Sep 28 2017, 10:56 PM
src/Unwind-sjlj.c
468

Sure, I can change that.

469

Oh, indeed, yes, I'll change that.

481

No, since they're declared earlier as non-static.

mstorsjo updated this revision to Diff 117089.Sep 28 2017, 10:57 PM
mstorsjo edited edge metadata.
mstorsjo edited the summary of this revision. (Show Details)

Made the tls variable static, changed ifndef into !defined().

compnerd accepted this revision.Oct 1 2017, 12:25 PM
compnerd added inline comments.
src/Unwind-sjlj.c
481

Yeah, that is a bug :-). __Unwind_SjLj_GetTopOfFunctionStack and __Unwind_SjLj_SetToOfFunctionStack are implementation details of LLVM's libunwind. They are not part of the public interfaces, and are not used outside of this TU, and should be marked as static as such. I think that changing the prototype declaration to indicate this is reasonable. I suppose that I can make that change separately.

This revision is now accepted and ready to land.Oct 1 2017, 12:25 PM
compnerd requested changes to this revision.Oct 1 2017, 12:40 PM

Actually, Ill go ahead and make the more invasive change and that will provide this properly.

This revision now requires changes to proceed.Oct 1 2017, 12:40 PM
mstorsjo added inline comments.Oct 1 2017, 1:07 PM
src/Unwind-sjlj.c
481

Yes, except that in the apple case, those functions are provided by another TU (Unwind_AppleExtras.cpp). So in that case, those functions only should be static in the non-apple case.

I believe that SVN r314632 should address the issue more thorougly.

mstorsjo abandoned this revision.Oct 1 2017, 1:25 PM

Works fine with the version from r314632 (with a minor fixup in r314635).