[Clang][ExprConstant] fix constant expression did not evaluate to integer

[Backl1ght]

fixes #96670

The cause is that we might return a lvalue here at

llvm-project/clang/lib/AST/ExprConstant.cpp

Lines 15861 to 15865 in 3e53c97

	if (CE->hasAPValueResult()) {
	Result.Val = CE->getAPValueResult();
	IsConst = true;
	return true;
	}

This PR will make sure we return a rvalue in FastEvaluateAsRValue.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Zhikai Zeng (Backl1ght)

Changes

fixes #96670

The cause is that we might return a lvalue here at

llvm-project/clang/lib/AST/ExprConstant.cpp

Lines 15861 to 15865 in 3e53c97

	if (CE->hasAPValueResult()) {
	Result.Val = CE->getAPValueResult();
	IsConst = true;
	return true;
	}

This PR will make sure we return a rvalue in FastEvaluateAsRValue.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/AST/ExprConstant.cpp (+1-1)
• (modified) clang/test/SemaCXX/eval-crashes.cpp (+10)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index feba3c7ba8d77..8ec1c105cef9f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -936,6 +936,7 @@ Bug Fixes to C++ Support
   forward-declared class. (#GH93512).
 - Fixed a bug in access checking inside return-type-requirement of compound requirements. (#GH93788).
 - Fixed an assertion failure about invalid conversion when calling lambda. (#GH96205).
+- Fixed an assertion failure about constant expression did not evaluate to integer. (#GH96670).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 374a3acf7aa26..0ccdc5eaabeef 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15858,7 +15858,7 @@ static bool FastEvaluateAsRValue(const Expr *Exp, Expr::EvalResult &Result,
   }
 
   if (const auto *CE = dyn_cast<ConstantExpr>(Exp)) {
-    if (CE->hasAPValueResult()) {
+    if (CE->hasAPValueResult() && !CE->getAPValueResult().isLValue()) {
       Result.Val = CE->getAPValueResult();
       IsConst = true;
       return true;
diff --git a/clang/test/SemaCXX/eval-crashes.cpp b/clang/test/SemaCXX/eval-crashes.cpp
index 017df977b26b7..0865dafe4bf92 100644
--- a/clang/test/SemaCXX/eval-crashes.cpp
+++ b/clang/test/SemaCXX/eval-crashes.cpp
@@ -61,3 +61,13 @@ struct array {
   array() : data(*new int[1][2]) {}
 };
 }
+
+namespace GH96670 {
+inline constexpr long ullNil = -1;
+
+template<typename T = long, const T &Nil = ullNil>
+struct Test {};
+
+inline constexpr long lNil = -1;
+Test<long, lNil> c;
+}

[Sirraide]

LGTM after rewording the release note a bit and addressing @tbaederr’s comment.

[efriedma-quic]

I'm concerned that we're calling EvaluateKnownConstInt on an lvalue; that might indicate there's something wrong with the AST. Usually there should be an lvalue-to-rvalue cast somewhere.

[Backl1ght]

I'm concerned that we're calling EvaluateKnownConstInt on an lvalue; that might indicate there's something wrong with the AST. Usually there should be an lvalue-to-rvalue cast somewhere.

@efriedma-quic lvalue-to-rvalue cast is done here at

llvm-project/clang/lib/AST/ExprConstant.cpp

Lines 15829 to 15835 in aec7670

	// Implicit lvalue-to-rvalue cast.
	if (E->isGLValue()) {
	LValue LV;
	LV.setFrom(Info.Ctx, Result);
	if (!handleLValueToRValueConversion(Info, E, E->getType(), LV, Result))
	return false;
	}

[Sirraide]

I'm concerned that we're calling EvaluateKnownConstInt on an lvalue; that might indicate there's something wrong with the AST. Usually there should be an lvalue-to-rvalue cast somewhere.

Yeah, I also remember finding that strange, but after looking into it I figured it made sense, but I don’t remmeber why exactly...

[Sirraide]

Actually, where are we calling EvaluateKnownConstInt here; am I missing something obvious?

[Sirraide]

Ah, that was in the original stack trace, I see. The fix here makes sense to me because it’s in FastEvaluateAsRValue, so it makes sense that that can fail since it’s just supposed to be a fast path.

[efriedma-quic]

To the extent that EvaluateAsRValue handles LValues, FastEvaluateAsRValue should also handle them, sure. I'm suspicious of the fact that EvaluateAsRValue is doing this conversion in the first place, but I guess this pull request isn't the place to address that.

Given that, this patch LGTM