[analyzer] Mention whether an event is about a condition in a bug report part 1

[Szelethus]

Summary

Can't add much more to the title! This is part 1, the case where the collapse point isn't in the condition point is the responsibility of ConditionBRVisitor, which I'm addressing in part 2.

[NoQ]

Fantastic! Let's open the wording bikeshed season?

I suspect that a simple "(The) Value -> Condition value" change would have worked better.

Another variant: "Value ..., which participates in a condition later".

[Szelethus]

In D65575#1611013, @NoQ wrote:

Fantastic! Let's open the wording bikeshed season?

I suspect that a simple "(The) Value -> Condition value" change would have worked better.

Another variant: "Value ..., which participates in a condition later".

Yea, I kinda prefer a more uniform indication as to whether we're explaining a condition or "THE value". While I personally took a unique approach in evaluating analysis results (my eye was hunting for the changes I made specifically), I did find each function call in the bug report super easy to understand:

See how this function call screams what it is about? Now, condition tracking is inherently imperfect (like bug report construction as a whole), and whenever I feel like the notes added by it provide little value, simply glancing at the notes can tell whether I should observe that function call or not.

I think it isn't crucial of getting rid of the "The" prefix, if we append ", which participates in a condition later" (which sounds so much better than what I added in this patch), so maybe changing WillBeUsedForACondition to that would be good enough. However, as I said, I realize that the way I looked at these results was a lot different than how the average user will do so, so I'm totally open on this topic.

[xazax.hun]

Yeah, this is important bike-shedding from the user point of view. Otherwise it looks good to me.

[Szelethus]

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

[NoQ]

In D65575#1625738, @Szelethus wrote:

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

Mmm, what are your thoughts on my suggestions with wording? I'll also poke Devin more.

Also it looks like most of the bottom of the stack is already accepted, you can just commit most of it, right?

[Szelethus]

In D65575#1625741, @NoQ wrote:

In D65575#1625738, @Szelethus wrote:

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

Mmm, what are your thoughts on my suggestions with wording? I'll also poke Devin more.

Pretty much this:

In D65575#1621425, @Szelethus wrote:

I think it isn't crucial of getting rid of the "The" prefix, if we append ", which participates in a condition later" (which sounds so much better than what I added in this patch), so maybe changing WillBeUsedForACondition to that would be good enough. However, as I said, I realize that the way I looked at these results was a lot different than how the average user will do so, so I'm totally open on this topic.

I figured we'd have another round on this, but if it sounds good, I'll update the patch.

Also it looks like most of the bottom of the stack is already accepted, you can just commit most of it, right?

Actually yea, I had a good reason to delay but not anymore.

[NoQ]

In D65575#1621425, @Szelethus wrote:

I think it isn't crucial of getting rid of the "The" prefix, if we append ", which participates in a condition later" (which sounds so much better than what I added in this patch), so maybe changing WillBeUsedForACondition to that would be good enough.

I guess let's just land that variant! I'm also not super worried about "The" here.

[Szelethus]

• Change the condition postfix as discussed
• expected-note{{...}} -> expected-note-re{{{{^}}...{{$}}}}

[NoQ]

Thanks!!

I also suspect we don't need the comma.

[Szelethus]

Hmm-hmm, I kinda like the comma, but would happily concede on this. Its been a while since my english exam :^)

[xazax.hun]

LGTM! I think the UIs could do better displaying this info in the future but this is not your job :)

[xazax.hun]

*actually accepting*

[Szelethus]

In D65575#1638430, @xazax.hun wrote:

LGTM! I think the UIs could do better displaying this info in the future but this is not your job :)

https://github.com/Ericsson/codechecker/pull/2279

CodeChecker now indents function calls! Woohoo!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 /// for which  the region of interest \p RegionOfInterest was passed into,
 /// but not written inside, and it has caused an undefined read or a null
 /// pointer dereference outside.
 class NoStoreFuncVisitor final : public BugReporterVisitor {
   const SubRegion *RegionOfInterest;
   MemRegionManager &MmrMgr;
   const SourceManager &SM;
   const PrintingPolicy &PP;
+  bugreporter::TrackingKind TKind;
 
   /// Recursion limit for dereferencing fields when looking for the
   /// region of interest.
   /// The limit of two indicates that we will dereference fields only once.
   static const unsigned DEREFERENCE_LIMIT = 2;
 
   /// Frames writing into \c RegionOfInterest.
   /// This visitor generates a note only if a function does not write into
   /// a region of interest. This information is not immediately available
   /// by looking at the node associated with the exit from the function
   /// (usually the return statement). To avoid recomputing the same information
   /// many times (going up the path for each node and checking whether the
   /// region was written into) we instead lazily compute the
   /// stack frames along the path which write into the region of interest.
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingRegion;
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
 
   using RegionVector = SmallVector<const MemRegion *, 5>;
 
 public:
-  NoStoreFuncVisitor(const SubRegion *R)
+  NoStoreFuncVisitor(const SubRegion *R, bugreporter::TrackingKind TKind)
       : RegionOfInterest(R), MmrMgr(*R->getMemRegionManager()),
         SM(MmrMgr.getContext().getSourceManager()),
-        PP(MmrMgr.getContext().getPrintingPolicy()) {}
+        PP(MmrMgr.getContext().getPrintingPolicy()), TKind(TKind) {}
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     static int Tag = 0;
     ID.AddPointer(&Tag);
     ID.AddPointer(RegionOfInterest);
   }
 
   void *getTag() const {
do {
     if (auto CE = N->getLocationAs<CallEnter>())
       if (CE->getCalleeContext() == OriginalSCtx)
         break;
 
     N = N->getFirstPred();
   } while (N);
 }
 
+static llvm::StringLiteral WillBeUsedForACondition =
+    ", which will be (a part of a) condition";
+
 std::shared_ptr<PathDiagnosticPiece> NoStoreFuncVisitor::maybeEmitNote(
     BugReport &R, const CallEvent &Call, const ExplodedNode *N,
     const RegionVector &FieldChain, const MemRegion *MatchedRegion,
     StringRef FirstElement, bool FirstIsReferenceType,
     unsigned IndirectionLevel) {
   // Optimistically suppress uninitialized value bugs that result
   // from system headers having a chance to initialize the value
   // but failing to do so. It's too unlikely a system header's fault.
std::shared_ptr<PathDiagnosticPiece> NoStoreFuncVisitor::maybeEmitNote(
   os << "Returning without writing to '";
 
   // Do not generate the note if failed to pretty-print.
   if (!prettyPrintRegionName(FirstElement, FirstIsReferenceType, MatchedRegion,
                              FieldChain, IndirectionLevel, os))
     return nullptr;
 
   os << "'";
+  if (TKind == bugreporter::TrackingKind::Condition)
+    os << WillBeUsedForACondition;
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
 bool NoStoreFuncVisitor::prettyPrintRegionName(StringRef FirstElement,
                                                bool FirstIsReferenceType,
                                                const MemRegion *MatchedRegion,
                                                const RegionVector &FieldChain,
                                                int IndirectionLevel,
if (LValue) {
         if (const auto *DD = dyn_cast<DeclaratorDecl>(DR->getDecl()))
           Out << " (loaded from '" << *DD << "')";
     }
 
     PathDiagnosticLocation L(Ret, BRC.getSourceManager(), CalleeSFC);
     if (!L.isValid() || !L.asLocation().isValid())
       return nullptr;
 
+    if (TKind == bugreporter::TrackingKind::Condition)
+      Out << WillBeUsedForACondition;
+
     auto EventPiece = std::make_shared<PathDiagnosticEventPiece>(L, Out.str());
 
     // If we determined that the note is meaningless, make it prunable, and
     // don't mark the stackframe interesting.
     if (WouldEventBeMeaningless)
       EventPiece->setPrunable(true);
     else
       BR.markInteresting(CalleeSFC);
FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
   } else if (StoreSite->getLocation().getAs<CallEnter>()) {
     if (const auto *VR = dyn_cast<VarRegion>(R))
       showBRParamDiagnostics(os, VR, V);
   }
 
   if (os.str().empty())
     showBRDefaultDiagnostics(os, R, V);
 
+  if (TKind == bugreporter::TrackingKind::Condition)
+    os << WillBeUsedForACondition;
+
   // Construct a new PathDiagnosticPiece.
   ProgramPoint P = StoreSite->getLocation();
   PathDiagnosticLocation L;
   if (P.getAs<CallEnter>() && InitE)
     L = PathDiagnosticLocation(InitE, BRC.getSourceManager(),
                                P.getLocationContext());
 
   if (!L.isValid() || !L.asLocation().isValid())
if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
     if (R) {
 
       // Mark both the variable region and its contents as interesting.
       SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
       report.addVisitor(
-          llvm::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R)));
+          llvm::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R), TKind));
 
       MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
           LVNode, R, EnableNullFPSuppression, report, V);
 
       report.markInteresting(V);
       report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(R));
 
       // If the contents are symbolic, find out when they became null.

clang/test/Analysis/track-control-dependency-conditions.cpp

 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-checker=core
 
 namespace example_1 {
 int flag;
 bool coin();
 
 void foo() {
-  flag = coin(); // tracking-note{{Value assigned to 'flag'}}
+  flag = coin(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
 }
 
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   flag = 1;
 
   foo(); // TODO: Add nodes here about flag's value being invalidated.
   if (flag) // expected-note   {{Assuming 'flag' is 0}}

 }
 } // end of namespace example_1
 
 namespace example_2 {
 int flag;
 bool coin();
 
 void foo() {
-  flag = coin(); // tracking-note{{Value assigned to 'flag'}}
+  flag = coin(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
 }
 
 void test() {
   int *x = 0;
   flag = 1;
 
   foo();
   if (flag) // expected-note   {{Assuming 'flag' is 0}}

 } // end of namespace example_2
 
 namespace global_variable_invalidation {
 int flag;
 bool coin();
 
 void foo() {
   // coin() could write bar, do it's invalidated.
-  flag = coin(); // tracking-note{{Value assigned to 'flag'}}
-                 // tracking-note@-1{{Value assigned to 'bar'}}
+  flag = coin(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
+                 // tracking-note@-1{{Value assigned to 'bar', which will be (a part of a) condition}}
 }
 
 int bar;
 
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   flag = 1;
 

 } // end of namespace note_from_different_but_not_nested_stackframe
 
 namespace important_returning_pointer_loaded_from {
 bool coin();
 
 int *getIntPtr();
 
 void storeValue(int **i) {
-  *i = getIntPtr(); // tracking-note{{Value assigned to 'i'}}
+  *i = getIntPtr(); // tracking-note{{Value assigned to 'i', which will be (a part of a) condition}}
 }
 
 int *conjurePointer() {
   int *i;
   storeValue(&i); // tracking-note{{Calling 'storeValue'}}
                   // tracking-note@-1{{Returning from 'storeValue'}}
-  return i; // tracking-note{{Returning pointer (loaded from 'i')}}
+  return i; // tracking-note{{Returning pointer (loaded from 'i'), which will be (a part of a) condition}}
 }
 
 void f(int *ptr) {
   if (ptr) // expected-note{{Assuming 'ptr' is null}}
            // expected-note@-1{{Taking false branch}}
     ;
   if (!conjurePointer())
     // tracking-note@-1{{Calling 'conjurePointer'}}

 namespace important_returning_value_note {
 bool coin();
 
 bool flipCoin() {
   if (coin()) // tracking-note{{Assuming the condition is false}}
               // tracking-note@-1{{Taking false branch}}
               // debug-note@-2{{Tracking condition 'coin()'}}
     return true;
-  return coin(); // tracking-note{{Returning value}}
+  return coin(); // tracking-note{{Returning value, which will be (a part of a) condition}}
 }
 
 void i(int *ptr) {
   if (ptr) // expected-note{{Assuming 'ptr' is null}}
            // expected-note@-1{{Taking false branch}}
     ;
   if (!flipCoin())
     // tracking-note@-1{{Calling 'flipCoin'}}

 
 namespace tracked_condition_written_in_nested_stackframe {
 int flag;
 int getInt();
 
 void foo() {
   int y;
   y = 1;
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
+  flag = y; // tracking-note{{The value 1 is assigned to 'flag', which will be (a part of a) condition}}
 }
 
 void f(int y) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
   foo(); // tracking-note{{Calling 'foo'}}
          // tracking-note@-1{{Returning from 'foo'}}
 
   if (flag) // expected-note{{'flag' is 1}}
             // expected-note@-1{{Taking true branch}}
             // debug-note@-2{{Tracking condition 'flag'}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace tracked_condition_written_in_nested_stackframe
 
 namespace condition_written_in_nested_stackframe_before_assignment {
 int flag = 0;
 int getInt();
 
 void foo() {
-  flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
+  flag = getInt(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
 }
 
 void f() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   int y = 0;
 
   foo(); // tracking-note{{Calling 'foo'}}
          // tracking-note@-1{{Returning from 'foo'}}

 #undef assert
 } // end of namespace dont_track_assert2like_or_conditions
 
 namespace only_track_the_evaluated_condition {
 
 bool coin();
 
 void bar(int &flag) {
-  flag = coin(); // tracking-note{{Value assigned to 'flag'}}
+  flag = coin(); // tracking-note{{Value assigned to 'flag', which will be (a part of a) condition}}
 }
 
 void bar2(int &flag2) {
   flag2 = coin();
 }
 
 void f(int *x) {
   if (x) // expected-note{{Assuming 'x' is null}}