[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

llvm-svn: 222264

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d01a2d6..aa93e63 100644 (file)
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/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/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
index c0b6df4..d63b734 100644 (file)
--- a/clang/include/clang/Sema/ScopeInfo.h
+++ b/clang/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/clang/lib/Sema/ScopeInfo.cpp b/clang/lib/Sema/ScopeInfo.cpp
index c4bf67b..00d9982 100644 (file)
--- a/clang/lib/Sema/ScopeInfo.cpp
+++ b/clang/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/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 72fc3e3..aa6bf17 100644 (file)
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/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/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 1429df7..0104020 100644 (file)
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/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/clang/test/Sema/nonnull.c b/clang/test/Sema/nonnull.c
index 3b654a8..de47e96 100644 (file)
--- a/clang/test/Sema/nonnull.c
+++ b/clang/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);
+ }
+}
+