[clang][noderef] Enable -Wnoderef warning for CXX C-style casts

[leonardchan]

Summary

We find it more valuable to warn on all instances we're removing the noderef attribute from a pointer. This patch removes the type holes where we initially didn't warn on C-style casts. For instances where removing noderef is intended, users can still disable the warning on line granularity with pragmas.

[aaron.ballman]

users can still disable the warning on line granularity with pragmas.

This makes me a bit uncomfortable because those pragmas extremely ugly (and not easily portable). Also, this will break code for users who were previously doing something that was explicitly allowed (but not explicitly documented, at least). How often do users need to (legitimately, rather than accidentally) use the C-style cast like this?

[leonardchan]

In D84202#2179618, @aaron.ballman wrote:

users can still disable the warning on line granularity with pragmas.

This makes me a bit uncomfortable because those pragmas extremely ugly (and not easily portable). Also, this will break code for users who were previously doing something that was explicitly allowed (but not explicitly documented, at least). How often do users need to (legitimately, rather than accidentally) use the C-style cast like this?

For fuchsia's case at least, we want to make it more difficult for users of this to explicitly remove the attribute (so adding dirty pragmas might help be a deterrent), and the only ideal case this would be stripped is in tests. For now, we do purposely have C-style casts scattered about since we're incrementally turning on this warning for various drivers, but we're getting closer to remove all these casts in non-testing driver code.

I can't speak for other projects use cases though. Would it help to send out an RFC to see if any other users have strong opinions?

[aaron.ballman]

In D84202#2179718, @leonardchan wrote:

In D84202#2179618, @aaron.ballman wrote:

users can still disable the warning on line granularity with pragmas.

This makes me a bit uncomfortable because those pragmas extremely ugly (and not easily portable). Also, this will break code for users who were previously doing something that was explicitly allowed (but not explicitly documented, at least). How often do users need to (legitimately, rather than accidentally) use the C-style cast like this?

For fuchsia's case at least, we want to make it more difficult for users of this to explicitly remove the attribute (so adding dirty pragmas might help be a deterrent), and the only ideal case this would be stripped is in tests. For now, we do purposely have C-style casts scattered about since we're incrementally turning on this warning for various drivers, but we're getting closer to remove all these casts in non-testing driver code.

Good to know. And yeah, the pragma may be a good deterrent, but I was trying to understand whether the casts are *necessary* in some circumstances (e.g., are we forcing users to write these pragmas for correct code?). It sounds like the answer is that the casts aren't necessary?

I can't speak for other projects use cases though. Would it help to send out an RFC to see if any other users have strong opinions?

It could help, but I'm not ready to insist yet. I'm mostly worried that this attribute has been available for 2+ years and I have no idea how widely used it is and what the usage patterns are. If you have evidence to suggest that this shouldn't be disruptive and that altering code to fit the new restriction is easy, that would help me feel more comfortable too.

clang/lib/Sema/SemaCast.cpp

void checkNonOverloadPlaceholders() {
       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 (const auto *PtrType = FromType->getAs<PointerType>()) {
       if (PtrType->getPointeeType()->hasAttr(attr::NoDeref)) {
-        if (const auto *DestType = dyn_cast<PointerType>(ToType)) {
+        if (const auto *DestType = ToType->getAs<PointerType>()) {
           if (!DestType->getPointeeType()->hasAttr(attr::NoDeref)) {
             S.Diag(OpLoc, diag::warn_noderef_to_dereferenceable_pointer);
           }
         }
       }
     }
   }
 

 }
 
 /// CheckReinterpretCast - Check that a reinterpret_cast\<DestType\>(SrcExpr) is
 /// valid.
 /// Refer to C++ 5.2.10 for details. reinterpret_cast is typically used in code
 /// like this:
 /// char *bytes = reinterpret_cast\<char*\>(int_ptr);
 void CastOperation::CheckReinterpretCast() {
+  CheckNoDerefRAII NoderefCheck(*this);
+
   if (ValueKind == VK_RValue && !isPlaceholder(BuiltinType::Overload))
     SrcExpr = Self.DefaultFunctionArrayLvalueConversion(SrcExpr.get());
   else
     checkNonOverloadPlaceholders();
   if (SrcExpr.isInvalid()) // if conversion failed, don't report another error
     return;
 
   unsigned msg = diag::err_bad_cxx_cast_generic;
while (isa<PointerType>(DestPtr) && isa<PointerType>(SrcPtr)) {
       DiagID = diag::ext_nested_pointer_qualifier_mismatch;
     }
   }
 }
 
 void CastOperation::CheckCXXCStyleCast(bool FunctionalStyle,
                                        bool ListInitialization) {
   assert(Self.getLangOpts().CPlusPlus);
+  CheckNoDerefRAII NoderefCheck(*this);
 
   // Handle placeholders.
   if (isPlaceholder()) {
     // C-style casts can resolve __unknown_any types.
     if (claimPlaceholder(BuiltinType::UnknownAny)) {
       SrcExpr = Self.checkUnknownAnyCast(DestRange, DestType,
                                          SrcExpr.get(), Kind,
                                          ValueKind, BasePath);

clang/test/Frontend/noderef.cpp

 }
 
 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;
+  int *b = reinterpret_cast<int *>(x); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
+  int *c = (int *)x; // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
 }
 
 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);
+  int *x5 = INT_PTR(x); // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
 
   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}}
 }