[Clang][Diagnostic] Add hidden-reinterpret-cast diagnostic warning

[TWeaver]

Summary

There's an argument to be made that C++ functional casts are just as dangerous as old style C casts. They have the ability to perform reinterpret casts, causing potentially dangerous bugs in software. On top of this, C++ functional style casts have the same syntactic 'look' as a constructor call. This has the potential, especially when dealing with template code, for a hidden reinterpret cast to be embedded in code without an author realising.

This patch adds a new diagnostic and warning flag (-Whidden-reinterpret-cast) that will fire whenever a C++ functional style cast fails a static cast check but succeeds a reinterpret cast check. This should help identify unintentional reinterpret casts in a codebase.

This work was partly inspired by the following blog post:
https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/

The template test in this change takes inspiration from the first example in said blogpost.

[TWeaver]

Fixed pre-merge issues:
updated hidden-reinterpret-test types to long longs to fix windows clang compile error.
ran clang-format over changed lines in SemaCast.cpp

[aaron.ballman]

Thank you for the patch!

We don't typically add new diagnostics that are ignored by default because we have significant evidence that users don't enable them enough to warrant adding them.

In this case, I don't think we should enable this by default because it's effectively a "you're using the language feature" kind of diagnostic rather than pointing out inherently incorrect code. However, if there are situations where it points out undefined behavior were weren't diagnosing and we can limit the diagnostic to those cases, I think that would be great to enable by default. Alternatively, I think this would make sense as a clang-tidy check rather than part of the compiler itself as it feels more like a stylistic diagnostic than a correctness one.

[lebedev.ri]

Looks like this and more is already being caught by clang-tidy's google-readability-casting check.

[TWeaver]

Hello all,

First of all, thank you so much for your reviews, comments and time.

Secondly, I apologise for the late reply.

I've be thinking about your statements and I have to somewhat agree. This doesn't catch undefined behaviour, at least by its self. It may help catch undefined behaviour as a result of 'bad-code', but it's not going to catch UB as a result of the cast in every case. The clang-tidy checks are fine but they don't actually drill down to the functionality I'm looking for, aka, "this functional cast is a reinterpret_cast - beware!" (missing from this patch is a suggestion to write 'reinterpret_cast<T>(...)' instead).

Also, I'm aware that there's a warning for picking up old style c casts in the compiler, enabled by providing '-Wold-style-cast' on the command line. Are old style c casts not also language features? If that's the case why is its diagnostic embedded in the compiler and not in clang-tidy?

I ask this because there's an argument to be made that C++ style functional casts are just as dangerous as old-style casts. They can produce reinterpret casts when used in the right (or wrong) context. Would it be possible to at least add a diagnostic for detecting any functional style casts instead?

$ clang.exe .\old-style-test.cpp -Wold-style-cast
.\old-style-test.cpp:3:11: warning: use of old-style cast [-Wold-style-cast]
  int y = (double)x;
          ^  

Thanks again for your time

[TWeaver]

Hi all,

thanks for your time and effort reviewing this patch.

I'm going to close this now due to the lack of interest in it's current form.

thanks again and happy coding,
Tom

clang/include/clang/Basic/DiagnosticGroups.td

 def InvalidPPToken : DiagGroup<"invalid-pp-token">;
 def Trigraphs      : DiagGroup<"trigraphs">;
 
+def HiddenReinterpretCast : DiagGroup<"hidden-reinterpret-cast">;
 def UndefinedReinterpretCast : DiagGroup<"undefined-reinterpret-cast">;
 def ReinterpretBaseClass : DiagGroup<"reinterpret-base-class">;
 def Unicode  : DiagGroup<"unicode">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "use 'static_cast' to adjust the pointer correctly while "
   "%select{upcasting|downcasting}0">;
 
+def warn_cxx_functional_style_cast_hidden_reinterpret_cast: Warning<
+  "C++ functional style cast may be a hidden reinterpret cast">,
+  InGroup<HiddenReinterpretCast>, DefaultIgnore;
+
 def err_bad_static_cast_overload : Error<
   "address of overloaded function %0 cannot be static_cast to type %1">;
 

clang/lib/Sema/SemaCast.cpp

         // ... and finally a reinterpret_cast, ignoring const and addr space.
         tcr = TryReinterpretCast(Self, SrcExpr, DestType, /*CStyle*/ true,
                                  OpRange, msg, Kind);
+
+        // If this CXX Functional cast is a reinterpret cast then we have a
+        // potentially hidden reinterpret cast here.
+        if (isValidCast(tcr)) {
+          Self.Diag(
+              OpRange.getBegin(),
+              diag::warn_cxx_functional_style_cast_hidden_reinterpret_cast);
+        }
+
         if (SrcExpr.isInvalid())
           return;
       }

clang/test/SemaCXX/hidden_reinterpret_test.cpp

+// Check that various conversions from integral to pointer to references
+// generate an appropriate hidden-reinterpret-cast diagnostic.
+
+// RUN: %clang_cc1 -verify %s -Whidden-reinterpret-cast
+
+class castee {
+public:
+  int i;
+};
+
+class caster {
+public:
+  caster(castee tbc):
+    i(tbc.i) {}
+private:
+  int i;
+};
+
+int main() {
+  using long_def = long long;
+  using long_ptr = long long*;
+  using long_ref = long long&;
+
+  // Check pointer to int conversion for reinterpret warning.
+  long_ptr ptr = 0x0;
+  long_def new_num = long_def(ptr); // expected-warning {{C++ functional style cast may be a hidden reinterpret cast}}
+
+  // Check int to pointer conversion for reinterpret warning.
+  long_def strange_address  = 0xDEADBEEF;
+  long_ptr strange_ptr = long_ptr(strange_address); // expected-warning {{C++ functional style cast may be a hidden reinterpret cast}}
+
+  // Check pointer to reference conversion for reinterpret warning.
+  long_ptr ptr_to_ref = 0x0;
+  long_ref ref_of_ptr = long_ref(ptr_to_ref); // expected-warning {{C++ functional style cast may be a hidden reinterpret cast}}
+
+  // Check reference to pointer conversion, should not provide a warning
+  long_def to_ref = 50;
+  long_ref ref_to_ptr = to_ref;
+  // "long_ptr(ptr_from_ref)" is synonymous with "long_ptr(&to_ref)" and
+  // should not generate a warning.
+  long_ptr ptr_from_ref = long_ptr(ptr_from_ref);
+
+  // implicit conversions and therefore static_casts, should not generate
+  // warnings.
+  float f = 1.5f;
+  int   i = int(f);
+
+  unsigned long int uli = 50;
+  char                c = char(uli);
+
+  long long int   value = 50;
+  long long int   &iref = value;
+  long long int iNotRef = long_def(iref);
+
+  int         small = 500;
+  long long int big = long_def(small);
+
+  // class conversion test, this will call the constructor of to_static_cast
+  // shouldn't generate a warning.
+  castee c1;
+  caster c2 = caster(c1);
+
+  return 0;
+}

clang/test/SemaCXX/hidden_reinterpret_test_template.cpp

+// RUN: %clang_cc1 -verify %s -Whidden-reinterpret-cast
+
+template <class T>
+T find(T obj[], T key, unsigned long int size) {
+  using X = decltype(obj[0]);
+  for (T *ix = 0; ix != obj+size; ++ix) {
+    if ((*ix) == key)
+      return X(ix); // expected-warning {{C++ functional style cast may be a hidden reinterpret cast}}
+  }
+  return X(key);
+}
+
+int main(const int argc, const char *argv[]) {
+  int array[] = {0, 1, 2, 3};
+  int *array_ptrs[] = {&(array[0]), &(array[1]), &(array[2]), &(array[3])};
+  int *key = &(array[2]);
+  find<int*>(array_ptrs, key, sizeof(array_ptrs));   // expected-note {{in instantiation of function template specialization 'find<int *>' requested here}}
+  return *key;
+}