Skip to content

C++: Support function calls throwing exceptions in the IR #15461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 31, 2024

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jan 29, 2024

This PR adds a new abstract class ThrowingFunction to model the exceptional behavior of a function. Specifically, to model that certain functions that may (or unconditionally) throw an exception one can implement the abstract class ThrowingFunction. For example, see the added models for the Windows structured exception handling functions (models/implementations/StructuredExceptionHandling.qll in this commit).

I wish the diff for this PR was just 1dfd32e, but unfortunately the current IR generation classes doesn't allow specifying an EdgeKind on the getChildSuccessor predicate 😭 (unlike getInstructionSuccessor, where we can specify this). So without adding this column we can't actually model that a call may have multiple successors (i.e., a "normal successor" and an "exceptional successor"). So there's a whole bunch of very boring commits that proceed this that actually propagate these EdgeKind columns around.

@github-actions github-actions bot added the C++ label Jan 29, 2024
@MathiasVP MathiasVP force-pushed the propagate-edge-kinds branch 2 times, most recently from ea8f05a to 508c918 Compare January 30, 2024 11:30
@MathiasVP MathiasVP force-pushed the propagate-edge-kinds branch from 508c918 to 33e3753 Compare January 30, 2024 11:38
@MathiasVP MathiasVP marked this pull request as ready for review January 30, 2024 11:40
@MathiasVP MathiasVP requested a review from a team as a code owner January 30, 2024 11:40
# 21| v21_10(void) = ExitFunction :

# 21| Block 5
# 21| v21_11(void) = Unreached :
Copy link
Contributor

@jketema jketema Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this Unreached because the exception would propagate?

It got eliminated because we have known constant values.

Copy link
Contributor Author

@MathiasVP MathiasVP Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Yes (after your edit 😂). There's an Unreached instruction here because we generate trivial comparisons that the IR deduces is unreachable when we implement the semantics of Microsoft's structured exception handling:

(because the condition in this case is just 1)

Comment on lines +811 to +837
# 26| Block 2
# 26| r26_1(int) = Constant[0] :
# 26| r26_2(bool) = CompareEQ : r26_8, r26_1
# 26| v26_3(void) = ConditionalBranch : r26_2
#-----| False -> Block 3
#-----| True -> Block 4

# 26| Block 3
# 26| r26_4(int) = Constant[1] :
# 26| r26_5(bool) = CompareEQ : r26_8, r26_4
# 26| v26_6(void) = ConditionalBranch : r26_5
#-----| True -> Block 6

# 26| Block 4
# 26| v26_7(void) = Unwind :
# 29| r29_1(glval<int>) = VariableAddress[#return] :
# 29| r29_2(int) = Constant[0] :
# 29| mu29_3(int) = Store[#return] : &:r29_1, r29_2
#-----| Goto -> Block 1

# 26| Block 5
# 26| r26_8(int) = Constant[1] :
# 26| r26_9(int) = Constant[-1] :
# 26| r26_10(bool) = CompareEQ : r26_8, r26_9
# 26| v26_11(void) = ConditionalBranch : r26_10
#-----| False -> Block 2
#-----| True -> Block 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No related to this PR (I think), but this looks somewhat odd. There's both unwinding in the case of -1 and 0, but reading https://learn.microsoft.com/en-us/cpp/cpp/try-except-statement, it seems there should be unwinding in the case of 0, while in the case of -1 the execution should continue at the place where the exception occurred. I don't we can model the latter properly at the moment though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, that's what this TODO is about:

// TODO: This is not really correct. The semantics of `EXCEPTION_CONTINUE_EXECUTION` is that

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but you might want to wait for @rdmarsh2 to also have a look?

@MathiasVP
Copy link
Contributor Author

LGTM, but you might want to wait for @rdmarsh2 to also have a look?

Thanks! Yeah, let's give @rdmarsh2 a chance to have a look as well

@rdmarsh2
Copy link
Contributor

I don't really understand the semantics of the EdgeKind in getFirstInstruction - is it the first instruction when the incoming edge is of kind EdgeKind, and when does that actually differ?

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Jan 30, 2024

I don't really understand the semantics of the EdgeKind in getFirstInstruction - is it the first instruction when the incoming edge is of kind EdgeKind, and when does that actually differ?

Sometimes getFirstInstruction calls this.getParent().getChildSuccessor(this) (for example here), and in that case we need to give some argument to getChildSuccessor since that one now needs a EdgeKind.

We could choose to just pass in any(GotoEdge kind) as an argument, but then we hardcode that any such case simply picks the GotoEdge. With this approach, we leave open the option of someone eventually passing in something other than GotoEdge.

If you prefer not having the extra argument on getFirstInstruction, we can also get rid of it and instead simply hardcode GotoEdge in the places where an argument is necessary

@rdmarsh2
Copy link
Contributor

No, that seems like a good reason to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants