Page MenuHomePhabricator

Implement CFG construction for __try / __except / __leave.
ClosedPublic

Authored by thakis on Aug 18 2017, 7:02 PM.

Details

Reviewers
rnk
arphaman
Summary

This makes -Wunreachable-code work for programs containing SEH (except for __finally, which is still missing for now).

__try is modeled like try (but simpler since it can only have a single __except or __finally), __except is fairly similar to catch (but simpler, since it can't contain declarations). __leave is implemented similarly to break / continue.

Use the existing addTryDispatchBlock infrastructure (which FindUnreachableCode() in ReachableCode.cpp uses via cfg->try_blocks_begin()) to mark things in the __except blocks as reachable.

Re-use TryTerminatedBlock. This means we add EH edges from calls to the __try block, but not from all other statements. While this is incomplete, it matches LLVM's SEH codegen support. Also, in practice, BuildOpts.AddEHEdges is always false in practice from what I can tell, so we never even insert the call EH edges either.

Diff Detail

Event Timeline

thakis created this revision.Aug 18 2017, 7:02 PM
rnk edited edge metadata.Aug 21 2017, 9:40 AM

Don't add any EH edges to the CFG for SEH. In practice, BuildOpts.AddEHEdges is always false in practice from what I can tell, and with SEH every single stmt would have to get an EH edge.

Since we can't mix C++ EH and SEH, do you think it would be better to reuse the TryTerminatedBlock chain so that we get edges from every call to the __except? That's the approximation of SEH that we actually support in LLVM anyway.

lib/Analysis/CFG.cpp
2586

Looks like a // got re-wrapped in the comment

test/Sema/warn-unreachable-ms.c
24

typo empty

thakis edited the summary of this revision. (Show Details)

Just use TryTerminatedBlock

thakis edited the summary of this revision. (Show Details)Aug 21 2017, 10:42 AM
thakis added inline comments.
lib/Analysis/CFG.cpp
448

(this is now a no-op and only a clang-formatting of the existing ctor code. I can omit this if you want.)

thakis marked 2 inline comments as done.Aug 21 2017, 3:16 PM
rnk added a comment.Aug 22 2017, 2:56 PM

Looks functionally correct

test/Sema/warn-unreachable-ms.c
43

Can we add a test to exercise that this builds the right CFG?

__try {
  __try {
    f();
  } __except(1) {
    __leave; // should exit outer try
  }
  __leave;
  f(); // expected-warning{{never be executed}}
} __except(1) {
}

add test from rnk

rnk accepted this revision.Aug 22 2017, 6:23 PM

Looks good!

test/Sema/warn-unreachable-ms.c
43

Sure. Did you intentionally put two leaves in there, or do you only want the one in the inner except?

I think both are required to trigger the warning in case f() doesn't throw, but I could be wrong.

This revision is now accepted and ready to land.Aug 22 2017, 6:23 PM
thakis closed this revision.Aug 23 2017, 8:34 AM
rnk added inline comments.Aug 23 2017, 9:10 AM
test/Sema/warn-unreachable-ms.c
43

I woke up this morning and realized what you meant. Is there another way we can check that leave in except exits the outer __try?