This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors
Needs ReviewPublic

Authored by baloghadamsoftware on Jul 14 2016, 11:28 AM.

Details

Reviewers
dcoughlin
NoQ
Summary

Many classes (e.g. in common stl implementations) contain user-written copy and move constructors that are identical to the implicit ones. This far ExprEngine could only handle the official "trivial" case. This patch extends ExprEngine for copy and move constructors that are user provided but in all other aspects they are the same as trivial ones.

A typical example is the GNU implementation of the iterator of std::deque, where the user provided copy constructor of the non-const version also accepts an instance of the const version as parameter. This patch allows correct simulation of this iterator.

Diff Detail

Event Timeline

baloghadamsoftware retitled this revision from to [analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors.
baloghadamsoftware updated this object.
baloghadamsoftware added a reviewer: dcoughlin.
xazax.hun added a comment.EditedJul 15 2016, 4:33 AM

Note that, once a constructor body is not available, we will conservatively treat it as nontrivial. This is not the case however for most of the templated code. Since the STL use templates heavily I think this patch is a great step forward in improving the modeling of C++ code.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
37

These two functions only used in this translation unit right? Maybe it would be better to make them static.

57

nit: need spaced around the operator.

63

Isn't this only applicable to ctors? Maybe you should reflect this in the name or change the type of the parameter accordingly.

67

Please document that, in case we do not see the body of a ctor, we treat it conservatively as non trivial. (hasTrivialBody returns false when the body is not available)

88

Maybe you could reduce the indentation if you use "early continue" here.

109

What types do we want to exclude and why? It might be a good idea to document them.

112

Instead of the O(n^2) algorithm, it might be better to just iterate through the initializers, and put each field into a llvm small pointer set, or something like that. Afterwards you can iterate through the fields of the struct and check whether each field is inside the set. This might be both more efficient and cleaner.

Revised version based on comments.

baloghadamsoftware marked 6 inline comments as done.Jul 15 2016, 9:34 AM
a.sidorin added inline comments.Jul 18 2016, 2:44 AM
test/Analysis/ctor.mm
177

Seems like we may lose some warnings due to this change. Did you test this change on some real code?

test/Analysis/ctor.mm
177

Sorry, but no. We got rid of a false positive (debug checker?). The member of the new object are no longer undefined since we do a trivial copy here because this copy constructor is "almost trivial".

Adam,

It is not a debug checker. It is UndefinedAssignment checker which correctly tells us that Inner.y is assigned with an uninitialized value while copying. So I wonder if we are allowed to skip such warnings because these warnings don't look like false positives.
As I understand, these warnings disappeared because performTrivialCopy() doesn't run checkers for check::Bindevent. Not sure but maybe we should fix that?

NoQ added a subscriber: NoQ.Jul 20 2016, 12:59 PM

Now I made a thorough check. Indeed, with the original version we get a warning because Other.y is not initialized. CheckerManager::runCheckersForBind() is called here during the default evaluation of the call. I tried to call the same function in performTrivialCopy, but without success, all members on the right hand side object are "Unknown" so no warnings are given. It seems like arguments are not bound to paremeters without executing evalCall(). I tried to copy some part of inlineCall(), but it did not help either. It seems evaluation of bindings in the constructor initializer list is strongly connected to its default evaluation.

NoQ added a comment.Jul 27 2016, 5:59 AM

I see what's going on.

performTrivialCopy() already calls evalBind(), which in turn calls runCheckersForBind(), so no effort is needed there.

However, the bind event itself is now different - instead of a separate event for every bind, you're having only one event for binding the nonloc::LazyCompoundVal.

For example, in testNonPODWrapper (which calls the test on line 177), you used to have one bind of an UndefinedVal into w2.p.x (which produced the warning) and one bind of 1 S32b into w2.p.y, which didn't produce the warning. But now you're having just one bind event of lazyCompoundVal{0x7fb5b80ef3b8,w} into w2. By unpacking the compound value's store, you could see that w.p.y in it contains an undefined value (though you wouldn't see it in the store's dump directly, the StoreManager would produce one for you when you ask for it).

At a glance, this might be worth fixing on the checker's side. Essentially, that might still be a problem when truly-trivial (as opposed to almost-trivial) copies are performed, so i guess the checker might need fixing regardless.

Dropping such warnings is also a viable option. We can think of this warning as false, because perhaps the purpose of this code was to ensure that at least w2.p.y is initialized.

But if w was completely unitialized (that is, LCV contained only uninitialized values in its store in the cluster defined by its parent region), then the warning should be useful. So i'm also having this feature request for all Undefined* checkers - perhaps not only consider straightforward undefined values, but also passing uninitialized structures by value or by const pointer/reference?

I agree with you. Do I have to modify the checker (in a separate patch), or someone else can do it? I do not know how difficult it is to unpack the store of a LazyCompoundVal (it probably has to be done recursively).

NoQ added a comment.Aug 3 2016, 9:34 AM

Hmm. I suggest:

  1. Change this test's constructor so that it was no longer almost-trivial. Because it isn't significant for this test if the constructor is almost-trivial or not. The test would pass.
  1. Add a FIXME-test for this checker, in which a completely undefined structure is being copied. Perhaps somebody implements this feature some day.
  1. Leave out the situation that the current test checks as a grey-area. I'm still not convinced that this situation (copying a partially-initialized almost-trivial structure) deserves a warning from this checker.

Does this make any sense? :)

In D22374#504855, @NoQ wrote:

Hmm. I suggest:

  1. Change this test's constructor so that it was no longer almost-trivial. Because it isn't significant for this test if the constructor is almost-trivial or not. The test would pass.

It is significant, because I want to test here the almost-trivial constructors.

  1. Add a FIXME-test for this checker, in which a completely undefined structure is being copied. Perhaps somebody implements this feature some day.

So it would be a new function with the same structure, and commented out beginning with a FIXME?

  1. Leave out the situation that the current test checks as a grey-area. I'm still not convinced that this situation (copying a partially-initialized almost-trivial structure) deserves a warning from this checker.

This is what I originally did here in the patch. Maybe a FIXME could be written there a well?

NoQ added a comment.Aug 4 2016, 12:23 PM
In D22374#504855, @NoQ wrote:

Hmm. I suggest:

  1. Change this test's constructor so that it was no longer almost-trivial. Because it isn't significant for this test if the constructor is almost-trivial or not. The test would pass.

It is significant, because I want to test here the almost-trivial constructors.

I mean, the positive in Inner::Inner(), which has regressed, was written long before the notion of "almost-trivial constructor" was introduced by you. So it shouldn't be a problem if we modify this test to let the constructor become non-almost-trivial; that wasn't the purpose of that particular test.

And we could create another test with almost-trivial constructor elsewhere, which would prove the usefulness of your patch.

  1. Add a FIXME-test for this checker, in which a completely undefined structure is being copied. Perhaps somebody implements this feature some day.

So it would be a new function with the same structure, and commented out beginning with a FIXME?

Yeah, that's what i thought - not completely commented out, just having an opposite expected-warning <=> no-warning, and a "FIXME: we should (not) warn here because *some hand-waving*". There are a few such tests around.

  1. Leave out the situation that the current test checks as a grey-area. I'm still not convinced that this situation (copying a partially-initialized almost-trivial structure) deserves a warning from this checker.

This is what I originally did here in the patch. Maybe a FIXME could be written there a well?

I think this test should still test what it used to test - i've a feeling it's ok if we modify the part of it that was not related to what it used to test, but i'm feeling bad about removing it completely.

Essentially, this plan of mine has the sole purpose of keeping my (and perhaps somebody else's) conscience from biting me for removing the test; it seems that simpler approaches seem to carry more of the "no-no, this shouldn't happen" feeling. I guess i could post a patch-over-a-patch if what i'm expressing isn't clear.

In D22374#506098, @NoQ wrote:

I guess i could post a patch-over-a-patch if what i'm expressing isn't clear.

I think this would be the best :-)

NoQ added a comment.Aug 5 2016, 8:04 AM

Something like this, what do you think?:

NoQ added a reviewer: zaks.anna.EditedOct 20 2016, 10:57 AM

Ping!~ Did my idea sound completely wrong to you? :)

Does D25660 depend on this patch? And/or did you find another workaround?

upd.: I also thought that this deserves more code comments - the purpose of the new mechanism should be more obvious by reading the code.

In D22374#575579, @NoQ wrote:

Ping!~ Did my idea sound completely wrong to you? :)

Does D25660 depend on this patch? And/or did you find another workaround?

upd.: I also thought that this deserves more code comments - the purpose of the new mechanism should be more obvious by reading the code.

Sorry, since it turned out that D25660 does not depend on this patch I downprioritized it.

By some mistake I missed the notification about your patch-over-patch. It seems OK to me.

DO I have to apply your path over patch and update the diff?

baloghadamsoftware edited reviewers, added: NoQ; removed: zaks.anna.Jan 10 2018, 4:44 AM