[Analyzer] Use of BugType in DereferenceChecker (NFC).

[balazske]

Summary

Use of BuiltinBug is replaced by BugType.
Class BuiltinBug seems to have no benefits and is confusing.

[balazske]

Fixed failed tests because change of bug category from "Logic error" to "Memory error".

[martong]

AFAIK, BuiltinBug is deprecated and we should be using BugType anyway.
LGTM!

[Szelethus]

Lets make sure that we invite others from the community to participate, here and on other patches as well. Otherwise, LGTM.

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

214

Maybe "is definitely null" here? Otherwise it can be pretty confusing with a double negation.

clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist

270 ↗

(On Diff #280385)

I might've missed some discussions on that matter, so please correct me on that.
In my opinion, null pointer dereference is not a memory error. Null address is not a correct memory and never was, so it is a logic error that a special value is being interpreted as the pointer to something.

[balazske]

clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist

270 ↗

(On Diff #280385)

I can not decide which is better, "logic error" can be said for almost every possible bug. Here the problem is at least related to memory handling. The reference of undefined pointer value is "logic error" too (it is known that the value was not initialized) but a memory error (try to access invalid or valid but wrong address). Probably "pointer handling error" is better?
(One value for bug category is not a good approach, it is never possible to classify a bug to exactly one.)

[NoQ]

BuiltinBug is deprecated and we should be using BugType anyway

Like, i've never heard of BuiltinBug being deprecated but i'm pretty happy that it gets removed :)

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

214

Also this entire comment should be inside the if-statement, on the same level as the next comment.

clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist

270 ↗

(On Diff #280385)

Maybe let's not change it then, if there are no clear pros or cons? Users already got used to it.

[balazske]

Changed MemoryError back to LogicError.
Improved placement and text of changed comment.

[balazske]

The category string seems to be not really important, better not to change it.

[vsavchenko]

Awesome, thanks!

[NoQ]

Thank you!

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

 using namespace clang;
 using namespace ento;
 
 namespace {
 class DereferenceChecker
     : public Checker< check::Location,
                       check::Bind,
                       EventDispatcher<ImplicitNullDerefEvent> > {
-  mutable std::unique_ptr<BuiltinBug> BT_null;
-  mutable std::unique_ptr<BuiltinBug> BT_undef;
+  BugType BT_Null{this, "Dereference of null pointer", categories::LogicError};
+  BugType BT_Undef{this, "Dereference of undefined pointer value",
+                   categories::LogicError};
 
   void reportBug(ProgramStateRef State, const Stmt *S, CheckerContext &C) const;
 
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
                      CheckerContext &C) const;
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
 

 
 void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S,
                                    CheckerContext &C) const {
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
     return;
 
-  // We know that 'location' cannot be non-null.  This is what
-  // we call an "explicit" null dereference.
-  if (!BT_null)
-    BT_null.reset(new BuiltinBug(this, "Dereference of null pointer"));
-
   SmallString<100> buf;
   llvm::raw_svector_ostream os(buf);
 
   SmallVector<SourceRange, 2> Ranges;
 
   switch (S->getStmtClass()) {
   case Stmt::ArraySubscriptExprClass: {
     os << "Array access";
AddDerefSource(os, Ranges, IV->getBase()->IgnoreParenCasts(),
                    State.get(), N->getLocationContext(), true);
     break;
   }
   default:
     break;
   }
 
   auto report = std::make_unique<PathSensitiveBugReport>(
-      *BT_null, buf.empty() ? BT_null->getDescription() : StringRef(buf), N);
+      BT_Null, buf.empty() ? BT_Null.getDescription() : StringRef(buf), N);
 
   bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report);
 
   for (SmallVectorImpl<SourceRange>::iterator
        I = Ranges.begin(), E = Ranges.end(); I!=E; ++I)
     report->addRange(*I);
 
   C.emitReport(std::move(report));
 }
 
 void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
                                        CheckerContext &C) const {
   // Check for dereference of an undefined value.
   if (l.isUndef()) {
     if (ExplodedNode *N = C.generateErrorNode()) {
-      if (!BT_undef)
-        BT_undef.reset(
-            new BuiltinBug(this, "Dereference of undefined pointer value"));
-
       auto report = std::make_unique<PathSensitiveBugReport>(
-          *BT_undef, BT_undef->getDescription(), N);
+          BT_Undef, BT_Undef.getDescription(), N);
       bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report);
       C.emitReport(std::move(report));
     }
     return;
   }
 
   DefinedOrUnknownSVal location = l.castAs<DefinedOrUnknownSVal>();
 
   // Check for null dereferences.
   if (!location.getAs<Loc>())
     return;
 
   ProgramStateRef state = C.getState();
 
   ProgramStateRef notNullState, nullState;
   std::tie(notNullState, nullState) = state->assume(location);
 
-  // The explicit NULL case.
   if (nullState) {
     if (!notNullState) {
+      // We know that 'location' can only be null.  This is what
+      // we call an "explicit" null dereference.
       const Expr *expr = getDereferenceExpr(S);
       if (!suppressReport(expr)) {
         reportBug(nullState, expr, C);
         return;
       }
     }
 
     // Otherwise, we have the case where the location could either be