Page MenuHomePhabricator

[analyzer] NFC: CallDescription: Implement describing C library functions.
ClosedPublic

Authored by NoQ on May 28 2019, 4:37 PM.

Details

Summary

When matching C standard library functions in the checker, it's easy to forget that they are often implemented as macros that are expanded to compiler builtins. Such builtins would have a different name, so matching the callee identifier would fail, or may sometimes have more arguments than expected (so matching the exact number of arguments would fail, but this is fine as long as we have all the arguments that we need in their respective places.

This patch adds a set of flags to the CallDescription class so that to handle various special matching rules, and adds the first flag into this set, which enables a more fuzzy matching for functions that may be implemented as builtins.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.May 28 2019, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 4:38 PM

Hi Artem,
Looks mostly good, but I have some comments inline.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1064 ↗(On Diff #201785)

Not for this review, but why do we have a vector of const char *, not StringRefs?

1066 ↗(On Diff #201785)

Is it a good idea to make Flags a bitfield structure?

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
379 ↗(On Diff #201785)

!II is never false due to the newly-introduced early return.

Szelethus added a comment.EditedJun 4 2019, 5:51 PM

I like the idea, but will need more time to get familiar with CallEvent to formally accept.

When matching C standard library functions in the checker, it's easy to forget that they are often implemented as macros that are expanded to compiler builtins. Such builtins would have a different name, so matching the callee identifier would fail, or may sometimes have more arguments than expected (so matching the exact number of arguments would fail, but this is fine as long as we have all the arguments that we need in their respective places.

Just a name an issue that came up recently, in the case of D59812, we probably shouldn't need to strip "__builtin_" off.

xazax.hun accepted this revision.Jun 5 2019, 1:46 AM

Once Aleksei's comments are resolved it is good to go. My comments are notes and not requests.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1052 ↗(On Diff #201785)

I wonder if this is the right name. Do checker authors know the relationship between a standard library function and builtins? Maybe something like CDF_IsCStandardLibFunc would be more prescriptive when to set this? Or do we have non standard library builtins that we might want to match using this facility? The comment above is great, I just wonder if checker authors will read that. In case it does show up in the doxygen feel free to leave the name as is.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
368 ↗(On Diff #201785)

In case this happens to be a performance problem we could cache the builtin id in the future. This is just a note, do not need to optimize this prematurely.

This revision is now accepted and ready to land.Jun 5 2019, 1:46 AM
NoQ updated this revision to Diff 206978.Jun 27 2019, 5:44 PM
NoQ marked 4 inline comments as done.

Fxd.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1064 ↗(On Diff #201785)

The constructor accepts ArrayRef<const char *> rather than ArrayRef<StringRef> because { "foo", "bar" } is not a valid initializer for ArrayRef<StringRef>. The field is the same for simplicity, as converting ArrayRef<const char *> to a vector<StringRef> is annoying and not really useful. See also D51390.

1066 ↗(On Diff #201785)

This would prevent us from passing the flags into the constructor as CDF_Foo | CDF_Bar etc.- we'll have to specify every field on every line. I wanted to make the initializers as concise as possible to avoid line breaks in code like D62557.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
379 ↗(On Diff #201785)

Right!

Charusso accepted this revision.Jul 1 2019, 10:15 AM

I have not seen any problem. Thanks!

NoQ updated this revision to Diff 207406.Jul 1 2019, 2:08 PM

Rebase!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 4:03 PM