[Sema] Patch to issue warning on comparing parameters with

nonnull attribute when comparison is always true/false.
Original patch by Steven Wu. I have added extra code to prevent issuing of
warning  when the nonnull parameter is modified prior to the comparison.
This addition prevents false positives in the most obvious cases.
There may still be false positive warnings in some cases (as one of my tests
indicates), but benefit far outweighs such cases. rdar://18712242

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

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index d01a2d62bdca742a936cdf4a0316f3cdf7ea9e77..aa93e63a4dfc97f414cf15b97ac70115d2f671ee 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2509,6 +2509,10 @@ def warn_impcast_pointer_to_bool : Warning<
     "address of%select{| function| array}0 '%1' will always evaluate to "
     "'true'">,
     InGroup<PointerBoolConversion>;
+def warn_cast_nonnull_to_bool : Warning<
+    "nonnull parameter '%0' will evaluate to "
+    "'true' on first encounter">,
+    InGroup<PointerBoolConversion>;
 def warn_this_bool_conversion : Warning<
   "'this' pointer cannot be null in well-defined C++ code; pointer may be "
   "assumed to always convert to true">, InGroup<UndefinedBoolConversion>;
@@ -2521,6 +2525,10 @@ def warn_null_pointer_compare : Warning<
     "comparison of %select{address of|function|array}0 '%1' %select{not |}2"
     "equal to a null pointer is always %select{true|false}2">,
     InGroup<TautologicalPointerCompare>;
+def warn_nonnull_parameter_compare : Warning<
+    "comparison of nonnull parameter '%0' %select{not |}1"
+    "equal to a null pointer is %select{true|false}1 on first encounter">,
+    InGroup<TautologicalPointerCompare>;
 def warn_this_null_compare : Warning<
   "'this' pointer cannot be null in well-defined C++ code; comparison may be "
   "assumed to always evaluate to %select{true|false}0">,

diff --git a/include/clang/Sema/ScopeInfo.h b/include/clang/Sema/ScopeInfo.h
index c0b6df40aa6d413eff87e55b21ed1b603d193215..d63b734a8dbffc0c10b1071bd110d5b8d4e10aef 100644 (file)
--- a/include/clang/Sema/ScopeInfo.h
+++ b/include/clang/Sema/ScopeInfo.h
@@ -144,6 +144,10 @@ public:
   /// current function scope.  These diagnostics are vetted for reachability
   /// prior to being emitted.
   SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags;
+  
+  /// \brief A list of parameters which have the nonnull attribute and are
+  /// modified in the function.
+  llvm::SmallPtrSet<const ParmVarDecl*, 8>  ModifiedNonNullParams;
 
 public:
   /// Represents a simple identification of a weak object.

diff --git a/lib/Sema/ScopeInfo.cpp b/lib/Sema/ScopeInfo.cpp
index c4bf67b76cda53df19ae9ee01c9bd79a55874b73..00d9982ac06fe4c996ceca3f93c22e7a9b4a439c 100644 (file)
--- a/lib/Sema/ScopeInfo.cpp
+++ b/lib/Sema/ScopeInfo.cpp
@@ -39,6 +39,7 @@ void FunctionScopeInfo::Clear() {
   ErrorTrap.reset();
   PossiblyUnreachableDiags.clear();
   WeakObjectUses.clear();
+  ModifiedNonNullParams.clear();
 }
 
 static const NamedDecl *getBestPropertyDecl(const ObjCPropertyRefExpr *PropE) {

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 72fc3e37490c4fef1b84b6b609b8d3949b6e5520..aa6bf1760d3da0210416cab60850ecd0855ae4d7 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -6739,7 +6739,39 @@ void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
   // Weak Decls can be null.
   if (!D || D->isWeak())
     return;
-    
+  
+  // Check for parameter decl with nonnull attribute
+  if (const ParmVarDecl* PV = dyn_cast<ParmVarDecl>(D)) {
+    if (getCurFunction() && !getCurFunction()->ModifiedNonNullParams.count(PV))
+      if (const FunctionDecl* FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
+        unsigned NumArgs = FD->getNumParams();
+        llvm::SmallBitVector AttrNonNull(NumArgs);
+        for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
+          if (!NonNull->args_size()) {
+            AttrNonNull.set(0, NumArgs);
+            break;
+          }
+          for (unsigned Val : NonNull->args()) {
+            if (Val >= NumArgs)
+              continue;
+            AttrNonNull.set(Val);
+          }
+        }
+        if (!AttrNonNull.empty())
+          for (unsigned i = 0; i < NumArgs; ++i)
+            if (FD->getParamDecl(i) == PV && AttrNonNull[i]) {
+              std::string Str;
+              llvm::raw_string_ostream S(Str);
+              E->printPretty(S, nullptr, getPrintingPolicy());
+              unsigned DiagID = IsCompare ? diag::warn_nonnull_parameter_compare
+                                          : diag::warn_cast_nonnull_to_bool;
+              Diag(E->getExprLoc(), DiagID) << S.str() << E->getSourceRange()
+                << Range << IsEqual;
+              return;
+            }
+      }
+    }
+  
   QualType T = D->getType();
   const bool IsArray = T->isArrayType();
   const bool IsFunction = T->isFunctionType();

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 1429df778b58390d90c64ec99ce47adec09de8b7..0104020bba896bf7beddc44aeae06ed93ad46daf 100644 (file)
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -9121,6 +9121,24 @@ QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
   return Context.getPointerType(op->getType());
 }
 
+static void RecordModifiableNonNullParam(Sema &S, const Expr *Exp) {
+  const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp);
+  if (!DRE)
+    return;
+  const Decl *D = DRE->getDecl();
+  if (!D)
+    return;
+  const ParmVarDecl *Param = dyn_cast<ParmVarDecl>(D);
+  if (!Param)
+    return;
+  if (const FunctionDecl* FD = dyn_cast<FunctionDecl>(Param->getDeclContext()))
+    if (!FD->hasAttr<NonNullAttr>())
+      return;
+  if (FunctionScopeInfo *FD = S.getCurFunction())
+    if (!FD->ModifiedNonNullParams.count(Param))
+      FD->ModifiedNonNullParams.insert(Param);
+}
+
 /// CheckIndirectionOperand - Type check unary indirection (prefix '*').
 static QualType CheckIndirectionOperand(Sema &S, Expr *Op, ExprValueKind &VK,
                                         SourceLocation OpLoc) {
@@ -9360,6 +9378,7 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
     }
     if (!ResultTy.isNull())
       DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
+    RecordModifiableNonNullParam(*this, LHS.get());
     break;
   case BO_PtrMemD:
   case BO_PtrMemI:
@@ -9833,6 +9852,7 @@ ExprResult Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc,
     break;
   case UO_AddrOf:
     resultType = CheckAddressOfOperand(Input, OpLoc);
+    RecordModifiableNonNullParam(*this, InputExpr);
     break;
   case UO_Deref: {
     Input = DefaultFunctionArrayLvalueConversion(Input.get());

diff --git a/test/Sema/nonnull.c b/test/Sema/nonnull.c
index 3b654a889072ea638509e8622a46a5897c3135a1..de47e9664ca10276dc97df28a5fa262d6cebc029 100644 (file)
--- a/test/Sema/nonnull.c
+++ b/test/Sema/nonnull.c
@@ -83,3 +83,61 @@ void redecl_test(void *p) {
   redecl(p, 0); // expected-warning{{null passed}}
   redecl(0, p); // expected-warning{{null passed}}
 }
+
+// rdar://18712242
+#define NULL (void*)0
+__attribute__((__nonnull__))
+int evil_nonnull_func(int* pointer, void * pv)
+{
+   if (pointer == NULL) {  // expected-warning {{comparison of nonnull parameter 'pointer' equal to a null pointer is false on first encounter}}
+     return 0;
+   } else {
+     return *pointer;
+   } 
+
+   pointer = pv;
+   if (!pointer)
+     return 0;
+   else
+     return *pointer;
+
+   if (pv == NULL) {} // expected-warning {{comparison of nonnull parameter 'pv' equal to a null pointer is false on first encounter}}
+}
+
+void set_param_to_null(int**);
+int another_evil_nonnull_func(int* pointer, char ch, void * pv) __attribute__((nonnull(1, 3)));
+int another_evil_nonnull_func(int* pointer, char ch, void * pv) {
+   if (pointer == NULL) { // expected-warning {{comparison of nonnull parameter 'pointer' equal to a null pointer is false on first encounter}}
+     return 0;
+   } else {
+     return *pointer;
+   } 
+
+   set_param_to_null(&pointer);
+   if (!pointer)
+     return 0;
+   else
+     return *pointer;
+
+   if (pv == NULL) {} // expected-warning {{comparison of nonnull parameter 'pv' equal to a null pointer is false on first encounter}}
+}
+
+extern void *returns_null(void**);
+extern void FOO();
+extern void FEE();
+
+extern void *pv;
+__attribute__((__nonnull__))
+void yet_another_evil_nonnull_func(int* pointer)
+{
+ while (pv) {
+   // This comparison will not be optimized away.
+   if (pointer) {  // expected-warning {{nonnull parameter 'pointer' will evaluate to 'true' on first encounter}}
+     FOO();
+   } else {
+     FEE();
+   } 
+   pointer = returns_null(&pv);
+ }
+}
+