Page MenuHomePhabricator

[analyzer] Fix analyzer warnings.
ClosedPublic

Authored by NoQ on Tue, Aug 27, 5:06 PM.

Details

Summary

They ain't gonna fix themselves... well, not yet.

This is an outcome for this summer's GSoC! - http://lists.llvm.org/pipermail/cfe-dev/2019-August/063195.html

Found one real crash, added a couple tests:

Addressed 17 style issues that aren't real crashes - mostly misuses of dyn_cast / getAs (i.e., "if the dyn_ part is really necessary here, then you crash with a null dereference a few lines below because you didn't check the result"). These changes indeed made the code cleaner and shorter.

Suppressed 4 false positives; i found these assertions/refactorings nice anyway, except one (which i'd prefer to have as a post-condition on a certain function but we didn't invent post-conditions yet in C++).

That leaves us with only one remaining false positive that isn't really actionable; i'll have to investigate.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Tue, Aug 27, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 27, 5:07 PM
NoQ updated this revision to Diff 217537.Tue, Aug 27, 5:20 PM
NoQ edited the summary of this revision. (Show Details)

Use auto in one more place.

Szelethus accepted this revision.Tue, Aug 27, 5:48 PM

if the dyn_ part is really necessary here, then you crash with a null dereference a few lines below because you didn't check the result

Hmm, can we detect things like this?:

if (isa<FunctionDecl>(D)) {
  const auto *FD = dyn_cast(D); // warn: D is known to be a FunctionDecl, prefer using cast<>
  // ...
}
This revision is now accepted and ready to land.Tue, Aug 27, 5:48 PM
NoQ marked an inline comment as done.Tue, Aug 27, 5:54 PM

if the dyn_ part is really necessary here, then you crash with a null dereference a few lines below because you didn't check the result

Hmm, can we detect things like this?:

if (isa<FunctionDecl>(D)) {
  const auto *FD = dyn_cast(D); // warn: D is known to be a FunctionDecl, prefer using cast<>
  // ...
}

We should be detecting things like this as of D66325 and D66423 (we remember which casts were successful and don't re-assume them again), but LocalizationChecker.cpp turned out to have a false positive of this kind. @Charusso, you may want to take a look.

clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
885–888 ↗(On Diff #217537)

Note: this was a false positive.

NoQ added a comment.EditedTue, Aug 27, 5:56 PM

I mean, such code should indeed be refactored to query the custom RTTI only once, but given that it's locally obvious that there's no null dereference in this code, we should not be emitting a warning on it. Or, at least, emitting a different kind of warning.

Another common mistake is this:

if (A) {
  const auto *B = dyn_cast_or_null<FunctionDecl>(A); // warn: A is known to be non-null, prefer dyn_cast
}
NoQ added a comment.Tue, Aug 27, 6:41 PM
if (isa<FunctionDecl>(D)) {
  const auto *FD = dyn_cast<FunctionDecl>(D); // warn: D is known to be a FunctionDecl, prefer using cast<>
  // ...
}
if (A) {
  const auto *B = dyn_cast_or_null<FunctionDecl>(A); // warn: A is known to be non-null, prefer dyn_cast
}

Both of these are must-problems. They are hard to solve via symbolic execution because all execution paths need to be taken into account simultaneously. Eg.:

if (isa<FunctionDecl>(D)) {
  if (coin)
    D = getDecl(); // Maybe not a FunctionDecl.

  // It's not enough to know that D is a FunctionDecl assuming coin is false.
  const auto *FD = dyn_cast<FunctionDecl>(D); // no-warning:
}

We could maybe make a syntactic checker or a data flow checker to find these anti-patterns. @Charusso also had an idea for a clang-tidy checker that emits fixits for code like dyn_cast<T>(x)->foo() ("use 'cast' instead").

NoQ added a comment.Tue, Aug 27, 6:51 PM

Filed the remaining false positive back on myself as https://bugs.llvm.org/show_bug.cgi?id=43135.

Closed by commit rL370246: [analyzer] Fix analyzer warnings on analyzer. (authored by dergachev, committed by ). · Explain WhyWed, Aug 28, 11:45 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 28, 11:45 AM