This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer
ClosedPublic

Authored by shuaiwang on Aug 12 2018, 6:56 PM.

Event Timeline

shuaiwang created this revision.Aug 12 2018, 6:56 PM

Please add a test for the same usecase as in Sema.

template <typename T>
struct SizeIndicator {
  constexpr int value = 8;
};
template <>
struct SizeIndicator<int> {
  constexpr int value = 4;
};
template <typename Foo>
void fooFunction() {
  char Characters[SizeIndicator<Foo>::value];
  void ArrayToPointerDecay(Characters);
}

template <>
void fooFunction<int>() {
  char Character[SizeIndicator<int>::value];
  void ArrayToPointerDecay(Characters);
}

In both cases the mutation must be detected. I assume the function specialization is diagnosed correctly, because everything is resolved.

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
485

Is there already a test for a method from a templated type?

E.g.

template <typename T>
struct Foo {
  T x;
  Foo() { int local_int; x(local_int); }
};

One can not say that local_int could be const or not.

485

x.method(local_int); would be interesting, just for security, given that the first case is an operator overload and the second one a classical method call.

shuaiwang updated this revision to Diff 160694.Aug 14 2018, 2:35 PM
shuaiwang marked 2 inline comments as done.
  • Fix a few cases overlooked previously
  • More test cases
JonasToth added inline comments.Aug 15 2018, 1:01 AM
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
117

I think you could add another test with X<T> x (if you don't have it already and I overlooked them)

309

I think we are missing tests for non-type template paramters (template <size_t N>). They should behave the same. But the following case would not be a mutation:

void non_mutating(const char* Array, int size) { /* Foo */ }
template <int N>
struct ArrayLike {
  char* data[N]; // Initialized to something
  void SomeFunction() {
    non_mutating(data, N);
  }
};

The difference between the 'normal' and non-type templates would be, that N is not mutatable at all and the semantics is clear (builtin integer-like type).

If the current implementation would not figure that out, you can just add a test for it and assume a mutation. Handling non-type templates later is absolutly ok.

410

Out of curiosity: Why is the result with y different from the result for x? Both time x is mutated and g() mutates them.

shuaiwang added inline comments.Aug 15 2018, 10:25 AM
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
410

This is ultimately caused by not handling pointers yet.
As soon as the address of an object is taken we assume the object is mutated.
e.g.

void f(const int*);
void g() {
  int x;
  f(&x); // <-- address of x taken, assume mutation
  int y[10];
  f(y); // <-- address of y taken, assume mutation
}

And in all such cases the "mutated by" expression is the expression that takes the address.

So back in this case, g(x) mutates x because we're assuming g mutates its argument through non-const reference. Note that the declared g might not be the one actually being called because of overload resolution, there could be another void g(char(&)[8])
While for g(y) we know it's calling the void g(char*) so there's an array to pointer decay, and the decay is the point we assumed mutation not the function call.

shuaiwang updated this revision to Diff 160952.Aug 15 2018, 5:19 PM
shuaiwang marked 3 inline comments as done.

More test cases

shuaiwang added inline comments.Aug 15 2018, 5:41 PM
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
309

We have to assume data is mutated here as well. I'll add a test case for this.

void g(const char*, int); // <-- doesn't mutate
void g(char(&)[8], int); // <-- do mutate

template <int N>
void f() {
    char data[N];
    g(data, N); // <-- we don't know which `g` will be called yet
}

void h() {
    f<8>(); // <-- f calls g(char(&)[8], int) internally
    f<9>(); // <-- f calls g(const char*, int) internally
}
shuaiwang updated this revision to Diff 160960.Aug 15 2018, 5:45 PM

Test case with non-type template

shuaiwang marked 2 inline comments as done.Aug 15 2018, 5:46 PM

I see, thank you for the clarification :)

Am 15.08.2018 um 19:25 schrieb Shuai Wang via Phabricator:

shuaiwang added inline comments.

Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:410
+ match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));

+}

JonasToth wrote:

Out of curiosity: Why is the result with y different from the result for x? Both time x is mutated and g() mutates them.

This is ultimately caused by not handling pointers yet.
As soon as the address of an object is taken we assume the object is mutated.
e.g.

void f(const int*);
void g() {
  int x;
  f(&x); // <-- address of x taken, assume mutation
  int y[10];
  f(y); // <-- address of y taken, assume mutation
}

And in all such cases the "mutated by" expression is the expression that takes the address.

So back in this case, g(x) mutates x because we're assuming g mutates its argument through non-const reference. Note that the declared g might not be the one actually being called because of overload resolution, there could be another void g(char(&)[8])
While for g(y) we know it's calling the void g(char*) so there's an array to pointer decay, and the decay is the point we assumed mutation not the function call.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

You are totally right.

Am 16.08.2018 um 02:41 schrieb Shuai Wang via Phabricator:

shuaiwang added inline comments.

Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:309

+TEST(ExprMutationAnalyzerTest, CallUnresolved) {

+ auto AST =

JonasToth wrote:

I think we are missing tests for non-type template paramters (template <size_t N>). They should behave the same. But the following case would not be a mutation:

void non_mutating(const char* Array, int size) { /* Foo */ }
template <int N>
struct ArrayLike {
  char* data[N]; // Initialized to something
  void SomeFunction() {
    non_mutating(data, N);
  }
};

The difference between the 'normal' and non-type templates would be, that N is not mutatable at all and the semantics is clear (builtin integer-like type).

If the current implementation would not figure that out, you can just add a test for it and assume a mutation. Handling non-type templates later is absolutly ok.

We have to assume data is mutated here as well. I'll add a test case for this.

void g(const char*, int); // <-- doesn't mutate
void g(char(&)[8], int); // <-- do mutate

template <int N>
void f() {
    char data[N];
    g(data, N); // <-- we don't know which `g` will be called yet
}

void h() {
    f<8>(); // <-- f calls g(char(&)[8], int) internally
    f<9>(); // <-- f calls g(const char*, int) internally
}

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

@shuaiwang i tried to apply this and check the clang-tidy part again, but it does not compile (log attached).
I update clang to master, did you add a matcher or something like this?

@shuaiwang i tried to apply this and check the clang-tidy part again, but it does not compile (log attached).
I update clang to master, did you add a matcher or something like this?

Oops, my bad, this depends on D50617

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

→ ~/opt/llvm/build/bin/clang-tidy -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* clang::CXXDependentScopeMemberExpr::getBase() const: Assertion `!isImplicitAccess()' failed.
Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

→ ~/opt/llvm/build/bin/clang-tidy -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* clang::CXXDependentScopeMemberExpr::getBase() const: Assertion `!isImplicitAccess()' failed.
Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.

D50617 updated, could you help try again? Thanks!

Sure, thanks for the fast fix :)

Am 22.08.2018 um 00:04 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D50619#1207785, @JonasToth wrote:

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

→ ~/opt/llvm/build/bin/clang-tidy -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* clang::CXXDependentScopeMemberExpr::getBase() const: Assertion `!isImplicitAccess()' failed.
Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.

https://reviews.llvm.org/D50617 updated, could you help try again? Thanks!

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

JonasToth accepted this revision.Sep 10 2018, 4:23 AM

Ups, sorry i overlooked.

I applied your changes to the current version of the const check, and everything seems fine. The false negative is gone.

This revision is now accepted and ready to land.Sep 10 2018, 4:23 AM
This revision was automatically updated to reflect the committed changes.