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

[comex]

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.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (comex)

Changes

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.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaExpr.cpp (+27-21)
• (added) clang/test/SemaCXX/builtin-overload-resolution.cpp (+8)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 237c068f59283..c00de2d4ed17b 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -6312,30 +6312,36 @@ static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext &Context,
   unsigned i = 0;
   SmallVector<QualType, 8> OverloadParams;
 
-  for (QualType ParamType : FT->param_types()) {
+  {
+    // Push an evaluation context since the lvalue conversions in this loop
+    // are only for type resolution and don't actually occur.
+    EnterExpressionEvaluationContext Unevaluated(
+        *Sema, Sema::ExpressionEvaluationContext::Unevaluated);
+    for (QualType ParamType : FT->param_types()) {
 
-    // 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;
-    }
+      // 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;
+      }
 
-    QualType PointeeType = ParamType->getPointeeType();
-    NeedsNewDecl = true;
-    LangAS AS = ArgType->getPointeeType().getAddressSpace();
+      QualType PointeeType = ParamType->getPointeeType();
+      NeedsNewDecl = true;
+      LangAS AS = ArgType->getPointeeType().getAddressSpace();
 
-    PointeeType = Context.getAddrSpaceQualType(PointeeType, AS);
-    OverloadParams.push_back(Context.getPointerType(PointeeType));
+      PointeeType = Context.getAddrSpaceQualType(PointeeType, AS);
+      OverloadParams.push_back(Context.getPointerType(PointeeType));
+    }
   }
 
   if (!NeedsNewDecl)
diff --git a/clang/test/SemaCXX/builtin-overload-resolution.cpp b/clang/test/SemaCXX/builtin-overload-resolution.cpp
new file mode 100644
index 0000000000000..81d3055a2f7b2
--- /dev/null
+++ b/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);
+}

[Sirraide]

LGTM, but please add a release note before merging.

[comex]

Updated. Aside from adding a release note, I also discovered some related issues that are fixed by adding a SFINAETrap in addition to the EnterExpressionEvaluationContext. See the added test in non-trivial-c-union.m (the __builtin_addressof call previously yielded a compile error), and the modified test in builtin-get-vtable-pointer.cpp (one of the calls previously yielded two copies of the same compile error; now it only yields one).

[cor3ntin]

LGTM, thanks!