Disallow dereferencing of void* in C++.

[erichkeane]

Summary

as Discussed:
https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708

There is no good reason to allow this when the other compilers all
reject this, and it messes with SFINAE/constraint checking.

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6941

Bikeshedding appreciated here :) Note that the diagnostic ALSO will show up in SFINAE/Concepts explanations, so we can't just say "will be removed" or something.

[aaron.ballman]

Adding the vendors group due to the potential to break existing code.

[cor3ntin]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6941

The usual terminology is dereferencing. "ISO C++ does not allow dereferencing a void*`"

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6941

Yeah, I thought the 'indirection' phrasing in the diagnostic above was strange as well, but figured I'd be consistent until someone came up with a good replacement here :)

[jrtc27]

What about __typeof__(*p)?

[erichkeane]

In D135287#3837584, @jrtc27 wrote:

What about __typeof__(*p)?

Yes, that would also be an error in C++, as it is on all other compilers.

[shafik]

LGTM

clang/docs/ReleaseNotes.rst

92

[erichkeane]

fix the release note spelling of permanent.

[aaron.ballman]

In D135287#3837617, @erichkeane wrote:

In D135287#3837584, @jrtc27 wrote:

What about __typeof__(*p)?

Yes, that would also be an error in C++, as it is on all other compilers.

This actually relates to a use case I brought up in the RFC -- in an unevaluated context where the expression is only used to determine a type, it doesn't seem particularly harmful to allow the dereference. There might be generic programming cases where this comes up for people who exclusively use Clang, but I'm not 100% certain.

clang/docs/ReleaseNotes.rst

89–98

Wordsmithing

clang/include/clang/Basic/DiagnosticSemaKinds.td

6938–6939

We no longer use this diagnostic in C++, so might as well simplify it.

6942–6945

Based on the code search for people using that diagnostic flag, I don't think we need to do this -- it seems like only a very small number of projects disable that warning (at least from a code search on sourcegraph). We still need a separate diagnostic (because of the DefaultError, but I think we can re-use the old diagnostic group. WDYT?

6945–6947

clang/test/SemaCXX/disallow_void_deref.cpp

2

You should also add a RUN line showing the behavior when the diagnostic is not an error and is disabled entirely.

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6942–6945

SGTM. I'm not attached to it.

6945–6947

This is actually the result of clang-format! I'll undo it.

[erichkeane]

Fix based on Aaron's comments.

clang/docs/ReleaseNotes.rst

 - Clang now disallows types whose sizes aren't a multiple of their alignments
   to be used as the element type of arrays.
 
   .. code-block:: c
 
   typedef char int8_a16 __attribute__((aligned(16)));
   int8_a16 array[4]; // Now diagnosed as the element size not being a multiple of the array alignment.
 
 - When compiling for Windows in MSVC compatibility mode (for example by using
   clang-cl), the compiler will now propagate dllimport/export declspecs in
   explicit specializations of class template member functions (`Issue 54717
   <https://github.com/llvm/llvm-project/issues/54717>`_):
 
   .. code-block:: c++
 
     template <typename> struct __declspec(dllexport) S {
       void f();
     };
     template<> void S<int>::f() {}  // clang-cl will now dllexport this.
 
   This matches what MSVC does, so it improves compatibility, but it can also
   cause errors for code which clang-cl would previously accept, for example:
 
   .. code-block:: c++
 
     template <typename> struct __declspec(dllexport) S {
       void f();
     };
     template<> void S<int>::f() = delete;  // Error: cannot delete dllexport function.
 
   .. code-block:: c++
 
     template <typename> struct __declspec(dllimport) S {
       void f();
     };
     template<> void S<int>::f() {};  // Error: cannot define dllimport function.
 
   These errors also match MSVC's behavior.
 
+- Clang now diagnoses indirection of ``void *`` in C++ mode as a warning which
+  defaults to an error. This is compatible with ISO C++, GCC, ICC, and MSVC. This
+  is also now a SFINAE error so constraint checking and SFINAE checking can be
+  compatible with other compilers. It is expected that this will be upgraded to
+  an error-only diagnostic in the next Clang release.
+
+  .. code-block:: c++
+
+  void func(void *p) {
+    *p; // Now diagnosed as a warning-as-error.
+  }
+
 What's New in Clang |release|?
 ==============================
 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 Major New Features

clang/include/clang/Basic/DiagnosticGroups.td

def IntToPointerCast : DiagGroup<"int-to-pointer-cast",
                                  [IntToVoidPointerCast]>;
 def VoidPointerToEnumCast : DiagGroup<"void-pointer-to-enum-cast">;
 def VoidPointerToIntCast : DiagGroup<"void-pointer-to-int-cast",
                                      [VoidPointerToEnumCast]>;
 def PointerToEnumCast : DiagGroup<"pointer-to-enum-cast",
                                   [VoidPointerToEnumCast]>;
 def PointerToIntCast : DiagGroup<"pointer-to-int-cast",
                                  [PointerToEnumCast, VoidPointerToIntCast]>;
+def VoidPointerDeref : DiagGroup<"void-ptr-dereference">;
 
 def FUseLdPath : DiagGroup<"fuse-ld-path">;
 
 def Move : DiagGroup<"move", [
     PessimizingMove,
     RedundantMove,
     ReturnStdMove,
     SelfMove

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_addrof_temporary : Error<
   "taking the address of a temporary object of type %0">;
 def err_typecheck_addrof_dtor : Error<
   "taking the address of a destructor">;
 def err_typecheck_unary_expr : Error<
   "invalid argument type %0 to unary expression">;
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<
-  "ISO %select{C|C++}0 does not allow indirection on operand of type %1">,
-  InGroup<DiagGroup<"void-ptr-dereference">>;
+  "ISO C does not allow indirection on operand of type %0">,
+  InGroup<VoidPointerDeref>;
+def ext_typecheck_indirection_through_void_pointer_cpp
+    : ExtWarn<"ISO C++ does not allow indirection on operand of type %0">,
+      InGroup<VoidPointerDeref>, DefaultError, SFINAEFailure;
 def warn_indirection_through_null : Warning<
   "indirection of non-volatile null pointer will be deleted, not trap">,
   InGroup<NullDereference>;
 def warn_binding_null_to_reference : Warning<
   "binding dereferenced null pointer to reference has undefined behavior">,
   InGroup<NullDereference>;
 def note_indirection_through_null : Note<
   "consider using __builtin_trap() or qualifying pointer with 'volatile'">;
 def warn_pointer_indirection_from_incompatible_type : Warning<
   "dereference of type %1 that was reinterpret_cast from type %0 has undefined "
   "behavior">,
   InGroup<UndefinedReinterpretCast>, DefaultIgnore;

clang/lib/Sema/SemaExpr.cpp

if (Result.isNull()) {
     return QualType();
   }
 
   if (Result->isVoidType()) {
     // C++ [expr.unary.op]p1:
     //   [...] the expression to which [the unary * operator] is applied shall
     //   be a pointer to an object type, or a pointer to a function type
     LangOptions LO = S.getLangOpts();
-    if (LO.CPlusPlus || (!(LO.C99 && IsAfterAmp) && !S.isUnevaluatedContext()))
+    if (LO.CPlusPlus)
+      S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer_cpp)
+          << OpTy << Op->getSourceRange();
+    else if (!(LO.C99 && IsAfterAmp) && !S.isUnevaluatedContext())
       S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer)
-          << LO.CPlusPlus << OpTy << Op->getSourceRange();
+          << OpTy << Op->getSourceRange();
   }
 
   // Dereferences are usually l-values...
   VK = VK_LValue;
 
   // ...except that certain expressions are never l-values in C.
   if (!S.getLangOpts().CPlusPlus && Result.isCForbiddenLValueType())
     VK = VK_PRValue;

clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 // Test instantiation of member functions of class templates defined out-of-line
 template<typename T, typename U>
 struct X0 {
   void f(T *t, const U &u);
   void f(T *);
 };
 
 template<typename T, typename U>
 void X0<T, U>::f(T *t, const U &u) {
-  *t = u; // expected-warning{{indirection on operand of type 'void *'}} expected-error{{not assignable}}
+  *t = u; // expected-error{{indirection on operand of type 'void *'}} expected-error{{not assignable}}
 }
 
 void test_f(X0<float, int> xfi, X0<void, int> xvi, float *fp, void *vp, int i) {
   xfi.f(fp, i);
   xvi.f(vp, i); // expected-note{{instantiation}}
 }

clang/test/SemaCXX/disallow_void_deref.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify=enabled,sfinae -std=c++20 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=sfinae -std=c++20 -Wno-void-ptr-dereference %s
+
+void f(void* p) {
+  (void)*p; // enabled-error{{ISO C++ does not allow indirection on operand of type 'void *'}}
+}
+
+template<class T>
+concept deref = requires (T& t) {
+      { *t }; // #FAILED_REQ
+};
+
+static_assert(deref<void*>);
+// sfinae-error@-1{{static assertion failed}}
+// sfinae-note@-2{{because 'void *' does not satisfy 'deref'}}
+// sfinae-note@#FAILED_REQ{{because '*t' would be invalid: ISO C++ does not allow indirection on operand of type 'void *'}}

clang/test/SemaCXX/reinterpret-cast.cpp

void dereference_reinterpret_cast() {
   // Casting from void pointer.
   (void)*reinterpret_cast<A*>(v_ptr);
   (void)*reinterpret_cast<B*>(v_ptr);
   (void)*reinterpret_cast<long*>(v_ptr);
   (void)*reinterpret_cast<double*>(v_ptr);
   (void)*reinterpret_cast<float*>(v_ptr);
 
   // Casting to void pointer
-  (void)*reinterpret_cast<void*>(&a); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast<void*>(&b); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast<void*>(&l); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast<void*>(&d); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast<void*>(&f); // expected-warning {{ISO C++ does not allow}}
+  (void)*reinterpret_cast<void*>(&a); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast<void*>(&b); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast<void*>(&l); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast<void*>(&d); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast<void*>(&f); // expected-error {{ISO C++ does not allow}}
 }
 
 void reinterpret_cast_allowlist () {
   // the dynamic type of the object
   int a;
   float b;
   (void)reinterpret_cast<int&>(a);
   (void)*reinterpret_cast<int*>(&a);