[analyzer] Correctly create a non-fatal error node for VA list checker.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp

215

Note that the LeakedVALists might be empty. In that case, we generate an error node without emitting any warnings. Is this intended?

[george.karpenkov]

clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp

215

Good point, it also has an early return.

[NoQ]

We noticed this when debugging D47856, because the visitor was saying "va_list ended" right before the report, because the key is indeed removed from the program state just before reporting the issue, so it's not there in the issue state and is there in the state before that.

Due to temporary insanity i thought that chaining two nodes together is the right approach here, but i guess other checkers work just fine without it, so i think it's most likely unnecessary.

Still, ideally we need to always generate[NonFatal]ErrorNode when we through our reports.

[george.karpenkov]

Turns out I don't need it for the other revision though.

clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp

void ValistChecker::checkDeadSymbols(SymbolReaper &SR,
                                      CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   InitializedVAListsTy TrackedVALists = State->get<InitializedVALists>();
   RegionVector LeakedVALists;
   for (auto Reg : TrackedVALists) {
     if (SR.isLiveRegion(Reg))
       continue;
     LeakedVALists.push_back(Reg);
-    State = State->remove<InitializedVALists>(Reg);
   }
-  if (ExplodedNode *N = C.addTransition(State))
+  ExplodedNode *Pred = C.getPredecessor();
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     reportLeakedVALists(LeakedVALists, "Initialized va_list", " is leaked", C,
                         N);
+    Pred = N;
+  }
+
+  for (auto Reg : LeakedVALists) {
+    State = State->remove<InitializedVALists>(Reg);
+  }
+  C.addTransition(State, Pred);
 }
 
 // This function traverses the exploded graph backwards and finds the node where
 // the va_list is initialized. That node is used for uniquing the bug paths.
 // It is not likely that there are several different va_lists that belongs to
 // different stack frames, so that case is not yet handled.
 const ExplodedNode *
 ValistChecker::getStartCallSite(const ExplodedNode *N,
if (Arg2) {
       if (ChecksEnabled[CK_CopyToSelf] && VAList == Arg2) {
         RegionVector LeakedVALists{VAList};
         if (ExplodedNode *N = C.addTransition(State))
           reportLeakedVALists(LeakedVALists, "va_list",
                               " is copied onto itself", C, N, true);
         return;
       } else if (!State->contains<InitializedVALists>(Arg2) && !Symbolic) {
         if (State->contains<InitializedVALists>(VAList)) {
-          State = State->remove<InitializedVALists>(VAList);
+          ExplodedNode *Pred = C.getPredecessor();
+          if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
             RegionVector LeakedVALists{VAList};
-          if (ExplodedNode *N = C.addTransition(State))
             reportLeakedVALists(LeakedVALists, "Initialized va_list",
                                 " is overwritten by an uninitialized one", C, N,
                                 true);
+            Pred = N;
+          }
+          State = State->remove<InitializedVALists>(VAList);
+          C.addTransition(State, Pred);
         } else {
           reportUninitializedAccess(Arg2, "Uninitialized va_list is copied", C);
         }
         return;
       }
     }
   }
   if (State->contains<InitializedVALists>(VAList)) {