[Analyzer] Only add container note tags to the operations of the affected container

[baloghadamsoftware]

Summary

If an error happens which is related to a container the Container Modeling checker adds note tags to all the container operations along the bug path. This may be disturbing if there are other containers beside the one which is affected by the bug. This patch restricts the note tags to only the affected container and adjust the debug checkers to be able to test this change.

[Charusso]

I believe our path and context sensitive engine is more extensible and precise than checking the source file. Are you sure it scales? I would prefer to tie this information for MemRegions, rather than arbitrary places in the source code. My knowledge is very weak in this checker but I have changed from the Tidy world to the Analyzer to enjoy its benefits. Please enjoy these benefits in your work as well.

[baloghadamsoftware]

Alternative approach for debugging (instead of checking the source range): clang_analyzer_express() from ExprInspection marks its argument as interesting in the bug report. DebugContainerModeling propagates the interestingness from the symbol (begin or end symbol of the container) to the container itself.

[Charusso]

Cool, that one a lucky one! I think the SymbolRef based world also working, just at some point it could not scale because other systems are region based... For now, it is a much better solution, and this pattern to overload the callback with all the interestingness seems like the standard way of using NoteTags. Thanks!

[steakhal]

I don't have any technical comments on this patch since I haven't used NoteTags yet, only a couple of readability ones.

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp

97–103

Probably a flattened version would be more readable.

auto *PSBR = dyn_cast<PathSensitiveBugReport>(&BR);
if (PSBR && PSBR->isInteresting(Field))
  PSBR->markInteresting(Cont);
return "";

clang/test/Analysis/container-modeling.cpp

218

I'm not sure about the convention about using dashes, but I've seen multiple time comments like this:
no-warning etc. Probably a simple no-note or something would be more conventional?

232–234

This line looks quite crowded, can't we use expected-note@-1 here just like it was used previously?
The only once comment could be in a separate line as well, just to fit nicely.

[baloghadamsoftware]

Updated according to the style comments.

[Szelethus]

In D75514#1903153, @baloghadamsoftware wrote:

Alternative approach for debugging (instead of checking the source range): clang_analyzer_express() from ExprInspection marks its argument as interesting in the bug report. DebugContainerModeling propagates the interestingness from the symbol (begin or end symbol of the container) to the container itself.

But why is this related to UndefinedVal?

[baloghadamsoftware]

In D75514#1905268, @Szelethus wrote:

But why is this related to UndefinedVal?

Because UndefinedVal seems to be the "null" value of SVal thus it is suitable for default value of the parameter.

[Szelethus]

In D75514#1907392, @baloghadamsoftware wrote:

In D75514#1905268, @Szelethus wrote:

But why is this related to UndefinedVal?

Because UndefinedVal seems to be the "null" value of SVal thus it is suitable for default value of the parameter.

Shouldn't we use Optional then? None means something completely different then UndefinedVal.

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

55–61

I would also prefer to comment what happens when ExprVal has a special value, because its non-obvious from the declaration.

[NoQ]

In D75514#1907392, @baloghadamsoftware wrote:

In D75514#1905268, @Szelethus wrote:

But why is this related to UndefinedVal?

Because UndefinedVal seems to be the "null" value of SVal

It is not. It is a very specific and exotic SVal that almost never appears in practice but plays a very important role in the analysis.
Let's remove the default constructor for SVal entirely because it seems to be a common source of confusion.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

723–724

Mmm, this is definitely impossible. We should change the NoteTag API to pass in a PathSensitiveBugReport &. My bad.

[baloghadamsoftware]

Updated according to the comments.

[baloghadamsoftware]

Updated according to the comments.

[baloghadamsoftware]

Typo fixed.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

723–724

Good idea. For now I changed it to simple cast<>() without check.

[baloghadamsoftware]

Rebased.

[Szelethus]

The code from in ExprInspectionChecker.cpp is duplicated from D75677, isn't it?

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

55–57

Please terminate this sentence.

clang/test/Analysis/container-modeling.cpp

99–100

Why did we unpack this? And the rest?

[baloghadamsoftware]

Updated according to the comments.

[baloghadamsoftware]

In D75514#1917450, @Szelethus wrote:

The code from in ExprInspectionChecker.cpp is duplicated from D75677, isn't it?

It is. The one accepted earlier will be committed earlier, then I will rebase the other one so this part disappears from that one.

clang/test/Analysis/container-modeling.cpp

99–100

We did not unpack this but forgot to pack it.

[baloghadamsoftware]

Updated according to the comments of D75677.

[NoQ]

Wonderful, thank you!

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

726

I suspect you can remove the explicit conversion to std::string given that you've specified the return type explicitly.

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp

32

llvm::function_ref?

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp

32

Is that a thing? Nice.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp

32

You mean a callback instead of a template?

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

726

Theoretically yes, because llvm::StringRef has a conversion operator for std::string. However, when compiling I get a compilation error:

error: could not convert `Out.llvm::raw_svector_ostream::str()` from `llvm::StringRef` to `std::__cxx11::string {aka std::__cxx11::basic_string<char>}`

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

if (const auto *DR = dyn_cast<DeclRegion>(ContReg)) {
     Name = DR->getDecl()->getName();
   // If the region is not a `DeclRegion` then use the expression instead
   } else if (const auto *DRE =
              dyn_cast<DeclRefExpr>(ContE->IgnoreParenCasts())) {
     Name = DRE->getDecl()->getName();
   }
 
   return C.getNoteTag(
       [Text, Name, ContReg](PathSensitiveBugReport &BR) -> std::string {
+        if (!BR.isInteresting(ContReg))
+          return "";
+
         SmallString<256> Msg;
         llvm::raw_svector_ostream Out(Msg);
         Out << "Container " << (!Name.empty() ? ("'" + Name.str() + "' ") : "" )
             << Text;
         return std::string(Out.str());
       });
 }
 

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp

 
 namespace {
 
 class DebugContainerModeling
   : public Checker<eval::Call> {
 
   std::unique_ptr<BugType> DebugMsgBugType;
 
   template <typename Getter>
   void analyzerContainerDataField(const CallExpr *CE, CheckerContext &C,
                                   Getter get) const;
   void analyzerContainerBegin(const CallExpr *CE, CheckerContext &C) const;
   void analyzerContainerEnd(const CallExpr *CE, CheckerContext &C) const;
   ExplodedNode *reportDebugMsg(llvm::StringRef Msg, CheckerContext &C) const;
 
   typedef void (DebugContainerModeling::*FnCheck)(const CallExpr *,
                                                  CheckerContext &) const;
void DebugContainerModeling::analyzerContainerDataField(const CallExpr *CE,
   const MemRegion *Cont = C.getSVal(CE->getArg(0)).getAsRegion();
   if (Cont) {
     const auto *Data = getContainerData(State, Cont);
     if (Data) {
       SymbolRef Field = get(Data);
       if (Field) {
         State = State->BindExpr(CE, C.getLocationContext(),
                                 nonloc::SymbolVal(Field));
-        C.addTransition(State);
+
+        // Progpagate interestingness from the container's data (marked
+        // interesting by an `ExprInspection` debug call to the container
+        // itself.
+        const NoteTag *InterestingTag =
+          C.getNoteTag(
+              [Cont, Field](PathSensitiveBugReport &BR) -> std::string {
+                if (BR.isInteresting(Field)) {
+                  BR.markInteresting(Cont);
+                }
+                return "";
+              });
+        C.addTransition(State, InterestingTag);
         return;
       }
     }
   }
 
   auto &BVF = C.getSValBuilder().getBasicValueFactory();
   State = State->BindExpr(CE, C.getLocationContext(),
                    nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(0))));

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols,
   void analyzerHashDump(const CallExpr *CE, CheckerContext &C) const;
   void analyzerDenote(const CallExpr *CE, CheckerContext &C) const;
   void analyzerExpress(const CallExpr *CE, CheckerContext &C) const;
   void analyzerIsTainted(const CallExpr *CE, CheckerContext &C) const;
 
   typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *,
                                                  CheckerContext &C) const;
 
-  ExplodedNode *reportBug(llvm::StringRef Msg, CheckerContext &C) const;
+  // Optional parameter `ExprVal` for expression value to be marked interesting.
+  ExplodedNode *reportBug(llvm::StringRef Msg, CheckerContext &C,
+                          Optional<SVal> ExprVal = None) const;
   ExplodedNode *reportBug(llvm::StringRef Msg, BugReporter &BR,
-                          ExplodedNode *N) const;
+                          ExplodedNode *N,
+                          Optional<SVal> ExprVal = None) const;
 
 public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
                         ExprEngine &Eng) const;
 };
 }
 
if (StTrue) {
     if (StFalse)
       return "FALSE";
     else
       llvm_unreachable("Invalid constraint; neither true or false.");
   }
 }
 
 ExplodedNode *ExprInspectionChecker::reportBug(llvm::StringRef Msg,
-                                               CheckerContext &C) const {
+                                               CheckerContext &C,
+                                               Optional<SVal> ExprVal) const {
   ExplodedNode *N = C.generateNonFatalErrorNode();
-  reportBug(Msg, C.getBugReporter(), N);
+  reportBug(Msg, C.getBugReporter(), N, ExprVal);
   return N;
 }
 
 ExplodedNode *ExprInspectionChecker::reportBug(llvm::StringRef Msg,
                                                BugReporter &BR,
-                                               ExplodedNode *N) const {
+                                               ExplodedNode *N,
+                                               Optional<SVal> ExprVal) const {
   if (!N)
     return nullptr;
 
   if (!BT)
     BT.reset(new BugType(this, "Checking analyzer assumptions", "debug"));
 
-  BR.emitReport(std::make_unique<PathSensitiveBugReport>(*BT, Msg, N));
+  auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
+  if (ExprVal) {
+    R->markInteresting(*ExprVal);
+  }
+  BR.emitReport(std::move(R));
   return N;
 }
 
 void ExprInspectionChecker::analyzerEval(const CallExpr *CE,
                                          CheckerContext &C) const {
   const LocationContext *LC = C.getPredecessor()->getLocationContext();
 
   // A specific instantiation of an inlined function may have more constrained

 
 void ExprInspectionChecker::analyzerExpress(const CallExpr *CE,
                                             CheckerContext &C) const {
   if (CE->getNumArgs() == 0) {
     reportBug("clang_analyzer_express() requires a symbol", C);
     return;
   }
 
-  SymbolRef Sym = C.getSVal(CE->getArg(0)).getAsSymbol();
+  SVal ArgVal = C.getSVal(CE->getArg(0));
+  SymbolRef Sym = ArgVal.getAsSymbol();
   if (!Sym) {
     reportBug("Not a symbol", C);
     return;
   }
 
   SymbolExpressor V(C.getState());
   auto Str = V.Visit(Sym);
   if (!Str) {
     reportBug("Unable to express", C);
     return;
   }
 
-  reportBug(*Str, C);
+  reportBug(*Str, C, ArgVal);
 }
 
 void ExprInspectionChecker::analyzerIsTainted(const CallExpr *CE,
                                               CheckerContext &C) const {
   if (CE->getNumArgs() != 1) {
     reportBug("clang_analyzer_isTainted() requires exactly one argument", C);
     return;
   }

clang/test/Analysis/container-modeling.cpp

 
 void push_back(std::vector<int> &V, int n) {
   V.cbegin();
   V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
-  V.push_back(n); // expected-note{{Container 'V' extended to the back by 1 position}}
-                  // expected-note@-1{{Container 'V' extended to the back by 1 position}}
+  V.push_back(n); // expected-note 2{{Container 'V' extended to the back by 1 position}}
 
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
                                                              // expected-note@-1{{$V.begin()}}
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end() + 1}}
                                                            // expected-note@-1{{$V.end() + 1}}
 }
 
 /// emplace_back()
 ///
 /// Design decision: extends containers to the ->BACK-> (i.e. the
 /// past-the-end position of the container is incremented).
 
 void emplace_back(std::vector<int> &V, int n) {
   V.cbegin();
   V.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V), "$V.begin()");
   clang_analyzer_denote(clang_analyzer_container_end(V), "$V.end()");
 
   V.emplace_back(n); // expected-note 2{{Container 'V' extended to the back by 1 position}}
 
-
   clang_analyzer_express(clang_analyzer_container_begin(V)); // expected-warning{{$V.begin()}}
                                                              // expected-note@-1{{$V.begin()}}
   clang_analyzer_express(clang_analyzer_container_end(V)); // expected-warning{{$V.end() + 1}}
                                                            // expected-note@-1{{$V.end() + 1}}
 }
 
 /// pop_back()
 ///

 void push_back1(std::vector<int> &V1, std::vector<int> &V2, int n) {
   V1.cbegin();
   V1.cend();
   V2.cbegin();
   V2.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V1), "$V1.begin()");
 
-  V2.push_back(n); // expected-note{{Container 'V2' extended to the back by 1 position}} FIXME: This note should not appear since `V2` is not affected in the "bug"
+  V2.push_back(n); // no-note
 
   clang_analyzer_express(clang_analyzer_container_begin(V1)); // expected-warning{{$V1.begin()}}
                                                               // expected-note@-1{{$V1.begin()}}
 }
 
 void push_back2(std::vector<int> &V1, std::vector<int> &V2, int n) {
   V1.cbegin();
   V1.cend();
   V2.cbegin();
   V2.cend();
 
   clang_analyzer_denote(clang_analyzer_container_begin(V1), "$V1.begin()");
   clang_analyzer_denote(clang_analyzer_container_begin(V2), "$V2.begin()");
 
-  V1.push_back(n); // expected-note 2{{Container 'V1' extended to the back by 1 position}}
-                   // FIXME: This should appear only once since there is only
-                   // one "bug" where `V1` is affected
+  V1.push_back(n); // expected-note{{Container 'V1' extended to the back by 1 position}}
+                   // Only once!
 
   clang_analyzer_express(clang_analyzer_container_begin(V1)); // expected-warning{{$V1.begin()}}
                                                               // expected-note@-1{{$V1.begin()}}
 
   clang_analyzer_express(clang_analyzer_container_begin(V2)); // expected-warning{{$V2.begin()}}
                                                               // expected-note@-1{{$V2.begin()}}
 }
 
 /// Print Container Data as Part of the Program State
 
 void clang_analyzer_printState();
 
 void print_state(std::vector<int> &V) {
   V.cbegin();