This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MisusedMovedObjectChecker: Fix a false positive on state-resetting a base-class sub-object.
ClosedPublic

Authored by NoQ on Mar 31 2017, 9:05 AM.

Diff Detail

Event Timeline

NoQ created this revision.Mar 31 2017, 9:05 AM
szepet edited edge metadata.Mar 31 2017, 10:32 AM

Thank you for working on that, Artem!
The changes look good, just one comment about that suspicious remove.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
426

I am wondering if I made a mistake but I think this should be removeFromState() function call. (We should remove every marked subregions of the object too.)
So I suspect a code like this would result a false positive:

struct A{
B b;
void clear();
};

void test(){
A a;
B b = std::move(a.b);
a.clear();
b = std::move(a); //report a bug
}

I mean it is probably a report we do not want to have.

szepet added inline comments.Mar 31 2017, 10:36 AM
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
426

Shame on the test file writer that he does not covered this, though. ^^

szepet accepted this revision.Apr 15 2017, 1:22 PM
szepet added inline comments.
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
426

Okay, I have checked it and yes, it produces a bugreport (false positive) on the code snippet above (which contains a misspelled variable name so let me write it again):

struct B{};

struct A{
B b;
void clear();
};

void test(){
A a;
B b = std::move(a.b);
a.clear();
b = std::move(a.b); //report a bug
}

In order to eliminate these type of bugs I suggest using here the removeFromState function and move this "WholeObjectRegion algorithm" (the for loop) to the removeFromState function.
It is probably out of scope from this patch. Anyway, I accept this since it is great to fix this bug and a good step to a follow-up patch. (Well, if you would like to make these changes in this patch. I'm not gonna hold you back. It is highly appreciated and I would certainly review and all the stuff but I would do it gladly in a follow-up too.)
Again, thank you for pointing this out and making the checker more precise!

This revision is now accepted and ready to land.Apr 15 2017, 1:22 PM
szepet added a comment.Oct 6 2017, 2:34 AM

Hello Artem!

Could you please commit these changes? (And D31541 as well.) Thanks in advance!

xazax.hun accepted this revision.Oct 9 2017, 6:14 AM

I think there was only one comment but that is already addressed in a dependent revision. So I think this one is good as is.

This revision was automatically updated to reflect the committed changes.