Page MenuHomePhabricator

disable sigaltstack on Apple platforms
ClosedPublic

Authored by bob.wilson on Jan 3 2017, 4:45 PM.

Details

Summary

Using sigaltstack on Apple platforms is a bad idea. Darwin's backtrace() function does not work with sigaltstack, and my change in r286851 was supposed to solve that by using _Unwind_Backtrace instead. I tested that _Unwind_Backtrace works for crashes but then discovered that it does not work for assertion failures when using sigaltstack, at least on macOS. The stack trace shows only the frames on the alternate stack. I also saw some reports of this happening for crashes, but it fails consistently for assertion failures. I tried various things to get it to work but the problem seems to be in _Unwind_Backtrace itself. Disabling sigaltstack is unfortunate since it would be nice to get backtraces for stack overflows, but at least this gets us backtraces for the more common cases. rdar://problem/29662459

Diff Detail

Event Timeline

bob.wilson updated this revision to Diff 82969.Jan 3 2017, 4:45 PM
bob.wilson retitled this revision from to disable sigaltstack on Apple platforms.
bob.wilson updated this object.
bob.wilson added a subscriber: llvm-commits.
davide edited edge metadata.Jan 4 2017, 4:58 AM

This is not ideal, TBH. As a temporary workaround to avoid crashes, I'm not opposed about this going in, but my question is: how hard is to make _Unwind_Backtrace reliable? (I assume you need this change anyway for a while but at least you can provide a proper fix for the next release and discriminate on the version number).

Yes, of course you are right that this is not ideal. It's not about avoiding crashes though -- the issue is that the backtraces we dump out after an assertion failure are useless since they stop at the signal handler.

Your comment prompted me to look a little deeper, and I discovered that the _Unwind_Backtrace issue is not specific to sigaltstack. We first try to use the backtrace() function, and if you're not using sigaltstack, that succeeds and we never try using _Unwind_Backtrace. If I modify the code to skip trying backtrace(), I consistently get stack traces that stop at the signal handler after an assertion failure. I filed a bug report about that for libunwind (rdar://problem/29866587). As I mentioned earlier with r286851, I also filed a bug report about backtrace() not working with sigaltstack, but I didn't get the impression that was being treated as a high priority.

Even if we get a fix for libunwind or backtrace (and I don't know if/when that might happen), it will take a while to get it released in macOS and we should wait a year or so beyond that so that the majority of LLVM developers will have updated to get the fix. In the meantime, we need this as a workaround. I should also revert my change in r286851 until _Unwind_Backtrace works reliably on Darwin.

davide accepted this revision.Jan 4 2017, 10:23 AM
davide edited edge metadata.

Yes, of course you are right that this is not ideal. It's not about avoiding crashes though -- the issue is that the backtraces we dump out after an assertion failure are useless since they stop at the signal handler.

Your comment prompted me to look a little deeper, and I discovered that the _Unwind_Backtrace issue is not specific to sigaltstack. We first try to use the backtrace() function, and if you're not using sigaltstack, that succeeds and we never try using _Unwind_Backtrace. If I modify the code to skip trying backtrace(), I consistently get stack traces that stop at the signal handler after an assertion failure. I filed a bug report about that for libunwind (rdar://problem/29866587). As I mentioned earlier with r286851, I also filed a bug report about backtrace() not working with sigaltstack, but I didn't get the impression that was being treated as a high priority.

Even if we get a fix for libunwind or backtrace (and I don't know if/when that might happen), it will take a while to get it released in macOS and we should wait a year or so beyond that so that the majority of LLVM developers will have updated to get the fix. In the meantime, we need this as a workaround. I should also revert my change in r286851 until _Unwind_Backtrace works reliably on Darwin.

Yes ... that's ... unfortunate. LGTM (with the hope that newer Mac OS/Darwin versions will ship with the fix)

This revision is now accepted and ready to land.Jan 4 2017, 10:23 AM
davide added inline comments.Jan 4 2017, 10:28 AM
config-ix.cmake
170–171

Just a small request: can you you expand this comment a little more? A condensed version of your comment would be excellent, ideally we should have a reference to something publicly accessing, but, that lacking a reference to the rdar is probably fine).

170–171

*publicly accessible

bob.wilson closed this revision.Jan 5 2017, 6:38 PM

Committed in r291206, with an expanded comment.
I also reverted my previous change to use _Unwind_Backtrace (r291207).