[clang] Forbid reinterpret_cast of function pointers in constexpr.

[efriedma-quic]

This has been explicitly forbidden since C++11, but somehow the edge case of converting a function pointer to void* using a cast like (void*)f wasn't handled.

Fixes #150340 .

[llvmbot]

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

This has been explicitly forbidden since C++11, but somehow the edge case of converting a function pointer to void* using a cast like (void*)f wasn't handled.

Fixes #150340 .

---

Full diff: https://github.com/llvm/llvm-project/pull/150557.diff

3 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+13-4)
• (modified) clang/test/CXX/expr/expr.const/p2-0x.cpp (+5)
• (modified) clang/test/Sema/constexpr-void-cast.c (+4-2)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 9808298a1b1d0..993b64b2752e9 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -9741,10 +9741,19 @@ bool PointerExprEvaluator::VisitCastExpr(const CastExpr *E) {
   case CK_AddressSpaceConversion:
     if (!Visit(SubExpr))
       return false;
-    // Bitcasts to cv void* are static_casts, not reinterpret_casts, so are
-    // permitted in constant expressions in C++11. Bitcasts from cv void* are
-    // also static_casts, but we disallow them as a resolution to DR1312.
-    if (!E->getType()->isVoidPointerType()) {
+    if (E->getType()->isFunctionPointerType() ||
+        SubExpr->getType()->isFunctionPointerType()) {
+      // Casting between two function pointer types, or between a function
+      // pointer and an object pointer, is always a reinterpret_cast.
+      CCEDiag(E, diag::note_constexpr_invalid_cast)
+          << diag::ConstexprInvalidCastKind::ThisConversionOrReinterpret
+          << Info.Ctx.getLangOpts().CPlusPlus;
+      Result.Designator.setInvalid();
+    } else if (!E->getType()->isVoidPointerType()) {
+      // Bitcasts to cv void* are static_casts, not reinterpret_casts, so are
+      // permitted in constant expressions in C++11. Bitcasts from cv void* are
+      // also static_casts, but we disallow them as a resolution to DR1312.
+      //
       // In some circumstances, we permit casting from void* to cv1 T*, when the
       // actual pointee object is actually a cv2 T.
       bool HasValidResult = !Result.InvalidBase && !Result.Designator.Invalid &&
diff --git a/clang/test/CXX/expr/expr.const/p2-0x.cpp b/clang/test/CXX/expr/expr.const/p2-0x.cpp
index 910c8635f7353..8401d3033eda9 100644
--- a/clang/test/CXX/expr/expr.const/p2-0x.cpp
+++ b/clang/test/CXX/expr/expr.const/p2-0x.cpp
@@ -438,6 +438,11 @@ namespace ReinterpretCast {
   struct U {
     int m : (long)(S*)6; // expected-warning {{constant expression}} expected-note {{reinterpret_cast}}
   };
+  void f();
+  constexpr void* fp1 = (void*)f; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
+  constexpr int* fp2 = (int*)f; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
+  constexpr int (*fp3)() = (int(*)())f; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
+  constexpr int (&fp4)() = (int(&)())f; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
 }
 
 // - a pseudo-destructor call (5.2.4);
diff --git a/clang/test/Sema/constexpr-void-cast.c b/clang/test/Sema/constexpr-void-cast.c
index 2ffc59f509b4b..ffaed9a263ace 100644
--- a/clang/test/Sema/constexpr-void-cast.c
+++ b/clang/test/Sema/constexpr-void-cast.c
@@ -3,8 +3,8 @@
 // RUN: %clang_cc1 -x c -fsyntax-only %s -verify=c -std=c11 -fexperimental-new-constant-interpreter
 // RUN: %clang_cc1 -x c -fsyntax-only %s -pedantic -verify=c-pedantic -std=c11 -fexperimental-new-constant-interpreter
 //
-// RUN: %clang_cc1 -x c++ -fsyntax-only %s -verify=cxx
-// RUN: %clang_cc1 -x c++ -fsyntax-only %s -pedantic -verify=cxx-pedantic
+// RUN: %clang_cc1 -x c++ -fsyntax-only %s -verify=cxx-nointerpreter
+// RUN: %clang_cc1 -x c++ -fsyntax-only %s -pedantic -verify=cxx-pedantic,cxx-nointerpreter
 // RUN: %clang_cc1 -x c++ -fsyntax-only %s -verify=cxx -fexperimental-new-constant-interpreter
 // RUN: %clang_cc1 -x c++ -fsyntax-only %s -pedantic -verify=cxx-pedantic -fexperimental-new-constant-interpreter
 
@@ -15,4 +15,6 @@ void f(void);
 struct S {char c;} s;
 _Static_assert(&s != (void *)&f, ""); // c-pedantic-warning {{not an integer constant expression}} \
                                       // c-pedantic-note {{this conversion is not allowed in a constant expression}} \
+                                      // cxx-nointerpreter-error {{static assertion expression is not an integral constant expression}} \
+                                      // cxx-nointerpreter-note {{cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression}} \
                                       // cxx-pedantic-warning {{'_Static_assert' is a C11 extension}}

[tbaederr]

The point of adding the RUN lines for the bytecode interpreter was to encourage people to fix changes in both interpreters, not add different verify prefixes.

[efriedma-quic]

Attempted to add bytecode interpreter support. Not sure I did it right, since the code is structured very differently from what I'm used to.

[tbaederr]

Thanks!
I quickly looked into this and seems like there were some small problems in classify() and us not handling CK_LValueBitCast which made this more complex than it had to be. I came up with this patch that tries to get by without the additional opcode:

diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index e760055a8d23..b692e4cb4388 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -472,6 +472,7 @@ bool Compiler<Emitter>::VisitCastExpr(const CastExpr *CE) {
   case CK_CPointerToObjCPointerCast:
     return this->delegate(SubExpr);
 
+  case CK_LValueBitCast:
   case CK_BitCast: {
     // Reject bitcasts to atomic types.
     if (CE->getType()->isAtomicType()) {
@@ -492,13 +493,12 @@ bool Compiler<Emitter>::VisitCastExpr(const CastExpr *CE) {
     assert(isPtrType(*FromT));
     assert(isPtrType(*ToT));
     if (FromT == ToT) {
-      if (CE->getType()->isVoidPointerType())
+      if (CE->getType()->isVoidPointerType() &&
+          !SubExpr->getType()->isFunctionPointerType())
         return this->delegate(SubExpr);
 
       if (!this->visit(SubExpr))
         return false;
-      if (CE->getType()->isFunctionPointerType())
-        return true;
       if (FromT == PT_Ptr)
         return this->emitPtrPtrCast(SubExprTy->isVoidPointerType(), CE);
       return true;
diff --git a/clang/lib/AST/ByteCode/Context.cpp b/clang/lib/AST/ByteCode/Context.cpp
index aaeb52e0fa44..63baa7038ade 100644
--- a/clang/lib/AST/ByteCode/Context.cpp
+++ b/clang/lib/AST/ByteCode/Context.cpp
@@ -351,7 +351,7 @@ OptPrimType Context::classify(QualType T) const {
       return PT_Float;
   }
 
-  if (T->isPointerOrReferenceType())
+  if (T->isPointerOrReferenceType() || T->isFunctionProtoType())
     return PT_Ptr;
 
   if (T->isMemberPointerType())
@@ -376,6 +376,9 @@ OptPrimType Context::classify(QualType T) const {
   if (const auto *DT = dyn_cast<DecltypeType>(T))
     return classify(DT->getUnderlyingType());
 
+  if (const auto *PT = dyn_cast<ParenType>(T))
+    return classify(PT->getInnerType());
+
   if (T->isObjCObjectPointerType() || T->isBlockPointerType())
     return PT_Ptr;

this seems to have the same effect as your changes (including the additional diagnostic in functions.cpp unfortunately). I don't have time to run the entire test suite against this patch today. If it breaks something for you, feel free to use your changes instead.

[efriedma-quic]

Your suggestion is subtly different from my patch in one way; consider:

#define fold(x) (__builtin_constant_p(x) ? (x) : (x))
constexpr double f() { return 1; }
typedef double (*DoubleFn)();
constexpr DoubleFn x = (DoubleFn)fold((void*)f);

if you write something like constexpr fnptr x = (fnptr)fold(void*)f));, my patch gives a warning, and yours doesn't. I should probably add a testcase for this. Granted, I'm not sure this matters a lot, since there's no way to get a function pointer into a void* without resorting to extensions.

---

The additional diagnostic in functions.cpp is because traditional consteval produces a diagnostic note_invalid_subexpr_in_const_expr if a pointer is called where the type of the function doesn't match the function pointer type. CallPtr doesn't produce a diagnostic if it finds a mismatch, so we return a "stale" diagnostic.

It also has a much looser interpretation of what counts as "matching". For example, it's happy if you call a function that returns "long" with a function pointer that returns "long long".