Page MenuHomePhabricator

[Analyzer][WebKit] NoUncountedMembersChecker
ClosedPublic

Authored by jkorous on Mar 31 2020, 2:43 PM.

Diff Detail

Event Timeline

jkorous created this revision.Mar 31 2020, 2:43 PM
jkorous updated this revision to Diff 264796.May 18 2020, 10:31 PM

Updated to reflect feedback from review of the parent patch.

NoQ added inline comments.May 21 2020, 8:26 AM
clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
106–107

Did you intentionally make a stronger check than the usual SourceManager::isInSystemHeader()? If so i'm curious why - like, what's so special about this checker that requires this? Should we fix all of our other checkers? It must be something something module maps. I don't know much about those.

110

This looks unused.

111

As far as i'm aware, in C++ every RecordDecl is a CXXRecordDecl.

112

What's the motivation for skipping those?

129–134

Can we elaborate a bit on why it's bad? Even something like ... which is error-prone would be great.

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp
10

I found members:: fairly unexpected. Maybe simply Class::Member? The surrounding namespace is obvious from the location of the warning and doesn't seem to be worth dumping.

jkorous marked 9 inline comments as done.May 22 2020, 5:59 PM
jkorous added inline comments.
clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
106–107

No, there was no intention. I'll just use SourceManager::isInSystemHeader(). Thanks for pointing this out!

110

Indeed! Thanks

112

This checker enforces that there are no raw-pointers/references to uncounted types as member variable but since that's actually how WebKit smartpointers are implemented we need to skip those.
I'll add a comment.

129–134

Ok. Shall I add it to the checkers.rst or here?

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp
10

Ok.

jkorous updated this revision to Diff 265828.May 22 2020, 6:00 PM
jkorous marked 3 inline comments as done.
  • rebased
  • addressed some of the comments
NoQ added inline comments.May 26 2020, 9:38 AM
clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
129–134

Here. I'd prefer to avoid this situation where the user reads the warning and is like "ok so what :/".

jkorous marked an inline comment as done.May 26 2020, 11:43 AM
jkorous updated this revision to Diff 266286.May 26 2020, 11:46 AM
NoQ accepted this revision.May 27 2020, 3:19 AM

Perfect, thank you!

This revision is now accepted and ready to land.May 27 2020, 3:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 8:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein added a subscriber: hokein.Jun 1 2020, 11:46 PM

Hi, @jkorous

Thanks for working on this analyzer check.

Just give you some feedback on this check, it seems to trigger the assertion quite often (we noticed that internally there were a lot of crashes).

you can easily reproduce it by running it a LLVM file, e.g. ./bin/clang-tidy -checks="-*,clang-analyzer-webkit*" ../clang-tools-extra/clangd/ClangdLSPServer.cpp

NoQ added a comment.Jun 2 2020, 12:47 AM

Looks similar to https://bugs.llvm.org/show_bug.cgi?id=46142. It sounds pretty important to address quickly because it crashes on a fairly large portion of C++ code with default settings.