[analyzer] Handle forwarding reference better in ExprMutationAnalyzer.
ClosedPublic

Authored by shuaiwang on Wed, Sep 12, 5:45 PM.

Details

Summary

We used to treat an Expr mutated whenever it's passed as non-const
reference argument to a function. This results in false positives in
cases like this:

int x;
std::vector<int> v;
v.emplace_back(x); // `x` is passed as non-const reference to `emplace_back`

In theory the false positives can be suppressed with
v.emplace_back(std::as_const(x)) but that's considered overly verbose,
inconsistent with existing code and spammy as diags.

This diff is first step in handling such cases by following into the function definition
and see whether the argument is mutated inside.

Diff Detail

Repository
rL LLVM
shuaiwang created this revision.Wed, Sep 12, 5:45 PM

Thanks for working on this! I tried, and it appears to not fix the issue at hand.

struct C1 {
  C1(const C1* c, int num);
};

int x = 0;
auto y = std::make_unique<C1>(nullptr, x); // <- still considered a mutation?
struct C3 {}; // some class

struct C2 {
  C2(const int* whatever, int n, C3 zz);
};

int x = 0;
std::vector<C2> v;
v.emplace_back(nullptr, x, {}); // <- still considered a mutation?

And so on. These are hand-reduced, so hopefully you can reproduce?

include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
32 ↗(On Diff #165189)

Thanks!
I know this has performance implications, but those will exist even if one has this in his own code.

The new functionality looks very good. It can be used in a readability check that suggests const for parameters.

include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
32 ↗(On Diff #165189)

The analysis itself should not be executed twice, as it is cached. Only the way to figuring out that its cached will be run. I think its acceptable.

60 ↗(On Diff #165189)

Why do we need a separate class for this?
I think you can just create another object of ExprMutAnalyzer and do the analysis with findDeclMutation(FunctioParm).

The Stmt is the functionDecl().getBody(). Right now it could be that you pass in an functionDecl without body.

Could there something happen with extern templates that the body is not accessible?

lib/Analysis/ExprMutationAnalyzer.cpp
201 ↗(On Diff #165189)

I think this will not all cases correctly.

Say

template <typename T>
struct Foo {
  static void bar() { SomethingWith(T()); }
};

bar it self is not a template and NotInstantiated will be false (only 90% sure on that) as the declaration of bar does not match.
In another check I had to do this: unless(has(expr(anyOf(isTypeDependent(), isValueDependent())))) to ensure that there are no unresolved constructs in the stmt.

318 ↗(On Diff #165189)

Same instantiation concerns

367 ↗(On Diff #165189)

Just to be sure, this will recurse ?

struct Foo {
  std::string s;
  Foo(std::string s) : s (std::move(s)) {}
};

std::move will be resolved through the new mechanism right?

unittests/Analysis/ExprMutationAnalyzerTest.cpp
625 ↗(On Diff #165189)

Please add a Results(declRefTo("y"), notMutated(y), same above

659 ↗(On Diff #165189)

There are tests missing for the constructors. Please ensure that std::move and std::forward are handled properly.

shuaiwang edited the summary of this revision. (Show Details)Thu, Sep 13, 10:30 AM

Just some quick comments, I'll take a deeper look into other comments later.

This diff along unfortunately won't be able to handle emplace_back just yet, the reason (I believe, haven't fully tested) is that std::forward is not handled properly and almost all std functions involving forwarding references at least std::forward'ed once.
I have some more changes locally that treats std::move and std::forward just as casts and that should be able to really push the analysis further down the forwarding chain instead of stopping at std::forward call.
Rephased diff description to be more clear. Sorry for the confusion.

include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
60 ↗(On Diff #165189)

It's mostly for the special handling of constructors in which case initializer list also could mutate param.

Just some quick comments, I'll take a deeper look into other comments later.

This diff along unfortunately won't be able to handle emplace_back just yet, the reason (I believe, haven't fully tested) is that std::forward is not handled properly and almost all std functions involving forwarding references at least std::forward'ed once.
I have some more changes locally that treats std::move and std::forward just as casts and that should be able to really push the analysis further down the forwarding chain instead of stopping at std::forward call.
Rephased diff description to be more clear. Sorry for the confusion.

I did look up the rules for resolved the universal references.

void f() {
  int x;
  std::move(x);
}

Will create a normal reference to x. This would be the point were have to recursivly follow all references created from a value and check if they are mutated. Do you see another possibility on handling universal references correctly?

include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
60 ↗(On Diff #165189)

Hmm, still why not within ExprMutAnalyzer?
You could make it a class living within ExprMutAnalyzer in the private section. Then its clear its an implementation detail.

Just some quick comments, I'll take a deeper look into other comments later.

This diff along unfortunately won't be able to handle emplace_back just yet

My apologies, for some reason i though it was supposed to.
Once again, thank you for working on this!

, the reason (I believe, haven't fully tested) is that std::forward is not handled properly and almost all std functions involving forwarding references at least std::forward'ed once.
I have some more changes locally that treats std::move and std::forward just as casts and that should be able to really push the analysis further down the forwarding chain instead of stopping at std::forward call.

Rephased diff description to be more clear. Sorry for the confusion.

No problem!

shuaiwang updated this revision to Diff 165420.Thu, Sep 13, 9:55 PM
shuaiwang marked 10 inline comments as done.

More test cases addressing review comments

Thanks for working on this! I tried, and it appears to not fix the issue at hand.

  • ` struct C1 { C1(const C1* c, int num); };

    int x = 0; auto y = std::make_unique<C1>(nullptr, x); <- still considered a mutation? ` * ` struct C3 {}; some class

    struct C2 { C2(const int* whatever, int n, C3 zz); };

    int x = 0; std::vector<C2> v; v.emplace_back(nullptr, x, {}); // <- still considered a mutation? `

    And so on. These are hand-reduced, so hopefully you can reproduce?

I've patched D51870 and tried these cases, they work correctly with this change plus the change making std::move & std::forward considered casts. I also tested

struct D {
  D(int&);
};
void h() {
  std::vector<D> v;
  for (int i = 0; i < 10; ++i) {
    v.emplace_back(i);
  }
}

and that's also correctly considered as mutation at emplace_back

include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
60 ↗(On Diff #165189)

Oh actually I'm planning to use this also in UnnecessaryValueParamCheck

lib/Analysis/ExprMutationAnalyzer.cpp
201 ↗(On Diff #165189)

I think it's fine here since we only care about skipping those with forwarding references.

367 ↗(On Diff #165189)

This diff along won't handle std::move "properly" yet.
We'll look into definition of std::move, it'll be something like

return static_cast<remove_reference<T>::type&&>(t);

and we'll simply conclude t is mutated: because it's being returned.

So for you case, we'll see s is mutated by std::move(s) and stop there, when we treat std::move as cast, we'll go one step further and found std::move(s) is passed as non-const argument to constructor of std::string, and we'll stop there concluding s is mutated.

The std::move as cast is a follow up patch?

From my side only the nits are left.

lib/Analysis/ExprMutationAnalyzer.cpp
333 ↗(On Diff #165420)

Please spell out the type here

339 ↗(On Diff #165420)

could T be a const pointer?

344 ↗(On Diff #165420)

could RefType be a const pointer?

347 ↗(On Diff #165420)

Please spell out the type here

381 ↗(On Diff #165420)

Please spell out the type here

This looks very useful!

shuaiwang marked 8 inline comments as done.

Addressed review comments.

The std::move as cast is a follow up patch?

Yes I'll send a follow up patch.

lib/Analysis/ExprMutationAnalyzer.cpp
381 ↗(On Diff #165420)

This type is a bit cumbersome to spell out as it's an iterator. I feel it's fine to keep it auto.

JonasToth accepted this revision.Fri, Sep 14, 12:29 PM

LGTM

lib/Analysis/ExprMutationAnalyzer.cpp
381 ↗(On Diff #165420)

Oh true, iterator can be autoed I would say ;)

This revision is now accepted and ready to land.Fri, Sep 14, 12:29 PM
This revision was automatically updated to reflect the committed changes.