This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Evaluate PredefinedExpressions
ClosedPublic

Authored by steakhal on Sep 2 2020, 2:30 AM.

Details

Summary

We did not evaluate such expressions, just returned unknown for such cases.
After this patch, we will be able to access a unique value identifying a template instantiation.

Diff Detail

Event Timeline

steakhal created this revision.Sep 2 2020, 2:30 AM
steakhal requested review of this revision.Sep 2 2020, 2:30 AM
Szelethus accepted this revision.Sep 2 2020, 2:51 AM

LGTM! Nice!

clang/test/Analysis/eval-predefined-exprs.cpp
2

Isn't it -std=c++17?

8–22

Why not put the expected warning right below the function call?

This revision is now accepted and ready to land.Sep 2 2020, 2:51 AM
vsavchenko requested changes to this revision.Sep 2 2020, 3:39 AM
vsavchenko added inline comments.
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
311

I'm not super familiar with predefined expressions, but it looks like it this commit doesn't cover all kinds of those. And for some this can return nullptr.

Can you, please, add tests for other kinds as well?

This revision now requires changes to proceed.Sep 2 2020, 3:39 AM
steakhal updated this revision to Diff 289395.Sep 2 2020, 4:43 AM
steakhal added subscribers: riccibruno, rjmccall.
  • Added tests for Microsoft extensions.
  • Added an assert requiring the PredefinedExpression to have a function name.

I don't know how could a PredefinedExpression lack the function name, probably @riccibruno or @rjmccall can help with this - according to D53605.

steakhal marked 2 inline comments as done.Sep 2 2020, 4:45 AM
steakhal added inline comments.
clang/test/Analysis/eval-predefined-exprs.cpp
2

Thanks, typo.

8–22

That would seriously violate the column limit.
This way it is easier to see and validate the pattern IMO.

martong added inline comments.Sep 2 2020, 5:35 AM
clang/test/Analysis/eval-predefined-exprs.cpp
8–22

This could be like:

clang_analyzer_dump(__FUNCDNAME__); // \
// expected-warning@-4 {{&Element{"??$func@UClass@?1??foo@@YAXXZ@$0CK@D@@YAXD@Z",0 S64b,char}}}
clang_analyzer_dump(L__FUNCTION__); // \
// expected-warning@-4 {{&Element{L"func",0 S64b,wchar_t}}}
...

So, this way you can keep the line limit, I think this is what @Szelethus refers to.

martong added inline comments.Sep 2 2020, 5:38 AM
clang/test/Analysis/eval-predefined-exprs.cpp
8–22

This could be like: ...

There is no need for the @-4 of course with that approach.

I don't know how could a PredefinedExpression lack the function name, probably @riccibruno or @rjmccall can help with this - according to D53605.

A PredefinedExpr whose declaration context is dependent has no name (see Sema::BuildPredefinedExpr). Example:

void f0() { __func__; }
-PredefinedExpr 0x6a76ba8 <col:13> 'const char [3]' lvalue __func__
`-StringLiteral 0x6a76b88 <col:13> 'const char [3]' lvalue "f0"

template <typename> void f1() { __func__; }
`-PredefinedExpr 0x6a76e78 <col:35> '<dependent type>' lvalue __func__

template void f1<int>();
-PredefinedExpr 0x6a77060 <col:35> 'const char [3]' lvalue __func__
`-StringLiteral 0x6a77040 <col:35> 'const char [3]' lvalue "f1"
clang/test/Analysis/eval-predefined-exprs.cpp
8–22

There is no column limit in test/.

steakhal updated this revision to Diff 289432.Sep 2 2020, 6:30 AM
steakhal marked 2 inline comments as done.

We only analyze instantiated functions, which are not dependently typed.
Safe to assume that every encountered PredefinedExpression has a defined (non-null) function name.

Just to be sure I added a clang_analyzer_warnIfReached in a test-case.

clang/test/Analysis/eval-predefined-exprs.cpp
8–22

Do you think it would be more readable that way? I'm still not convinced.

Awesome!
Can we cover __builtin_unique_stable_name as well?

vsavchenko added inline comments.Sep 2 2020, 6:40 AM
clang/test/Analysis/eval-predefined-exprs.cpp
102

Also can you please add // no-warning as a matter of conventions?

riccibruno added inline comments.Sep 2 2020, 6:41 AM
clang/test/Analysis/eval-predefined-exprs.cpp
8–22

I have no opinion on whether it is more readable. I was just saying that test/ is an exception to the column limit policy, and indeed that *many* tests have lines longer that the limit.

steakhal updated this revision to Diff 289446.Sep 2 2020, 7:50 AM
  • Added no-warning.
  • Added test-case for __builtin_unique_stable_name as well.
steakhal marked an inline comment as done.Sep 2 2020, 7:51 AM

Thank you all for the comments!

clang/test/Analysis/eval-predefined-exprs.cpp
102

Sure, thanks.

vsavchenko accepted this revision.Sep 2 2020, 7:53 AM

Thanks!
Great job!

This revision is now accepted and ready to land.Sep 2 2020, 7:53 AM
This revision was landed with ongoing or failed builds.Sep 13 2020, 11:44 PM
This revision was automatically updated to reflect the committed changes.
steakhal marked an inline comment as done.