[analyzer] ConditionBRVisitor: MemberExpr support

Summary: -

Reviewers: NoQ, george.karpenkov

Reviewed By: NoQ

Subscribers: cfe-commits, xazax.hun, baloghadamsoftware, szepet, a.sidorin,
             mikhail.ramalho, Szelethus, donat.nagy, dkrupp

Tags: #clang

Differential Revision: https://reviews.llvm.org/D58206

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@362026 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index 36372dacd3978502ed7bab97538d918311f48a7f..ef5d327d39da5932ea3fb6531f7b9c0bf467223d 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -202,6 +202,11 @@ public:
                 BugReporterContext &BRC, BugReport &R, const ExplodedNode *N,
                 bool TookTrue, bool IsAssuming);
 
+  std::shared_ptr<PathDiagnosticPiece>
+  VisitTrueTest(const Expr *Cond, const MemberExpr *ME, BugReporterContext &BRC,
+                BugReport &R, const ExplodedNode *N, bool TookTrue,
+                bool IsAssuming);
+
   std::shared_ptr<PathDiagnosticPiece>
   VisitConditionVariable(StringRef LhsString, const Expr *CondVarExpr,
                          BugReporterContext &BRC, BugReport &R,
@@ -225,7 +230,8 @@ public:
                     BugReporterContext &BRC,
                     BugReport &R,
                     const ExplodedNode *N,
-                    Optional<bool> &prunable);
+                    Optional<bool> &prunable,
+                    bool IsSameFieldName);
 
   static bool isPieceMessageGeneric(const PathDiagnosticPiece *Piece);
 };

diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index fb5b37508234a30436fc63dbc103a35760c18f14..c45b519582104be40a3f79eec0cdfdd509420d1a 100644 (file)
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1991,6 +1991,11 @@ ConditionBRVisitor::VisitTrueTest(const Expr *Cond, BugReporterContext &BRC,
                                    BRC, R, N, TookTrueTmp, IsAssuming))
           return P;
         break;
+      case Stmt::MemberExprClass:
+        if (auto P = VisitTrueTest(Cond, cast<MemberExpr>(CondTmp),
+                                   BRC, R, N, TookTrueTmp, IsAssuming))
+          return P;
+        break;
       case Stmt::UnaryOperatorClass: {
         const auto *UO = cast<UnaryOperator>(CondTmp);
         if (UO->getOpcode() == UO_LNot) {
@@ -2025,7 +2030,8 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex,
                                       BugReporterContext &BRC,
                                       BugReport &report,
                                       const ExplodedNode *N,
-                                      Optional<bool> &prunable) {
+                                      Optional<bool> &prunable,
+                                      bool IsSameFieldName) {
   const Expr *OriginalExpr = Ex;
   Ex = Ex->IgnoreParenCasts();
 
@@ -2091,6 +2097,17 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex,
     return false;
   }
 
+  if (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
+    if (!IsSameFieldName)
+      Out << "field '" << ME->getMemberDecl()->getName() << '\'';
+    else
+      Out << '\''
+          << Lexer::getSourceText(
+                 CharSourceRange::getTokenRange(Ex->getSourceRange()),
+                 BRC.getSourceManager(), BRC.getASTContext().getLangOpts(), 0)
+          << '\'';
+  }
+
   return false;
 }
 
@@ -2100,13 +2117,23 @@ std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
   bool shouldInvert = false;
   Optional<bool> shouldPrune;
 
+  // Check if the field name of the MemberExprs is ambiguous. Example:
+  // " 'a.d' is equal to 'h.d' " in 'test/Analysis/null-deref-path-notes.cpp'.
+  bool IsSameFieldName = false;
+  if (const auto *LhsME =
+          dyn_cast<MemberExpr>(BExpr->getLHS()->IgnoreParenCasts()))
+    if (const auto *RhsME =
+            dyn_cast<MemberExpr>(BExpr->getRHS()->IgnoreParenCasts()))
+      IsSameFieldName = LhsME->getMemberDecl()->getName() ==
+                        RhsME->getMemberDecl()->getName();
+
   SmallString<128> LhsString, RhsString;
   {
     llvm::raw_svector_ostream OutLHS(LhsString), OutRHS(RhsString);
-    const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS,
-                                       BRC, R, N, shouldPrune);
-    const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS,
-                                       BRC, R, N, shouldPrune);
+    const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS, BRC, R,
+                                       N, shouldPrune, IsSameFieldName);
+    const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS, BRC, R,
+                                       N, shouldPrune, IsSameFieldName);
 
     shouldInvert = !isVarLHS && isVarRHS;
   }
@@ -2170,11 +2197,15 @@ std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
   const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
 
+  // Convert 'field ...' to 'Field ...' if it is a MemberExpr.
+  std::string Message = Out.str();
+  Message[0] = toupper(Message[0]);
+
   // If we know the value create a pop-up note.
   if (!IsAssuming)
-    return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Out.str());
+    return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Message);
 
-  auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
+  auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Message);
   if (shouldPrune.hasValue())
     event->setPrunable(shouldPrune.getValue());
   return event;
@@ -2246,6 +2277,30 @@ std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
   return std::move(event);
 }
 
+std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
+    const Expr *Cond, const MemberExpr *ME, BugReporterContext &BRC,
+    BugReport &report, const ExplodedNode *N, bool TookTrue, bool IsAssuming) {
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream Out(Buf);
+
+  Out << (IsAssuming ? "Assuming field '" : "Field '")
+      << ME->getMemberDecl()->getName() << "' is ";
+
+  if (!printValue(ME, Out, N, TookTrue, IsAssuming))
+    return nullptr;
+
+  const LocationContext *LCtx = N->getLocationContext();
+  PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
+  if (!Loc.isValid() || !Loc.asLocation().isValid())
+    return nullptr;
+
+  // If we know the value create a pop-up note.
+  if (!IsAssuming)
+    return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Out.str());
+
+  return std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
+}
+
 bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream &Out,
                                     const ExplodedNode *N, bool TookTrue,
                                     bool IsAssuming) {

diff --git a/test/Analysis/Inputs/expected-plists/edges-new.mm.plist b/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
index 4eca510c3f0568d6daf6821f6cbef2f167a29cdf..b4c79018c666533be916798f7a2af2ed82b228a5 100644 (file)
--- a/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ b/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -22227,6 +22227,68 @@
            <key>file</key><integer>0</integer>
           </dict>
          </array>
+        <key>end</key>
+         <array>
+          <dict>
+           <key>line</key><integer>587</integer>
+           <key>col</key><integer>11</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+          <dict>
+           <key>line</key><integer>587</integer>
+           <key>col</key><integer>11</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+         </array>
+       </dict>
+      </array>
+    </dict>
+    <dict>
+     <key>kind</key><string>pop-up</string>
+     <key>location</key>
+     <dict>
+      <key>line</key><integer>587</integer>
+      <key>col</key><integer>11</integer>
+      <key>file</key><integer>0</integer>
+     </dict>
+     <key>ranges</key>
+     <array>
+       <array>
+        <dict>
+         <key>line</key><integer>587</integer>
+         <key>col</key><integer>11</integer>
+         <key>file</key><integer>0</integer>
+        </dict>
+        <dict>
+         <key>line</key><integer>587</integer>
+         <key>col</key><integer>16</integer>
+         <key>file</key><integer>0</integer>
+        </dict>
+       </array>
+     </array>
+     <key>extended_message</key>
+     <string>Field &apos;b&apos; is equal to 2</string>
+     <key>message</key>
+     <string>Field &apos;b&apos; is equal to 2</string>
+    </dict>
+    <dict>
+     <key>kind</key><string>control</string>
+     <key>edges</key>
+      <array>
+       <dict>
+        <key>start</key>
+         <array>
+          <dict>
+           <key>line</key><integer>587</integer>
+           <key>col</key><integer>11</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+          <dict>
+           <key>line</key><integer>587</integer>
+           <key>col</key><integer>11</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+         </array>
         <key>end</key>
          <array>
           <dict>

diff --git a/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist b/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist
index 35b14fba4c89a066d55ec45e2475f5374cab0d51..47c3b8df09c9fac7d007563fee4ca6279ab2b081 100644 (file)
--- a/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist
+++ b/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist
@@ -165,9 +165,9 @@
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Assuming pointer value is null</string>
+     <string>Assuming field &apos;x&apos; is null</string>
      <key>message</key>
-     <string>Assuming pointer value is null</string>
+     <string>Assuming field &apos;x&apos; is null</string>
     </dict>
     <dict>
      <key>kind</key><string>control</string>
@@ -454,9 +454,9 @@
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Assuming pointer value is null</string>
+     <string>Assuming field &apos;x&apos; is null</string>
      <key>message</key>
-     <string>Assuming pointer value is null</string>
+     <string>Assuming field &apos;x&apos; is null</string>
     </dict>
     <dict>
      <key>kind</key><string>control</string>

diff --git a/test/Analysis/diagnostics/deref-track-symbolic-region.c b/test/Analysis/diagnostics/deref-track-symbolic-region.c
index 63d0971b85d60e64a8e2a5d6e54e9ce9961d19d2..1a1190f934d0690bbebe1d6ee329e01ffb263681 100644 (file)
--- a/test/Analysis/diagnostics/deref-track-symbolic-region.c
+++ b/test/Analysis/diagnostics/deref-track-symbolic-region.c
@@ -15,8 +15,8 @@ void test(struct S syz, int *pp) {
 
   struct S *ps = &syz;
   if (ps->x)
-    //expected-note@-1{{Taking false branch}}
-    //expected-note@-2{{Assuming pointer value is null}}
+    //expected-note@-1{{Assuming field 'x' is null}}
+    //expected-note@-2{{Taking false branch}}
 
     m++;
 
@@ -30,8 +30,8 @@ void testTrackConstraintBRVisitorIsTrackingTurnedOn(struct S syz, int *pp) {
 
   struct S *ps = &syz;
   if (ps->x)
-    //expected-note@-1{{Taking false branch}}
-    //expected-note@-2{{Assuming pointer value is null}}
+    //expected-note@-1{{Assuming field 'x' is null}}
+    //expected-note@-2{{Taking false branch}}
 
     m++;
   int *p = syz.x; //expected-note {{'p' initialized to a null pointer value}}

diff --git a/test/Analysis/diagnostics/dtors.cpp b/test/Analysis/diagnostics/dtors.cpp
index b3fe7ec803a9d8c92b5f78034f0dc5881ab9f6b1..18bedc61f98e8ab59a19f5fe89a5312ea4396d5c 100644 (file)
--- a/test/Analysis/diagnostics/dtors.cpp
+++ b/test/Analysis/diagnostics/dtors.cpp
@@ -16,10 +16,11 @@ struct smart_ptr {
   S *s;
   smart_ptr(S *);
   S *get() {
-    return (x || 0) ? nullptr : s; // expected-note{{Left side of '||' is false}}
-                                   // expected-note@-1{{'?' condition is false}}
-                                   // expected-warning@-2{{Use of memory after it is freed}}
-                                   // expected-note@-3{{Use of memory after it is freed}}
+    return (x || 0) ? nullptr : s; // expected-note{{Field 'x' is 0}}
+                                   // expected-note@-1{{Left side of '||' is false}}
+                                   // expected-note@-2{{'?' condition is false}}
+                                   // expected-warning@-3{{Use of memory after it is freed}}
+                                   // expected-note@-4{{Use of memory after it is freed}}
   }
 };
 

diff --git a/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist b/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist
index f517d4dddbc60cb39c5b8e303a66d763ebfb2451..c9fd8c848bdcaeefb8ea54d45d5e1bae87be819c 100644 (file)
--- a/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist
+++ b/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist
@@ -4271,9 +4271,9 @@
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Assuming pointer value is null</string>
+     <string>Assuming field &apos;arr&apos; is null</string>
      <key>message</key>
-     <string>Assuming pointer value is null</string>
+     <string>Assuming field &apos;arr&apos; is null</string>
     </dict>
     <dict>
      <key>kind</key><string>control</string>

diff --git a/test/Analysis/inlining/path-notes.cpp b/test/Analysis/inlining/path-notes.cpp
index 43859237438f2605c71452c03e34fde0b56d21e5..ef56cc76f82cfa9935242a67e381a40f37b5f0c1 100644 (file)
--- a/test/Analysis/inlining/path-notes.cpp
+++ b/test/Analysis/inlining/path-notes.cpp
@@ -231,7 +231,7 @@ struct Owner {
 };
 
 void Owner::testGetDerefExprOnMemberExprWithADot() {
-       if (arr)  // expected-note {{Assuming pointer value is null}}
+       if (arr)  // expected-note {{Assuming field 'arr' is null}}
             // expected-note@-1 {{Taking false branch}}
          ;
        arr[1].x = 1; //expected-warning {{Dereference of null pointer}}

diff --git a/test/Analysis/null-deref-path-notes.cpp b/test/Analysis/null-deref-path-notes.cpp
index dd54b24e78471266b5567d44bad468a71f35e9b4..c7b0619e297b3c5b254db1daa13c7a8e92bf9c3b 100644 (file)
--- a/test/Analysis/null-deref-path-notes.cpp
+++ b/test/Analysis/null-deref-path-notes.cpp
@@ -19,7 +19,7 @@ void c::f(B &g, int &i) {
                 // expected-note@-1{{Array access (via field 'd') results in a null pointer dereference}}
   B h, a; // expected-note{{Value assigned to 'h.d'}}
   a.d == __null; // expected-note{{Assuming the condition is true}}
-  a.d != h.d; // expected-note{{Assuming pointer value is null}}
+  a.d != h.d; // expected-note{{Assuming 'a.d' is equal to 'h.d'}}
   f(h, b); // expected-note{{Calling 'c::f'}}
 }
 }

diff --git a/test/Analysis/osobject-retain-release.cpp b/test/Analysis/osobject-retain-release.cpp
index 9d7cd854422eba9326e81df6b2e49c99d1b35805..10ef144bf36e9a7e7307f8de26e557ca4b5884b0 100644 (file)
--- a/test/Analysis/osobject-retain-release.cpp
+++ b/test/Analysis/osobject-retain-release.cpp
@@ -601,16 +601,18 @@ void test_smart_ptr_uaf() {
   {
     OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr<OSObject>'}}
    // expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
+    // expected-note@os_smart_ptr.h:13{{Field 'pointer' is non-null}}
     // expected-note@os_smart_ptr.h:13{{Taking true branch}}
     // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}}
     // expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}}
     // expected-note@os_smart_ptr.h:14{{Returning from 'smart_ptr::_retain'}}
   } // expected-note{{Calling '~smart_ptr'}}
+  // expected-note@os_smart_ptr.h:35{{Field 'pointer' is non-null}}
   // expected-note@os_smart_ptr.h:35{{Taking true branch}}
   // expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}}
   // expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}}
   // expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}}
- // expected-note@-5{{Returning from '~smart_ptr'}}
+ // expected-note@-6{{Returning from '~smart_ptr'}}
   obj->release(); // expected-note{{Object released}}
   obj->release(); // expected-warning{{Reference-counted object is used after it is released}}
 // expected-note@-1{{Reference-counted object is used after it is released}}
@@ -621,16 +623,18 @@ void test_smart_ptr_leak() {
   {
     OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr<OSObject>'}}
    // expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
+    // expected-note@os_smart_ptr.h:13{{Field 'pointer' is non-null}}
     // expected-note@os_smart_ptr.h:13{{Taking true branch}}
     // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}}
     // expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}}
     // expected-note@os_smart_ptr.h:14{{Returning from 'smart_ptr::_retain'}}
   } // expected-note{{Calling '~smart_ptr'}}
+  // expected-note@os_smart_ptr.h:35{{Field 'pointer' is non-null}}
   // expected-note@os_smart_ptr.h:35{{Taking true branch}}
   // expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}}
   // expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}}
   // expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}}
- // expected-note@-5{{Returning from '~smart_ptr'}}
+ // expected-note@-6{{Returning from '~smart_ptr'}}
 } // expected-warning{{Potential leak of an object stored into 'obj'}}
 // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
 

diff --git a/test/Analysis/uninit-vals.m b/test/Analysis/uninit-vals.m
index 5b959c7bfe19e87105c16cbcc37f89f74b649ccd..b59f72b10770b0d406e39fd4b0ca4308ce6618fe 100644 (file)
--- a/test/Analysis/uninit-vals.m
+++ b/test/Analysis/uninit-vals.m
@@ -164,7 +164,7 @@ void PR14765_test() {
                                            // expected-note@-1{{TRUE}}
 
   testObj->origin = makePoint(0.0, 0.0);
-  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming field 'size' is <= 0}}
                                // expected-note@-1{{Taking false branch}}
 
   // FIXME: Assigning to 'testObj->origin' kills the default binding for the
@@ -219,13 +219,13 @@ void PR14765_test_int() {
                                                // expected-note@-1{{TRUE}}
 
   testObj->origin = makeIntPoint(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming field 'size' is <= 0}}
                                // expected-note@-1{{Taking false branch}}
-                               // expected-note@-2{{Assuming the condition is false}}
+                               // expected-note@-2{{Assuming field 'size' is <= 0}}
                                // expected-note@-3{{Taking false branch}}
-                               // expected-note@-4{{Assuming the condition is false}}
+                               // expected-note@-4{{Assuming field 'size' is <= 0}}
                                // expected-note@-5{{Taking false branch}}
-                               // expected-note@-6{{Assuming the condition is false}}
+                               // expected-note@-6{{Assuming field 'size' is <= 0}}
                                // expected-note@-7{{Taking false branch}}
 
   // FIXME: Assigning to 'testObj->origin' kills the default binding for the
@@ -321,9 +321,12 @@ void testSmallStructInLargerStruct() {
                                                // expected-note@-1{{TRUE}}
 
   testObj->origin = makeIntPoint2D(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Field 'size' is <= 0}}
                                // expected-note@-1{{Taking false branch}}
-                               // expected-note@-2{{Taking false branch}}
+                               // expected-note@-2{{Field 'size' is <= 0}}
+                               // expected-note@-3{{Taking false branch}}
+                               // expected-note@-4{{Field 'size' is <= 0}}
+                               // expected-note@-5{{Taking false branch}}
 
   clang_analyzer_eval(testObj->size == 0); // expected-warning{{TRUE}}
                                            // expected-note@-1{{TRUE}}