[clang] Fix side effects resolving overloads of builtin functions (#138651) (#154034)

When parsing `__builtin_addressof(Value)`, where `Value` is a constexpr
variable of primitive type, we will run through
`rewriteBuiltinFunctionDecl`.

`rewriteBuiltinFunctionDecl` is meant to handle a special case which is
not applicable here. (It only applies when a builtin function's type has
a parameter with pointer type, which is not true for
`__builtin_addressof`, not even if the actual argument is a pointer.)
Therefore, `rewriteBuiltinFunctionDecl` returns `nullptr` and things go
on as usual.

But `rewriteBuiltinFunctionDecl` accidentally has a side effect. It
calls `DefaultFunctionArrayLvalueConversion` ->
`DefaultLvalueConversion` -> `CheckLValueToRValueConversionOperand` ->
`rebuildPotentialResultsAsNonOdrUsed` -> `MarkNotOdrUsed`, which removes
the expression from `S.MaybeODRUseExprs`.

This would be correct if `Value` were actually going through an
lvalue-to-rvalue conversion, because it's a constant expression:

https://eel.is/c++draft/basic.def.odr#5.2.2.2

But in this case the conversion is only hypothetical, as part of
`rewriteBuiltinFunctionDecl`'s pseudo-overload-resolution logic.

Fix the side effect by pushing an `ExpressionEvaluationContext`, like we
do for real overload resolution.

Similarly, push a `SFINAETrap` to suppress diagnostics emitted during
`DefaultFunctionArrayLvalueConversion`. This fixes a false-positive
compile error when applying `__builtin_addressof` to certain volatile
union variables, and fixes a duplicated compile error when applying
`__builtin_get_vtable_pointer` to an overloaded function name.

---------

Co-authored-by: Corentin Jabot <[email protected]>

Author: comex

clang/docs/ReleaseNotes.rst

@@ -350,6 +350,8 @@ Bug Fixes to C++ Support
   authentication enabled. (#GH152601)
 - Fix the check for narrowing int-to-float conversions, so that they are detected in
   cases where converting the float back to an integer is undefined behaviour (#GH157067).
+- Fix an assertion failure when a ``constexpr`` variable is only referenced through
+  ``__builtin_addressof``, and related issues with builtin arguments. (#GH154034)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^

clang/lib/Sema/SemaExpr.cpp

@@ -6313,30 +6313,38 @@ static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext &Context,
   unsigned i = 0;
   SmallVector<QualType, 8> OverloadParams;
 
-  for (QualType ParamType : FT->param_types()) {
+  {
+    // The lvalue conversions in this loop are only for type resolution and
+    // don't actually occur.
+    EnterExpressionEvaluationContext Unevaluated(
+        *Sema, Sema::ExpressionEvaluationContext::Unevaluated);
+    Sema::SFINAETrap Trap(*Sema, /*ForValidityCheck=*/true);
 
-    // Convert array arguments to pointer to simplify type lookup.
-    ExprResult ArgRes =
-        Sema->DefaultFunctionArrayLvalueConversion(ArgExprs[i++]);
-    if (ArgRes.isInvalid())
-      return nullptr;
-    Expr *Arg = ArgRes.get();
-    QualType ArgType = Arg->getType();
-    if (!ParamType->isPointerType() ||
-        ParamType->getPointeeType().hasAddressSpace() ||
-        !ArgType->isPointerType() ||
-        !ArgType->getPointeeType().hasAddressSpace() ||
-        isPtrSizeAddressSpace(ArgType->getPointeeType().getAddressSpace())) {
-      OverloadParams.push_back(ParamType);
-      continue;
-    }
+    for (QualType ParamType : FT->param_types()) {
 
-    QualType PointeeType = ParamType->getPointeeType();
-    NeedsNewDecl = true;
-    LangAS AS = ArgType->getPointeeType().getAddressSpace();
+      // Convert array arguments to pointer to simplify type lookup.
+      ExprResult ArgRes =
+          Sema->DefaultFunctionArrayLvalueConversion(ArgExprs[i++]);
+      if (ArgRes.isInvalid())
+        return nullptr;
+      Expr *Arg = ArgRes.get();
+      QualType ArgType = Arg->getType();
+      if (!ParamType->isPointerType() ||
+          ParamType->getPointeeType().hasAddressSpace() ||
+          !ArgType->isPointerType() ||
+          !ArgType->getPointeeType().hasAddressSpace() ||
+          isPtrSizeAddressSpace(ArgType->getPointeeType().getAddressSpace())) {
+        OverloadParams.push_back(ParamType);
+        continue;
+      }
 
-    PointeeType = Context.getAddrSpaceQualType(PointeeType, AS);
-    OverloadParams.push_back(Context.getPointerType(PointeeType));
+      QualType PointeeType = ParamType->getPointeeType();
+      NeedsNewDecl = true;
+      LangAS AS = ArgType->getPointeeType().getAddressSpace();
+
+      PointeeType = Context.getAddrSpaceQualType(PointeeType, AS);
+      OverloadParams.push_back(Context.getPointerType(PointeeType));
+    }
   }
 
   if (!NeedsNewDecl)

clang/test/SemaCXX/builtin-get-vtable-pointer.cpp

@@ -66,9 +66,7 @@ struct PolymorphicTemplate {
 };
 
 void test_function(int);    // expected-note{{possible target for call}}
-                            // expected-note@-1{{possible target for call}}
 void test_function(double); // expected-note{{possible target for call}}
-                            // expected-note@-1{{possible target for call}}
 
 void getVTablePointer() {
   ForwardDeclaration *fd = nullptr;
@@ -89,7 +87,6 @@ void getVTablePointer() {
   __builtin_get_vtable_pointer(np_array);      // expected-error{{__builtin_get_vtable_pointer requires an argument of polymorphic class pointer type, but 'NonPolymorphic' has no virtual methods}}
   __builtin_get_vtable_pointer(&np_array);     // expected-error{{__builtin_get_vtable_pointer requires an argument of class pointer type, but 'NonPolymorphic (*)[1]' was provided}}
   __builtin_get_vtable_pointer(test_function); // expected-error{{reference to overloaded function could not be resolved; did you mean to call it?}}
-                                               // expected-error@-1{{reference to overloaded function could not be resolved; did you mean to call it?}}
   Foo<double> Food;
   Foo<int> Fooi;
   __builtin_get_vtable_pointer(Food); // expected-error{{__builtin_get_vtable_pointer requires an argument of class pointer type, but 'Foo<double>' was provided}}

clang/test/SemaCXX/builtin-overload-resolution.cpp

@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -std=c++20 %s -emit-obj -o /dev/null
+
+const int* test_odr_used() {
+  // This previously crashed due to Value improperly being removed from
+  // MaybeODRUseExprs.
+  static constexpr int Value = 0;
+  return __builtin_addressof(Value);
+}

clang/test/SemaObjC/non-trivial-c-union.m

@@ -87,3 +87,10 @@ void testVolatileLValueToRValue(volatile U0 *a) {
 void unionInSystemHeader0(U0_SystemHeader);
 
 void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
+
+void testAddressof(void) {
+  extern volatile U0 t0;
+  // These don't dereference so they shouldn't cause an error.
+  (void)&t0;
+  (void)__builtin_addressof(t0);
+}