r220496 - patch to issue warning on comparing parameters with

[Richard Trieu]

Sorry for the duplicate email.  Once more, with the list this time.

On Thu, Oct 23, 2014 at 12:00 PM, Fariborz Jahanian <fjahanian at apple.com>
wrote:

> Author: fjahanian
> Date: Thu Oct 23 14:00:10 2014
> New Revision: 220496
>
> URL: http://llvm.org/viewvc/llvm-project?rev=220496&view=rev
> Log:
> patch to issue warning on comparing parameters with
> nonnull attribute when comparison is always
> true/false. Patch by Steven Wu with few fixes and minor
> refactoring and adding tests by me. rdar://18712242
>
> Modified:
>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>     cfe/trunk/lib/Sema/SemaChecking.cpp
>     cfe/trunk/test/Sema/nonnull.c
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=220496&r1=220495&r2=220496&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Oct 23
> 14:00:10 2014
> @@ -2500,6 +2500,10 @@ def warn_impcast_pointer_to_bool : Warni
>      "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 always evaluate to "
> +    "'true'">,
> +    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>;
> @@ -2512,6 +2516,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 always %select{true|false}1">,
> +    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">,
>
> Why are two new diagnostic messages added here?  The text is nearly
identical, so it should work by adding a new choice to the %select in the
existing warning message.

> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=220496&r1=220495&r2=220496&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Oct 23 14:00:10 2014
> @@ -6678,7 +6678,37 @@ void Sema::DiagnoseAlwaysNonNullPointer(
>    // Weak Decls can be null.
>    if (!D || D->isWeak())
>      return;
> -
>
Move the non-null parameter check to a helper function which takes a
ParmVarDecl as a parameter and use early exit to prevent extra work.

> +  // Check for parameter decl with nonnull attribute
> +  if (ParmVarDecl* PV = dyn_cast<ParmVarDecl>(D)) {
> +    if (FunctionDecl* FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
> +      unsigned NumArgs = FD->getNumParams();
> +      llvm::SmallBitVector AttrNonNull(NumArgs);
>
BitVector not needed in new function.  Access to AttrNonNull are turned in
returns.

> +      for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
> +        if (!NonNull->args_size()) {
> +          AttrNonNull.set(0, NumArgs);
>
Early exit here.

> +          break;
> +        }
> +        for (unsigned Val : NonNull->args()) {
> +          if (Val >= NumArgs)
> +            continue;
> +          AttrNonNull.set(Val);
>
Use Val to get the ParmVarDecl from the FunctionDecl, which can be compared
to ParmVarDecl and return on equality.

> +        }
> +      }
> +      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();
>
> Once the new warning and the old warning are folded together, use the
current Diag call instead having a new one here.

> Modified: cfe/trunk/test/Sema/nonnull.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/nonnull.c?rev=220496&r1=220495&r2=220496&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Sema/nonnull.c (original)
> +++ cfe/trunk/test/Sema/nonnull.c Thu Oct 23 14:00:10 2014
> @@ -83,3 +83,28 @@ 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 always false}}
> +     return 0;
> +   } else {
> +     return *pointer;
> +   }
> +
> +   if (pv == NULL) {} // expected-warning {{comparison of nonnull
> parameter 'pv' equal to a null pointer is always false}}
> +}
> +
> +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 always false}}
> +     return 0;
> +   } else {
> +     return *pointer;
> +   }
> +
> +   if (pv == NULL) {} // expected-warning {{comparison of nonnull
> parameter 'pv' equal to a null pointer is always false}}
> +}
>
> Add a test which has multiple pointer parameters, only a few which are
marked non-null.  Test that the warning only fires on the non-null pointer
parameters.

>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141023/95049191/attachment.html>