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

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: None (comex)

<details>
<summary>Changes</summary>

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
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);
+}

``````````

</details>


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