[Attribute] Fix noderef attribute false-negatives

[leonardchan]

Summary

noderef was failing to trigger warnings in some cases related to c++ style casting. This patch addresses them.

[rsmith]

clang/lib/Sema/SemaCast.cpp

280

Warning on this seems reasonable for static_cast and dynamic_cast, but should reinterpret_cast (or the reinterpret_cast interpretation of a C-style cast) warn? Presumably we want to leave open some type system holes for the case where the programmer really does know what they're doing.

clang/lib/Sema/SemaInit.cpp

8203–8210

Are there any static_cast cases that don't get here? I'm also a little surprised that static_cast would get here but the static_cast interpretation of a C-style or functional cast would not.

[aaron.ballman]

clang/test/Frontend/noderef.c

211 ↗

(On Diff #256428)

The name of the function is a bit odd given that there's an explicit cast.

[leonardchan]

clang/lib/Sema/SemaCast.cpp

280

My initial idea for this was that users could explicitly disable the warning with -Wno-noderef or pragmas for line-level granularity, but I can see how this could be less convenient/not consistent with other casts. Will update to allow C-style + reinterpret_casts.

clang/lib/Sema/SemaInit.cpp

8203–8210

I'll double check on this. The reason I added this here was because the cast in static_cast_from_same_ptr_type() triggered 2 of the warn_noderef_to_dereferenceable_pointer warnings and one of them was triggered here.

clang/test/Frontend/noderef.c

211 ↗

(On Diff #256428)

Woops. Will update the name.

[leonardchan]

Updated to address comments and added an RAII class for easier checking before exiting functions.

clang/lib/Sema/SemaInit.cpp

8203–8210

Yeah it seems the int *a = static_cast<int *>(x); case in cast_from_void_ptr() is an example of a static_cast that doesn't go through here.

[leonardchan]

Rebased

[rsmith]

clang/lib/Sema/SemaCast.cpp

750

Please use UpperCamelCase for local variables.

2593–2594

Should we be checking this for a C-style cast? You previously said you were going to turn off the checks for C-style casts. Did you change your mind on that?

3030

As above, should we be checking this for a C-style cast? (It also looks like we're doing this check twice in C++.)

clang/lib/Sema/SemaInit.cpp

8203–8210

I see, this is only reached for the part of static_cast that considers implicit conversion sequences. OK.

I expect there's similar overlap for all the other kinds of cast too. Should we be suppressing this check for *all* casts, given that we perform the check in SemaCast instead?

[leonardchan]

clang/lib/Sema/SemaCast.cpp

2593–2594

No, you're right. I accidentally left this in.

3030

Also accidentally left this in.

clang/lib/Sema/SemaInit.cpp

8203–8210

Hmm. I think for this particular location, static_cast contexts are all we need to suppress it for to prevent duplicate warnings from showing.

We don't need to suppress for these since the attribute is allowed to pass through these:

IC_CStyleCast
IC_FunctionalCast

We perform the check for implicit casts here since (AFAICT) only explicit casts (C-style, functional, and C++ style) seem to be covered in SemaCast:

IC_Implicit

This would cover cases like int *ptr = noderef_ptr;.

I'm not sure about these ones, but none of the cases I have seem to fail if I suppress only them:

IC_Normal
IC_ExplicitConvs

It could be though that there's a particular case I'm not covering in my tests.

[leonardchan]

*ping*

[aaron.ballman]

LGTM, but please wait for a few days in case @rsmith has further concerns.

clang/lib/Sema/SemaInit.cpp

8203–8209

The comment should end with a period.

[leonardchan]

Rebased

[leonardchan]

Lemme know if there's any more feedback. Will aim for committing this sometime at the end of the day.

clang/include/clang/Sema/Initialization.h

InitKind getKind() const {
     return Kind;
   }
 
   /// Determine whether this initialization is an explicit cast.
   bool isExplicitCast() const {
     return Context >= IC_StaticCast;
   }
 
+  /// Determine whether this initialization is a static cast.
+  bool isStaticCast() const { return Context == IC_StaticCast; }
+
   /// Determine whether this initialization is a C-style cast.
   bool isCStyleOrFunctionalCast() const {
     return Context >= IC_CStyleCast;
   }
 
   /// Determine whether this is a C-style cast.
   bool isCStyleCast() const {
     return Context == IC_CStyleCast;

clang/lib/Sema/SemaCast.cpp

void checkNonOverloadPlaceholders() {
         return;
 
       SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
       if (SrcExpr.isInvalid())
         return;
       PlaceholderKind = (BuiltinType::Kind) 0;
     }
   };
+
+  void CheckNoDeref(Sema &S, const QualType FromType, const QualType ToType,
+                    SourceLocation OpLoc) {
+    if (const auto *PtrType = dyn_cast<PointerType>(FromType)) {
+      if (PtrType->getPointeeType()->hasAttr(attr::NoDeref)) {
+        if (const auto *DestType = dyn_cast<PointerType>(ToType)) {
+          if (!DestType->getPointeeType()->hasAttr(attr::NoDeref)) {
+            S.Diag(OpLoc, diag::warn_noderef_to_dereferenceable_pointer);
+          }
+        }
+      }
+    }
+  }
+
+  struct CheckNoDerefRAII {
+    CheckNoDerefRAII(CastOperation &Op) : Op(Op) {}
+    ~CheckNoDerefRAII() {
+      if (!Op.SrcExpr.isInvalid())
+        CheckNoDeref(Op.Self, Op.SrcExpr.get()->getType(), Op.ResultType,
+                     Op.OpRange.getBegin());
+    }
+
+    CastOperation &Op;
+  };
 }
 
 static void DiagnoseCastQual(Sema &Self, const ExprResult &SrcExpr,
                              QualType DestType);
 
 // The Try functions attempt a specific way of casting. If they succeed, they
 // return TC_Success. If their way of casting is not appropriate for the given
 // arguments, they return TC_NotApplicable and *may* set diag to a diagnostic
Sema::ActOnCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,
   if (D.isInvalidType())
     return ExprError();
 
   if (getLangOpts().CPlusPlus) {
     // Check that there are no default arguments (C++ only).
     CheckExtraCXXDefaultArguments(D);
   }
 
   return BuildCXXNamedCast(OpLoc, Kind, TInfo, E,
                            SourceRange(LAngleBracketLoc, RAngleBracketLoc),
                            SourceRange(LParenLoc, RParenLoc));
 }
 
 ExprResult
 Sema::BuildCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,
                         TypeSourceInfo *DestTInfo, Expr *E,
                         SourceRange AngleBrackets, SourceRange Parens) {
static TryCastResult getCastAwayConstnessCastKind(CastAwayConstnessKind CACK,
 
   llvm_unreachable("unexpected cast away constness kind");
 }
 
 /// CheckDynamicCast - Check that a dynamic_cast\<DestType\>(SrcExpr) is valid.
 /// Refer to C++ 5.2.7 for details. Dynamic casts are used mostly for runtime-
 /// checked downcasts in class hierarchies.
 void CastOperation::CheckDynamicCast() {
+  CheckNoDerefRAII NoderefCheck(*this);
+
   if (ValueKind == VK_RValue)
     SrcExpr = Self.DefaultFunctionArrayLvalueConversion(SrcExpr.get());
   else if (isPlaceholder())
     SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
   if (SrcExpr.isInvalid()) // if conversion failed, don't report another error
     return;
 
   QualType OrigSrcType = SrcExpr.get()->getType();

 }
 
 /// CheckConstCast - Check that a const_cast\<DestType\>(SrcExpr) is valid.
 /// Refer to C++ 5.2.11 for details. const_cast is typically used in code
 /// like this:
 /// const char *str = "literal";
 /// legacy_function(const_cast\<char*\>(str));
 void CastOperation::CheckConstCast() {
+  CheckNoDerefRAII NoderefCheck(*this);
+
   if (ValueKind == VK_RValue)
     SrcExpr = Self.DefaultFunctionArrayLvalueConversion(SrcExpr.get());
   else if (isPlaceholder())
     SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
   if (SrcExpr.isInvalid()) // if conversion failed, don't report another error
     return;
 
   unsigned msg = diag::err_bad_cxx_cast_generic;
void CastOperation::CheckReinterpretCast() {
   }
 }
 
 
 /// CheckStaticCast - Check that a static_cast\<DestType\>(SrcExpr) is valid.
 /// Refer to C++ 5.2.9 for details. Static casts are mostly used for making
 /// implicit conversions explicit and getting rid of data loss warnings.
 void CastOperation::CheckStaticCast() {
+  CheckNoDerefRAII NoderefCheck(*this);
+
   if (isPlaceholder()) {
     checkNonOverloadPlaceholders();
     if (SrcExpr.isInvalid())
       return;
   }
 
   // This test is outside everything else because it's the only case where
   // a non-lvalue-reference target type does not lead to decay.
if (tcr == TC_NotApplicable) {
         return;
 
       if (tcr == TC_NotApplicable) {
         // ... and finally a reinterpret_cast, ignoring const and addr space.
         tcr = TryReinterpretCast(Self, SrcExpr, DestType, /*CStyle*/ true,
                                  OpRange, msg, Kind);
         if (SrcExpr.isInvalid())
           return;
       }
     }
   }
 
   if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
       isValidCast(tcr))
     checkObjCConversion(CCK);
 
   if (tcr != TC_Success && msg != 0) {
     if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
ExprResult Sema::BuildCStyleCastExpr(SourceLocation LPLoc,
   }
 
   if (Op.SrcExpr.isInvalid())
     return ExprError();
 
   // -Wcast-qual
   DiagnoseCastQual(Op.Self, Op.SrcExpr, Op.DestType);
 
   return Op.complete(CStyleCastExpr::Create(Context, Op.ResultType,
                               Op.ValueKind, Op.Kind, Op.SrcExpr.get(),
                               &Op.BasePath, CastTypeInfo, LPLoc, RPLoc));
 }
 
 ExprResult Sema::BuildCXXFunctionalCastExpr(TypeSourceInfo *CastTypeInfo,
                                             QualType Type,
                                             SourceLocation LPLoc,
                                             Expr *CastExpr,

clang/lib/Sema/SemaInit.cpp

for (step_iterator Step = step_begin(), StepEnd = step_end();
 
     case SK_ConversionSequence:
     case SK_ConversionSequenceNoNarrowing: {
       if (const auto *FromPtrType =
               CurInit.get()->getType()->getAs<PointerType>()) {
         if (const auto *ToPtrType = Step->Type->getAs<PointerType>()) {
           if (FromPtrType->getPointeeType()->hasAttr(attr::NoDeref) &&
               !ToPtrType->getPointeeType()->hasAttr(attr::NoDeref)) {
+            // Do not check static casts here because they are checked earlier
+            // in Sema::ActOnCXXNamedCast()
+            if (!Kind.isStaticCast()) {
               S.Diag(CurInit.get()->getExprLoc(),
                      diag::warn_noderef_to_dereferenceable_pointer)
                   << CurInit.get()->getSourceRange();
             }
           }
         }
+      }
 
       Sema::CheckedConversionKind CCK
         = Kind.isCStyleCast()? Sema::CCK_CStyleCast
         : Kind.isFunctionalCast()? Sema::CCK_FunctionalCast
         : Kind.isExplicitCast()? Sema::CCK_OtherCast
         : Sema::CCK_ImplicitConversion;
       ExprResult CurInitExprRes =
         S.PerformImplicitConversion(CurInit.get(), Step->Type, *Step->ICS,

clang/test/Frontend/noderef.cpp

void BlockPointers() {
   typedef int NODEREF (^IntBlock)(); // expected-warning{{'noderef' can only be used on an array or pointer type}}
 }
 
 class A {
 public:
   int member;
   int NODEREF *member2;
   int NODEREF member3; // expected-warning{{'noderef' can only be used on an array or pointer type}}
+  int *member4;
+
+  int func() { return member; }
+  virtual int func_virt() { return member; }
+
+  A(NODEREF int *x) : member4(x) {} // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
 };
 
+class Child : public A {};
+
 void MemberPointer() {
   int NODEREF A::*var = &A::member; // expected-warning{{'noderef' can only be used on an array or pointer type}}
 }
 
+int MethodCall(NODEREF A *a) { // expected-note{{a declared here}}
+  return a->func();            // expected-warning{{dereferencing a; was declared with a 'noderef' type}}
+}
+
+int ChildCall(NODEREF Child *child) { // expected-note{{child declared here}}
+  return child->func();               // expected-warning{{dereferencing child; was declared with a 'noderef' type}}
+}
+
 template <class Ty>
 class B {
   Ty NODEREF *member;
   Ty NODEREF member2; // expected-warning{{'noderef' can only be used on an array or pointer type}}
 };
 
 void test_lambdas() {
   auto l = [](int NODEREF *x){  // expected-note{{x declared here}}
     return *x;  // expected-warning{{dereferencing x; was declared with a 'noderef' type}}
   };
 }
 
 int NODEREF *glob_ptr;  // expected-note{{glob_ptr declared here}}
 int glob_int = *glob_ptr;  // expected-warning{{dereferencing glob_ptr; was declared with a 'noderef' type}}
+
+void cast_from_void_ptr(NODEREF void *x) {
+  int *a = static_cast<int *>(x);      // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+
+  // Allow regular C-style casts and C-style through reinterpret_casts to be holes
+  int *b = reinterpret_cast<int *>(x);
+  int *c = (int *)x;
+}
+
+void conversion_sequences() {
+  NODEREF int *x;
+  int *x2 = x;                     // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+  int *x3 = static_cast<int *>(x); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+  int *x4 = reinterpret_cast<int *>(x);
+
+  // Functional cast - This is exactly equivalent to a C-style cast.
+  typedef int *INT_PTR;
+  int *x5 = INT_PTR(x);
+
+  NODEREF Child *child;
+  Child *child2 = dynamic_cast<Child *>(child); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+int *static_cast_from_same_ptr_type(NODEREF int *x) {
+  return static_cast<int *>(x); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+A *dynamic_cast_up(NODEREF Child *child) {
+  return dynamic_cast<A *>(child); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+Child *dynamic_cast_down(NODEREF A *a) {
+  return dynamic_cast<Child *>(a); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+A *dynamic_cast_side(NODEREF A *a) {
+  return dynamic_cast<A *>(a); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+void *dynamic_cast_to_void_ptr(NODEREF A *a) {
+  return dynamic_cast<void *>(a); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+int *const_cast_check(NODEREF const int *x) {
+  return const_cast<int *>(x); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}
+
+const int *const_cast_check(NODEREF int *x) {
+  return const_cast<const int *>(x); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+}