Page MenuHomePhabricator

[analyzer] Fix some checker's output plist not containing the checker name #2
ClosedPublic

Authored by xazax.hun on Dec 22 2017, 3:13 AM.

Details

Summary

Unfortunately, currently, the analyzer core sets the checker name after the constructor was already run. So if we set the BugType in the constructor, the output plist will not contain a checker name. Right now the idiomatic solution is to create the BugType lazily. This patch moves the lazy initialization from the checker side to the analyzer core. I also found some bugs, when some checkers were triggered even if they were not enabled or wrong checker name is emitted to the plist.

In the future probably it would be better to alter the signature of the checkers' constructor to set the name in the constructor so it is possible to create the BugType eagerly.

This patch is an alternative approach to https://reviews.llvm.org/D37437.

Diff Detail

Repository
rC Clang

Event Timeline

xazax.hun created this revision.Dec 22 2017, 3:13 AM

Hello Gabor,

For me, this patch looks much better than previous. I have some questions inline.

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
49

Hm, do we really want to use elaborated name here: class CheckName?

59

Looks like a misspell. Should it be "The check names are set after the constructors are run"?

63

Possibly I am missing the context, but could you please explain, why do we modify CheckName in getName() but not in getCheckName()? It seems a bit unclear to me.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp
277

If I understand correctly, if we report uninitialized and then unterminated, the second report will have wrong CheckName because it is never reset.

test/Analysis/malloc.c
1723

Should we enable the checker instead of removing test?

xazax.hun updated this revision to Diff 128240.Dec 27 2017, 1:46 PM
xazax.hun marked 5 inline comments as done.
  • Address review comments.
include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
63

Good catch, it is just an oversight. I got confused by CheckName vs Name. And since the analyzer called getName before getCheckName the tests passed.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp
277

That is right. Unfortunately, If unterminated check is not turned on but uninitialized is, we can end up with empty check names. I replaced this workaround with a slightly more robust one.

test/Analysis/malloc.c
1723

I tried that once, but this caused more new warnings (and sinks) so more changes would be required to make the tests pass. And this case is already tested in string.c.

a.sidorin added inline comments.Dec 29 2017, 7:48 AM
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
277

Maybe we should use different BugTypes for Uninitialized and Unterminated?

test/Analysis/malloc.c
1723

Thank you, I got it.

NoQ added a comment.Jan 3 2018, 2:59 PM

I'm still not quite sure what's the whole point of having BugType without a checker. We can still easily write anything we want in the "category" and "name" fields anyways, so we can easily produce bugs that are indistinguishable to the user from different checkers, while being able to distinguish them when we, as developers, want to figure out what checker throws them, so what was the whole point this whole time? I might be missing something, but if you have time, maybe remove the whole CheckName-based constructor and just tie every BugType to the checker? The current approach is totally fine though :)

include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
51–61

I suggest:

if (Checker)
  return Checker->getCheckname().getName();
return Check.getName();

Like, what's the point of storing the StringRef if we can retrieve it any time anyway?

I guess Checker does live long enough?

lib/StaticAnalyzer/Checkers/ValistChecker.cpp
277

Yep, this rings my bells too. We shouldn't race on how do we initialize a BugType depending on what bug is encountered first in the translation unit.

xazax.hun updated this revision to Diff 128630.Jan 4 2018, 10:24 AM
xazax.hun marked an inline comment as done.
  • Address some review comments.
include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
51–61

I slightly modified your snippet to preserve the assertion that can be useful to not to reintroduce this bug in the future.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp
277

Maybe I am missing something but right now there is no race on how we initialize the BugType. The result of the initialization can depend on, however, the set of checks that are registered. I agree that this is unfortunate but introducing a new BugType does not solve this problem or at least it is not apparent for me how would it solve this. One option would be to simply not report these issues unless the corresponding check is registered. The other option would be to make this depend on a new check name that needs to be enabled.

NoQ accepted this revision.Jan 4 2018, 4:52 PM
NoQ added inline comments.
include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
51–61

Yep, totally!

lib/StaticAnalyzer/Checkers/ValistChecker.cpp
277

Aha, that's right, yep, so this BugType's checker name would be different depending on which checks are enabled, not on bug first encountered, sorry. So it'd only have any effect if someone is grepping through bug types and turns these checkers on and off individually at the same time. Still worth a FIXME i guess? Is it hard to make a third bugtype with a hardcoded name that doesn't rely on the checker?

This revision is now accepted and ready to land.Jan 4 2018, 4:52 PM
This revision was automatically updated to reflect the committed changes.

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

Sure! I will try to do so: https://reviews.llvm.org/rC321933

hans added a subscriber: hans.Jan 16 2018, 5:00 AM

It missed the 6.0 branching. Will you try to get it on this branch?
Thanks

Sure! I will try to do so: https://reviews.llvm.org/rC321933

Merged in r322550. Thanks, Hans.