Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[WIP] Add support for return from an SEH __finally block.
Needs ReviewPublic

Authored by efriedma on Apr 28 2022, 3:57 PM.

Details

Summary

MSVC has a warning C4532 for jumping out of a finally block. However, that only specifically covers break, continue, and goto. There's no warning for __leave and return.

It's not completely clear to me why MSVC considers "__leave" and "return" special, compared to other sorts of control flow. My best guess is that return-in-finally is so pervasive across drivers that the warning would be way too noisy. An alternative theory is that MSVC generates a call to _local_unwind in the normal path for certain uses of break/continue/goto, and that's somehow a hazard for unwinding.

In any case, the end result is that return shows up somewhat frequently in __finally blocks in Windows drivers, but I haven't seen any of the others.

This patch generates code that's similar to what MSVC generates: it branches directly to the return block for normal code, and uses _local_unwind to break out of exception handling.

Currently, this only handles return. Adding support for other ways of exiting a __finally blocks should be straightforward on top of this; it's mostly a question of managing the different possible destinations.

To represent the _local_unwind, this patch includes a new intrinsic llvm.seh_localunwind. This generates the necessary arguments to _local_unwind to the nearest catchpad automatically.

Some basic testing shows this seems to do the right thing in simple cases.

Handling __try/__finally inside the __try of a __try/__finally depends on the change to WinException.cpp. Still a bit ugly, but not sure how to make it cleaner.

__try/__finally inside a __finally block is broken because recoverAddrOfEscapedLocal can't recover the address of a variable allocated inside a __finally block. Here specifically, we compute the wrong value for SEHRetNowParent, but it's a general issue affecting any variable declared inside a __finally block. We probably need to store the frame pointer in an alloca in the parent function, and then load it later so we can recover the pointer we want, or something like that.

Based on the patch by Ten Tzen, particularly the code to call _local_unwind. I rewrote a bunch of the code to simplify it, and avoid using _local_unwind in cases where it isn't necessary.

Diff Detail

Event Timeline

efriedma created this revision.Apr 28 2022, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 3:57 PM
efriedma requested review of this revision.Apr 28 2022, 3:57 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 28 2022, 3:57 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
efriedma edited the summary of this revision. (Show Details)Apr 28 2022, 3:59 PM
dpaoliello resigned from this revision.Apr 28 2022, 4:08 PM
dpaoliello removed a reviewer: dpaoliello.
efriedma updated this revision to Diff 426486.May 2 2022, 12:30 PM
efriedma edited the summary of this revision. (Show Details)

Figured out the __finally nesting thing; we were missing unwind table entries. (This is a consequence of the fact that our unwind tables don't cover the entire function, just invokes.) Need to think about a cleaner solution.

I'm currently thinking it makes sense to introduce an intrinsic "void @llvm.localunwind() noreturn". No arguments. Only valid with SEH personality; must unwind to a catchpad with a filter named __IsLocalUnwind, or something like that. It would basically just lower to a call to _local_unwind, but it would compute the arguments in the backend. Primary downside is that it wouldn't be possible to unwind to anything other than the closest catchpad. But not sure if unwinding further is actually useful in practice, given the ways we need to use it.

The other thing I'm thinking about is how to restructure the code to make the catchpad hack in WinException.cpp a bit less hacky.

efriedma updated this revision to Diff 426814.May 3 2022, 1:25 PM
efriedma edited the summary of this revision. (Show Details)

Switch to use llvm.seh_localunwind intrinsic.

I'd appreciate any feedback at this point.

rnk added a comment.May 16 2023, 9:01 AM

The approach makes sense to me, but can you include test cases to illustrate the IR and code that gets generated?

clang/lib/CodeGen/CGException.cpp
1830

We have the option to generalize the naming here to include other kinds of jumps that exit the scope of the finally. I think it's worth implementing __leave eventually, which I think users have asked for at some point. I could go either way, your choice.

2202

If we were to generalize now, we'd want to name this something like HasLocalUnwind.

llvm/lib/CodeGen/AsmPrinter/WinException.cpp
596–606

Perhaps it's time to revisit this.

623–626

I don't understand what this is doing, but I should look at the tests.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3040

Since this isn't verifier checked yet, is it possible to emit a diagnostic via the LLVMContext and select this instruction to nothing, or would that leave behind disconnected unreachable MBBs that break too many invariants later?

efriedma edited the summary of this revision. (Show Details)May 16 2023, 1:35 PM
efriedma added inline comments.May 16 2023, 1:48 PM
clang/lib/CodeGen/CGException.cpp
1830

Probably better to rename once we actually implement that? I'm intentionally not trying to detect other forms of control flow because EnterSEHTryStmt only knows how to return from the function. If we're going to branch elsewhere, we need to return a list of the destinations, not just a boolean "there's a return statement".

llvm/lib/CodeGen/AsmPrinter/WinException.cpp
623–626

I don't remember all the details of this, but as I noted in the commit message, it affects nesting __try/__finally in __try. I think without this, the destination of the _local_unwind somehow ended up inheriting an unrelated SEH state.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3040

I can't find any other code trying to report diagnostics that way, but I guess I can look into it.

efriedma updated this revision to Diff 522773.May 16 2023, 1:50 PM

Rebased so it builds, and added a couple tests, to unblock anyone wanting to look at this further. Haven't re-done my runtime testing. Still haven't addressed all the review comments.

efriedma updated this revision to Diff 522782.May 16 2023, 2:09 PM

Fixup a couple LLVM tests which are failing; I think they're affected by the WinException.cpp changes? Maybe need to look a bit more closely to see if the changes make sense.