[analyzer] TrackConstraintBRVisitor: Do not track unknown values

[Charusso]

Summary

-

[Szelethus]

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

[NoQ]

This code is super messy. It'd be great to actually test this change on something to see if it has any unwanted side effects.

Could you see if it also fixes https://bugs.llvm.org/show_bug.cgi?id=42938 and, if so, add it as a test?

[NoQ]

In D66267#1630728, @Szelethus wrote:

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

I'm actually curious about one particular mess that we have here. Namely, there's a visitor that says "assuming..." and there's checker notes when checkers themselves assume something; how can we be sure they don't duplicate each other?

[Szelethus]

In D66267#1632164, @NoQ wrote:

In D66267#1630728, @Szelethus wrote:

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

I'm actually curious about one particular mess that we have here. Namely, there's a visitor that says "assuming..." and there's checker notes when checkers themselves assume something; how can we be sure they don't duplicate each other?

BugReporter.cpp does some deduplication of notes originating from TrackConstraintBRVisitor and ConditionBRVisitor, but I'm not sure how many other visitors/checkers do we have doing the same.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1980–1984

When trackNulOrUndeflValue to trackExpressionValue, I think it was an oversight that the comments in it weren't changed accordingly. This makes a lot more sense now, cheers!

[Charusso]

@Szelethus pointed out well. My patch is about nullability, and it is perfect. The bug report you made is about deduplication rather than nullability. The fact is we fail to call the deduplication function: eventsDescribeSameCondition() from removeRedundantMsgs(PathPieces &path) because the path does not contain the path pieces. We call removeRedundantMsgs with passing the PD->getMutablePieces(), but this only contains one piece of the foo function call in the test case you have attached.

I would upstream my hotfix of nullability without any tests as the comment says the intention and also we have plenty of tests of TrackConstraintBRVisitor, none of them changed.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1980–1984

was renamed to* oops.

[Charusso]

• Rebased.

[Szelethus]

In D66267#1632344, @Charusso wrote:

I would upstream my hotfix of nullability without any tests as the comment says the intention and also we have plenty of tests of TrackConstraintBRVisitor, none of them changed.

Sounds fair.

[Charusso]

Thanks for the reviews! I hope that mentioned error will be visible by the BugReporter revisions.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (R) {
           std::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R)));
 
       MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
           LVNode, R, EnableNullFPSuppression, report, V);
 
       report.markInteresting(V);
       report.addVisitor(std::make_unique<UndefOrNullArgVisitor>(R));
 
-      // If the contents are symbolic, find out when they became null.
-      if (V.getAsLocSymbol(/*IncludeBaseRegions*/ true))
+      // If the contents are symbolic and null, find out when they became null.
+      if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true))
+        if (LVState->isNull(V).isConstrainedTrue())
           report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
               V.castAs<DefinedSVal>(), false));
 
       // Add visitor, which will suppress inline defensive checks.
       if (auto DV = V.getAs<DefinedSVal>())
         if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() &&
             EnableNullFPSuppression)
           report.addVisitor(
               std::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
                                                                       LVNode));

clang/test/Analysis/cast-value.cpp

void evalReferences(const Shape &S) {
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   // logic-warning@-4 {{Dereference of null pointer}}
 }
 
 void evalNonNullParamNonNullReturnReference(const Shape &S) {
   const auto *C = dyn_cast_or_null<Circle>(S);
   // expected-note@-1 {{Assuming dynamic cast from 'Shape' to 'Circle' succeeds}}
-  // expected-note@-2 {{Assuming pointer value is null}}
-  // expected-note@-3 {{'C' initialized here}}
+  // expected-note@-2 {{'C' initialized here}}
 
   (void)(1 / !(bool)C);
   // expected-note@-1 {{'C' is non-null}}
   // expected-note@-2 {{Division by zero}}
   // expected-warning@-3 {{Division by zero}}
   // logic-warning@-4 {{Division by zero}}
 }
 
 void evalNonNullParamNonNullReturn(const Shape *S) {
   const auto *C = cast<Circle>(S);
   // expected-note@-1 {{Checked cast from 'Shape' to 'Circle' succeeds}}
-  // expected-note@-2 {{Assuming pointer value is null}}
-  // expected-note@-3 {{'C' initialized here}}
+  // expected-note@-2 {{'C' initialized here}}
 
   (void)(1 / !(bool)C);
   // expected-note@-1 {{'C' is non-null}}
   // expected-note@-2 {{Division by zero}}
   // expected-warning@-3 {{Division by zero}}
   // logic-warning@-4 {{Division by zero}}
 }
 
 void evalNonNullParamNullReturn(const Shape *S) {
   const auto *C = dyn_cast_or_null<Circle>(S);
   // expected-note@-1 {{Assuming dynamic cast from 'Shape' to 'Circle' fails}}
-  // expected-note@-2 {{Assuming pointer value is null}}
 
   if (const auto *T = dyn_cast_or_null<Triangle>(S)) {
     // expected-note@-1 {{Assuming dynamic cast from 'Shape' to 'Triangle' succeeds}}
     // expected-note@-2 {{'T' initialized here}}
     // expected-note@-3 {{'T' is non-null}}
     // expected-note@-4 {{Taking true branch}}
 
     (void)(1 / !T);
void evalNullParamNullReturn(const Shape *S) {
   (void)(1 / (bool)C);
   // expected-note@-1 {{Division by zero}}
   // expected-warning@-2 {{Division by zero}}
   // logic-warning@-3 {{Division by zero}}
 }
 
 void evalZeroParamNonNullReturnPointer(const Shape *S) {
   const auto *C = S->castAs<Circle>();
-  // expected-note@-1 {{Assuming pointer value is null}}
-  // expected-note@-2 {{Checked cast to 'Circle' succeeds}}
-  // expected-note@-3 {{'C' initialized here}}
+  // expected-note@-1 {{Checked cast to 'Circle' succeeds}}
+  // expected-note@-2 {{'C' initialized here}}
 
   (void)(1 / !(bool)C);
   // expected-note@-1 {{'C' is non-null}}
   // expected-note@-2 {{Division by zero}}
   // expected-warning@-3 {{Division by zero}}
   // logic-warning@-4 {{Division by zero}}
 }