This is an archive of the discontinued LLVM Phabricator instance.

Fix __builtin_setjmp in combination with sjlj exception handling.
ClosedPublic

Authored by MatzeB on Apr 27 2015, 5:52 PM.

Details

Summary

builtin_setjmp in combination with the sjlj exception handling style was broken. This was caused by the strange decision to produce exception handling dispatch jump tables upon seeing the setjmp instruction. This resulted in builtin_setjmp being broken on ARM because it tried to build those dispatch tables with no sjlj exception handling code being present.

Commit message: SJLJ exception handling expects the backends to emit dispatch jump
tables at the beginning of the function, we cannot reuse llvm.eh.sjlj.setjmp to trigger this effect or we get into trouble when __builtin_setjmp was used independently of exception handling.
To solve this issue a new llvm.eh.sjlj.setup_dispatch intrinsic is
introduces which is used instead of llvm.eh.sjlj.setjmp in the sjlj
exception handling style.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 24520.Apr 27 2015, 5:52 PM
MatzeB retitled this revision from to Fix __builtin_setjmp in combination with sjlj exception handling..
MatzeB updated this object.
MatzeB edited the test plan for this revision. (Show Details)
MatzeB added a reviewer: grosbach.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: Unknown Object (MLST).

Isn't llvm.eh.sjlj.setjmp specifically the intrinsic for sjlj exception-handling setjmp? In which case emitting the dispatch table when it's seen sounds reasonable. Intrinsics.td lists it under "Exception Handling Intrinsics".

Clang does generate llvm.eh.sjlj.setjmp for __builtin_setjmp, but that sounds more like a bug in clang to me.

MatzeB added a comment.May 7 2015, 5:06 PM

The thing is eh_sjlj_longjmp was never used by llvms sjlj handling code and the implementation in the X86 target was for a __builtin_setjmp handling and would not have worked with llvms current sjlj exception handling code. So introducing a new intrinsic for eh_sjlj makes alot of sense.

As a followup I created http://reviews.llvm.org/D9589 which should make this obvious that they are used to implement builtin_setjmp and builtin_longjmp by renaming the intrinsics to lightweight_{setjmp|longjmp}.

MatzeB added a subscriber: joerg.May 20 2015, 2:50 PM

Isn't llvm.eh.sjlj.setjmp specifically the intrinsic for sjlj exception-handling setjmp? In which case emitting the dispatch table > when it's seen sounds reasonable. Intrinsics.td lists it under "Exception Handling Intrinsics".

Clang does generate llvm.eh.sjlj.setjmp for __builtin_setjmp, but that sounds more like a bug in clang to me.

I just had a chat with Joerg on IRC as he objected my plan to simply rename sjlj.setjmp to something else. Apparently sjlj.setjmp/sjlj.longjmp are the right thing to use to implement builtin_setjmp and builtin_longjmp and in fact if you look at the SjLj runtime in libcxxabi you will find __builtin_longjmp being used to implement exactly this scheme.

So in short: I think it is expected and fine to use llvm.eh.sjlj.setjmp/llvm.ej.sjlj.longjmp to implement the builtins in clang. So I think the proposed change to split dispatch table setup into an own intrinsic is indeed reasonable.

compnerd accepted this revision.Jul 13 2015, 11:48 AM
compnerd added a reviewer: compnerd.
compnerd added a subscriber: compnerd.

LGTM, though it would be nice to have a test for an unsupported target as well (e.g. Linux).

This revision is now accepted and ready to land.Jul 13 2015, 11:48 AM
This revision was automatically updated to reflect the committed changes.