Make dereferencing a void* a hard-error instead of warn-as-error

[erichkeane]

Summary

Clang 16 changed to consider dereferencing a void* to be a
warning-as-error, plus made this an error in SFINAE contexts, since this
resulted in incorrect template instantiation. When doing so, the Clang
16 documentation was updated to reflect that this was likely to change
again to a non-disablable error in the next version.

As there has been no response to changing from a warning to an error, I
believe this is a non-controversial change.

This patch changes this to be an Error, consistent with the standard and
other compilers.

This was discussed in this RFC:
https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708

[rupprecht]

I found one use here: https://github.com/ericniebler/range-v3/blob/48dc2eb666c07e6afc8ec5edf7db9a5c6cde7a56/include/range/v3/functional/invoke.hpp#L51

[shafik]

LGTM

[rnk]

Can you expand on the motivation for removing this extension? This doesn't seem to save a lot of code complexity, and it increases migration costs for our users. I'd like to have some motivation for putting them through the trouble of migrating.

[erichkeane]

In D150875#4353209, @rupprecht wrote:

I found one use here: https://github.com/ericniebler/range-v3/blob/48dc2eb666c07e6afc8ec5edf7db9a5c6cde7a56/include/range/v3/functional/invoke.hpp#L51

Huh, that is an interesting use! I notice that he enables that for both GCC AND Clang, but probably doesn't notice because he disables Wpragma as well. So this change would change the legality of his program (potentially) for Clang (though, we ALREADY would have broken any SFINAE/concepts uses because of the SFINAE change in 16), but only to be what GCC gives him? I think we can be ok with that?

[erichkeane]

In D150875#4353358, @rnk wrote:

Can you expand on the motivation for removing this extension? This doesn't seem to save a lot of code complexity, and it increases migration costs for our users. I'd like to have some motivation for putting them through the trouble of migrating.

The motivation is essentially the same motivation for making this a SFINAE error: We are the only of the major compilers with this "extension" (I hesitate to call it that, as I'm not sure this FITS in the 'extension's permitted by standard), and we gave warning last release in the release notes that we'd be doing this. You're right there is no additional code complexity, but there IS standards compliance issues, as well as "do what everyone else does".

I WILL say the SFINAE change last release was more significant as a motivation, since it had the ability to change the overload set.

[aaron.ballman]

In D150875#4353358, @rnk wrote:

Can you expand on the motivation for removing this extension? This doesn't seem to save a lot of code complexity, and it increases migration costs for our users. I'd like to have some motivation for putting them through the trouble of migrating.

Note, there was an RFC: https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708

[rnk]

In D150875#4353398, @aaron.ballman wrote:

Note, there was an RFC: https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708

Thanks, that addresses my concern. Please link to it from the commit message, it will help anyone bisecting the behavior change have context.

[erichkeane]

In D150875#4353435, @rnk wrote:

In D150875#4353398, @aaron.ballman wrote:

Note, there was an RFC: https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708

Thanks, that addresses my concern. Please link to it from the commit message, it will help anyone bisecting the behavior change have context.

Will do! I've put it in my local git, and will put it in phab as well.

[jrtc27]

We heavily rely on this extension in CheriBSD via __typeof__((*(p))) * __capability as we want to be able to take any pointer, including to an array or function that needs to undergo decay to be an actual pointer, and turn it into a __capability-qualified one. Presumably you're saying we should instead use __typeof__((0, (p))) __capability as an uglier alternative way to force decay?

[erichkeane]

In D150875#4353467, @jrtc27 wrote:

We heavily rely on this extension in CheriBSD via __typeof__((*(p))) * __capability as we want to be able to take any pointer, including to an array or function that needs to undergo decay to be an actual pointer, and turn it into a __capability-qualified one. Presumably you're saying we should instead use __typeof__((0, (p))) __capability as an uglier alternative way to force decay?

Do you do that in C++, or just C? Note that this does NOT change the behavior in C. In C++ I'd probably just suggest using std::decay.

[jrtc27]

In D150875#4353484, @erichkeane wrote:

In D150875#4353467, @jrtc27 wrote:

We heavily rely on this extension in CheriBSD via __typeof__((*(p))) * __capability as we want to be able to take any pointer, including to an array or function that needs to undergo decay to be an actual pointer, and turn it into a __capability-qualified one. Presumably you're saying we should instead use __typeof__((0, (p))) __capability as an uglier alternative way to force decay?

Do you do that in C++, or just C? Note that this does NOT change the behavior in C. In C++ I'd probably just suggest using std::decay.

In practice just C (kernel-only macro), though ideally the macro would still work in C++ (at least on CheriBSD; C++-based OSes will differ, but can of course do their own thing).

[rsmith]

In D150875#4353384, @erichkeane wrote:

We are the only of the major compilers with this "extension" (I hesitate to call it that, as I'm not sure this FITS in the 'extension's permitted by standard)

I'm not objecting to removing this extension, but... do you have reason to doubt that it's conforming, or just a lack of confidence that it is? (If the SFINAEFailure change wasn't enough, then it's not clear to me why this change would be. We use a SFINAEFailure diagnostic for other extensions, and if that's not sufficient for conformance then we probably have a lot of conformance gaps of this kind.)

clang/include/clang/Basic/DiagnosticSemaKinds.td

6985

We normally only use this "ISO C++ does not allow" phrasing for extensions (with the implication being that ISO C++ doesn't allow it, but Clang does). Can you rephrase the diagnostic too, to remove those unnecessary words?

[erichkeane]

In D150875#4353535, @rsmith wrote:

In D150875#4353384, @erichkeane wrote:

We are the only of the major compilers with this "extension" (I hesitate to call it that, as I'm not sure this FITS in the 'extension's permitted by standard)

I'm not objecting to removing this extension, but... do you have reason to doubt that it's conforming, or just a lack of confidence that it is? (If the SFINAEFailure change wasn't enough, then it's not clear to me why this change would be. We use a SFINAEFailure diagnostic for other extensions, and if that's not sufficient for conformance then we probably have a lot of conformance gaps of this kind.)

No, just lack of confidence. I guess if it is supposed to be ill-formed, we can permit it (except in SFINAE?), right? The intent here is mostly just for 'cleanup' as we see it.

clang/include/clang/Basic/DiagnosticSemaKinds.td

6985

I definitely can. Though, I wonder if we can just use the err_typecheck_indirection_requires_pointer above instead and remove this diagnostic? We might need to change that to indirection requires non-void pointer operand (%0 invalid). WDYT?

[erichkeane]

Ok, so I was curious why this was an extension. This is the original patch: https://github.com/llvm/llvm-project/commit/80877c228d019e9cdca6295f3197867cc86e1720 where @rsmith mentions there is no good reason for this limitation, then mentions a core issue having been filed. I found CWG1923 (https://open-std.org/JTC1/SC22/WG21/docs/cwg_closed.html#1923), which is said issue. The resolution is "Needs an EWG Paper", which seems to be the end of the line, as I see no such paper?

[aaron.ballman]

In D150875#4353658, @erichkeane wrote:

Ok, so I was curious why this was an extension. This is the original patch: https://github.com/llvm/llvm-project/commit/80877c228d019e9cdca6295f3197867cc86e1720 where @rsmith mentions there is no good reason for this limitation, then mentions a core issue having been filed. I found CWG1923 (https://open-std.org/JTC1/SC22/WG21/docs/cwg_closed.html#1923), which is said issue. The resolution is "Needs an EWG Paper", which seems to be the end of the line, as I see no such paper?

Because the Core issue ended up in extension territory and no extension paper came out, and because we don't know there's evidence of a significant user community, I think it's reasonable to remove this extension (it doesn't match our usual requirements for adding extensions). However, if @rsmith is planning to write such a paper (or if there is a paper and we just haven't spotted it), then that's a different story.

[MaskRay]

clang/docs/ReleaseNotes.rst

59

Mention -Wvoid-ptr-dereference?

Clang 16 converted this to a default-error -Wvoid-ptr-dereference as well as a SFINAE error.

Does this patch intentionally keep -Wvoid-ptr-dereference (which is a no-op now)?

[erichkeane]

clang/docs/ReleaseNotes.rst

59

Good suggestions!

Yes, Clang keeps Wvoid-ptr-dereference because it is still a valid flag for C.

[erichkeane]

Update release note per @MaskRay , change error message per @rsmith

[aaron.ballman]

Precommit CI found various failures that need to be addressed. Also, have you tested to see what breaks in llvm-test-suite?

clang/docs/ReleaseNotes.rst

58

[erichkeane]

In D150875#4362434, @aaron.ballman wrote:

Precommit CI found various failures that need to be addressed. Also, have you tested to see what breaks in llvm-test-suite?

Woops! I must have forgotten to re-make my diff before submitting. I fixed those in the patch earlier.

I have not tested this against the llvm-test-suite as I'm not thrilled having to figure out how to build that again :) But I guess I should make a companion patch ahead of time. Last time however I don't recall us needing to add the flag for the warning however.

[erichkeane]

Fix tests + Aaron's formatting request. Guess I forgot to upload this last night!

[erichkeane]

I did the llvm-test-suite build, and have a handful of linker issues (because I've not built libclang, and don't have some newer GCC library for some float16 commands), but everythign builds at least. If necessary, I can try to make sure it all builds, but I hope/suspect this is sufficient.

[aaron.ballman]

Fantastic! Presuming precommit CI comes back green this time, LGTM.

clang/docs/ReleaseNotes.rst

- Indirect edges of asm goto statements under certain circumstances may now be
   .. code-block:: c
 
     foo: asm goto ("# %0 %1"::"i"(&&foo)::foo);
 
 C++ Specific Potentially Breaking Changes
 -----------------------------------------
 - Clang won't search for coroutine_traits in std::experimental namespace any more.
   Clang will only search for std::coroutine_traits for coroutines then.
+- Clang no longer allows dereferencing of a ``void*`` as an extension. Clang 16
+  converted this to a default-error as ``-Wvoid-ptr-dereference``, as well as a
+  SFINAE error. This flag is still valid however, as it disables the equivalent
+  warning in C.
 
 ABI Changes in This Version
 ---------------------------
 
 
 What's New in Clang |release|?
 ==============================
 Some of the major new features and improvements to Clang are listed

clang/include/clang/Basic/DiagnosticSemaKinds.td

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 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 err_typecheck_indirection_through_void_pointer_cpp
+    : Error<"indirection not permitted on operand of type %0">;
 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'">;

clang/lib/Sema/SemaExpr.cpp

static QualType CheckIndirectionOperand(Sema &S, Expr *Op, ExprValueKind &VK,
   }
 
   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)
-      S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer_cpp)
+      S.Diag(OpLoc, diag::err_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)
           << OpTy << Op->getSourceRange();
   }
 
   // Dereferences are usually l-values...
   VK = VK_LValue;

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
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
 
 void f(void* p) {
-  (void)*p; // enabled-error{{ISO C++ does not allow indirection on operand of type 'void *'}}
+  (void)*p; // expected-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 *'}}
+// expected-error@-1{{static assertion failed}}
+// expected-note@-2{{because 'void *' does not satisfy 'deref'}}
+// expected-note@#FAILED_REQ{{because '*t' would be invalid: ISO C++ does not allow indirection on operand of type 'void *'}}