This is an archive of the discontinued LLVM Phabricator instance.

[CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
ClosedPublic

Authored by NoQ on Jul 20 2018, 5:45 PM.

Details

Summary

Because CXXOperatorCallExpr's argument 0 is the this-argument of the operator if the operator is a member. This doesn't correspond to operator declaration parameters.

Do not provide argument construction context for such arguments. For the remaining arguments, provide a context, even though argument index still doesn't match parameter index; the user would, unfortunately, need to work around that, as we can't satisfy both.

Actually supporting such this-argument construction contexts would address the FIXME that we've added in D32642 but this patch doesn't go that far.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Jul 20 2018, 5:45 PM
NoQ edited the summary of this revision. (Show Details)Jul 20 2018, 5:46 PM
NoQ edited the summary of this revision. (Show Details)

I take it we are crashing otherwise?

include/clang/Analysis/ConstructionContext.h
632

optional nit: I think comments are more readable when there's a blank line before each comment block.

This revision is now accepted and ready to land.Jul 23 2018, 10:47 AM
NoQ updated this revision to Diff 156827.Jul 23 2018, 10:57 AM
NoQ marked an inline comment as done.

I take it we are crashing otherwise?

Not yet, but i'm about to commit code that would (D49443).

NoQ added a comment.Jul 23 2018, 11:00 AM

What i'm fixing here is a false detection of construction context of a certain kind. These are very bad in general, because they cause us to inline the constructor without knowing what object we're constructing or how to handle this object.

This comment was removed by baloghadamsoftware.

How much different is a correct this-argument construction context from real argument construction contexts?

NoQ added a comment.Jul 31 2018, 2:30 PM

How much different is a correct this-argument construction context from real argument construction contexts?

It's identified in a similar manner; this patch pretty much already shows how to identify it. It carries the same amount of information: what call, what argument.

The tricky part is to come up with a good description for the target region and make sure it's actually used when the operator call is inlined. It would probably be a simple CXXTempObjectRegion because we don't have a VarDecl to make a VarRegion.

Then we might need to proactively bind in in the Store to CXXThisRegion of the call, and for that purpose we might need a crystal ball to guess the future StackFrameContext, like in case of arguments.

We still need to invalidate the target region if operator call is conservatively evaluated, like in case of arguments, because the destructor will use the updated object.

NoQ added a comment.Jul 31 2018, 7:20 PM

Hmm, AST has just changed in rC338135. While the current patch is still relevant, it might be that we'll need to treat this construction context as if it's a simple temporary now.

NoQ updated this revision to Diff 158668.Aug 1 2018, 5:21 PM

Ok, so with rC338135 operator this-argument constructors do indeed work exactly like temporaries. This patch is not necessary anymore. I'd still prefer to add tests and i've added a path-sensitive test as well to demonstrate that the constructor works correctly.

This revision was automatically updated to reflect the committed changes.