Page MenuHomePhabricator

[analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
ClosedPublic

Authored by rnkovacs on Jul 25 2018, 11:38 AM.

Details

Summary

The CoreEngine only gives us a ReturnStmt if the last element in the CFGBlock is a CFGStmt, otherwise the ReturnStmt is nullptr.
This patch adds support for the case when the last element is a CFGAutomaticObjDtor, by returning its TriggerStmt as a ReturnStmt.

Diff Detail

Repository
rC Clang

Event Timeline

rnkovacs created this revision.Jul 25 2018, 11:38 AM

I'm not sure how to test this.
I'll need it in D49361 when I update it to use the changed checkEndFunction() callback, and that will kind of test this too.

NoQ accepted this revision.Jul 25 2018, 1:14 PM

Yet another great catch!

I guess you could write a test with debug.AnalysisOrder (by making its checkEndFunction callback (that you'll have to define) print different things depending on the return statement), not sure if it's worth it; you can also merge this commit with D49361 instead.

This revision is now accepted and ready to land.Jul 25 2018, 1:14 PM
NoQ added a comment.Jul 25 2018, 3:38 PM

Devin has recently pointed out that we might have as well reordered CFG elements to have return statement kick in after automatic destructors, so that callbacks were called in a different order. I don't see any problems with that solution, but let's stick to the current solution for now, because who knows.

rnkovacs updated this revision to Diff 158680.Aug 1 2018, 7:57 PM
In D49811#1175726, @NoQ wrote:

I guess you could write a test with debug.AnalysisOrder (by making its checkEndFunction callback (that you'll have to define) print different things depending on the return statement), not sure if it's worth it; you can also merge this commit with D49361 instead.

This is what I could come up with, what do you think?

xazax.hun accepted this revision.Aug 2 2018, 10:51 AM

We could also print something about the ReturnStmt, like source location or pretty print of its expressions so we can check that we picked the right one in case we have multiple.
But consider this as an optional task if you have nothing better to do. I am ok with committing this as is.

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Aug 21 2018, 10:56 AM
In D49811#1175927, @NoQ wrote:

Devin has recently pointed out that we might have as well reordered CFG elements to have return statement kick in after automatic destructors, so that callbacks were called in a different order. I don't see any problems with that solution, but let's stick to the current solution for now, because who knows.

If we treat an occurrence of "return" in the CFG is meaning "bind an expression result for the return value" and not as a transfer of control then it is is fine for the destructors to appear after the "return". From this view, the transfer back to the caller is when we hit the Exit block.

Another possible solution is to check in the destructor if the newly dangling pointer is already bound to the return statement.