This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Treat std::{move,forward} as casts in ExprMutationAnalyzer.
ClosedPublic

Authored by shuaiwang on Sep 14 2018, 1:14 PM.

Details

Summary

This is a follow up of D52008 and should make the analyzer being able to handle perfect forwardings in real world cases where forwardings are done through multiple layers of function calls with std::forward.

Fixes PR38891

Diff Detail

Repository
rC Clang

Event Timeline

shuaiwang created this revision.Sep 14 2018, 1:14 PM
shuaiwang edited the summary of this revision. (Show Details)Sep 14 2018, 1:16 PM

@lebedev.ri could you help test whether this fully resolves PR38891? Thanks!

@lebedev.ri could you help test whether this fully resolves PR38891? Thanks!

That is what i'm trying to do here :)
Still waiting for the build to finish...

lebedev.ri accepted this revision.Sep 14 2018, 2:00 PM

Thank you for working on this!

@lebedev.ri could you help test whether this fully resolves PR38891? Thanks!

That is what i'm trying to do here :)
Still waiting for the build to finish...

Ok, got the results.
Without that change, my check fires ~1k times.
With this change it only fires 572 times.
So this clearly changes things.
The entirety of the new build log:


I see no mentions of emplace_back/push_back.
Though some of that is still false-positives (pointers, va_arg, callback lambdas passed as templated function param), but i'll file further bugs i guess.

As far i'm concerned, this is good.
But please wait for the actual code review.

Once again, thank you for working on this!

This revision is now accepted and ready to land.Sep 14 2018, 2:00 PM

Though some of that is still false-positives (pointers, va_arg, callback lambdas passed as templated function param), but i'll file further bugs i guess.

Filed the ones that i can pinpoint, marked them as blockers of PR38890

Thank you very much for the good work from my side as well!

unittests/Analysis/ExprMutationAnalyzerTest.cpp
400–459

I am thinking about the correctness of the change.

If we have this case:

std::string s1 = "Hello World!";
std::string s2 = std::move(s1);

s1 gets mutated, but it looks like it would not be considered as mutation?

On the other hand

int i1 = 42;
int i2 = std::move(i1);

should resolve in a copy i1 and therefor not be a mutation.

Could you please add tests demonstrating this difference and the correct diagnostic detection for that.

Potentially interesting:

  • Builtin types should be Copy-Only?, builtin arrays as expensive to move types for the completeness please as well
  • Types with deleted move operations, should resolve in copy too
  • Move-Only types like unique_ptr
  • Types with both move and copy constructor (vector-like)
  • and something with everything defaulted

These thoughts are just thinking and writing, I just wondered that moving a variable with std::move is not considered as mutation here, even though there are cases were it is actually a mutation.

JonasToth added inline comments.Sep 15 2018, 5:46 AM
shuaiwang marked 2 inline comments as done.Sep 16 2018, 10:40 AM
shuaiwang added inline comments.
unittests/Analysis/ExprMutationAnalyzerTest.cpp
400–459

Added these tests:

  • std::move copy only type doesn't mutate
  • std::move move only type does mutate
  • std::move copiable & movable type does mutate
  • std::move const copiable & movable type doesn't mutate

Didn't test array because array can't be assigned (so copy/move isn't relevant) and passing array to function can only be done via reference (passing by value results in array to pointer decay) so no copy/move there as well.

shuaiwang marked an inline comment as done.

Added more test cases around std::move

JonasToth accepted this revision.Sep 17 2018, 3:17 AM

The last testcase i mentioned, after that LGTM

unittests/Analysis/ExprMutationAnalyzerTest.cpp
441

You could add a final test for

struct S {
  S();
  S(S s);
  S& operator(S s);
};

Note that the copy and assignment take by value, so should not resolve in a mutation by move.

Then i think the testsuite covers all potential variants

shuaiwang marked an inline comment as done.

Added test case with copy-ctor & assignment operator taking value as param.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.