This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix analyzer warnings.
ClosedPublic

Authored by NoQ on Aug 27 2019, 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.Aug 27 2019, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 5:07 PM
NoQ updated this revision to Diff 217537.Aug 27 2019, 5:20 PM
NoQ edited the summary of this revision. (Show Details)

Use auto in one more place.

Szelethus accepted this revision.Aug 27 2019, 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.Aug 27 2019, 5:48 PM
NoQ marked an inline comment as done.Aug 27 2019, 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.EditedAug 27 2019, 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.Aug 27 2019, 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.Aug 27 2019, 6:51 PM

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2019, 11:45 AM