[clang][Interp] Do r-to-l conversion immediately when returning

[tbaederr]

First, we need to register local constant variables in C, so we get the same diagnostic behavior as the current interpeter.

Second, when returning an LValue (as a Pointer), which we eventually convert to an RValue, we need to do the conversion immediately when saving the Pointer in the EvaluationResult. Otherwise, we will possibly deallocate the data before doing the conversion (which will look at the Block*).

[tbaederr]

Asking for review here since I'm not sure if the reasoning makes sense for other people.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

First, we need to register local constant variables in C, so we get the same diagnostic behavior as the current interpeter.

Second, when returning an LValue (as a Pointer), which we eventually convert to an RValue, we need to do the conversion immediately when saving the Pointer in the EvaluationResult. Otherwise, we will possibly deallocate the data before doing the conversion (which will look at the Block*).

---

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

7 Files Affected:

• (modified) clang/lib/AST/Interp/ByteCodeExprGen.cpp (+1-2)
• (modified) clang/lib/AST/Interp/Context.cpp (+4-12)
• (modified) clang/lib/AST/Interp/EvalEmitter.cpp (+18-2)
• (modified) clang/lib/AST/Interp/EvalEmitter.h (+4-1)
• (modified) clang/lib/AST/Interp/EvaluationResult.h (+1-1)
• (modified) clang/test/AST/Interp/c.c (+13)
• (modified) clang/test/Sema/warn-char-subscripts.c (+1)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 3ca4f56903fda..3248a3b9471a6 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -3002,8 +3002,7 @@ bool ByteCodeExprGen<Emitter>::VisitDeclRefExpr(const DeclRefExpr *E) {
   // This happens in C.
   if (!Ctx.getLangOpts().CPlusPlus) {
     if (const auto *VD = dyn_cast<VarDecl>(D);
-        VD && VD->hasGlobalStorage() && VD->getAnyInitializer() &&
-        VD->getType().isConstQualified()) {
+        VD && VD->getAnyInitializer() && VD->getType().isConstQualified()) {
       if (!this->visitVarDecl(VD))
         return false;
       // Retry.
diff --git a/clang/lib/AST/Interp/Context.cpp b/clang/lib/AST/Interp/Context.cpp
index 5f5a6622f10f3..061f4e1f35779 100644
--- a/clang/lib/AST/Interp/Context.cpp
+++ b/clang/lib/AST/Interp/Context.cpp
@@ -44,7 +44,7 @@ bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {
   assert(Stk.empty());
   ByteCodeExprGen<EvalEmitter> C(*this, *P, Parent, Stk, Result);
 
-  auto Res = C.interpretExpr(E);
+  auto Res = C.interpretExpr(E, /*ConvertResultToRValue=*/E->isGLValue());
 
   if (Res.isInvalid()) {
     Stk.clear();
@@ -58,16 +58,7 @@ bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {
   Stk.clear();
 #endif
 
-  // Implicit lvalue-to-rvalue conversion.
-  if (E->isGLValue()) {
-    std::optional<APValue> RValueResult = Res.toRValue();
-    if (!RValueResult) {
-      return false;
-    }
-    Result = *RValueResult;
-  } else {
-    Result = Res.toAPValue();
-  }
+  Result = Res.toAPValue();
 
   return true;
 }
@@ -120,7 +111,8 @@ bool Context::evaluateAsInitializer(State &Parent, const VarDecl *VD,
         !Res.checkFullyInitialized(C.getState()))
       return false;
 
-    // lvalue-to-rvalue conversion.
+    // lvalue-to-rvalue conversion. We do this manually here so we can
+    // examine the result above before converting and returning it.
     std::optional<APValue> RValueResult = Res.toRValue();
     if (!RValueResult)
       return false;
diff --git a/clang/lib/AST/Interp/EvalEmitter.cpp b/clang/lib/AST/Interp/EvalEmitter.cpp
index a60f893de8bda..5a36b46b74543 100644
--- a/clang/lib/AST/Interp/EvalEmitter.cpp
+++ b/clang/lib/AST/Interp/EvalEmitter.cpp
@@ -33,7 +33,9 @@ EvalEmitter::~EvalEmitter() {
   }
 }
 
-EvaluationResult EvalEmitter::interpretExpr(const Expr *E) {
+EvaluationResult EvalEmitter::interpretExpr(const Expr *E,
+                                            bool ConvertResultToRValue) {
+  this->ConvertResultToRValue = ConvertResultToRValue;
   EvalResult.setSource(E);
 
   if (!this->visitExpr(E))
@@ -119,12 +121,26 @@ template <PrimType OpType> bool EvalEmitter::emitRet(const SourceInfo &Info) {
 template <> bool EvalEmitter::emitRet<PT_Ptr>(const SourceInfo &Info) {
   if (!isActive())
     return true;
-  EvalResult.setPointer(S.Stk.pop<Pointer>());
+
+  const Pointer &Ptr = S.Stk.pop<Pointer>();
+  // Implicitly convert lvalue to rvalue, if requested.
+  if (ConvertResultToRValue) {
+    if (std::optional<APValue> V = Ptr.toRValue(Ctx)) {
+      EvalResult.setValue(*V);
+    } else {
+      return false;
+    }
+  } else {
+    EvalResult.setPointer(Ptr);
+  }
+
   return true;
 }
 template <> bool EvalEmitter::emitRet<PT_FnPtr>(const SourceInfo &Info) {
   if (!isActive())
     return true;
+  // Function pointers are always lvalues to us and cannot be converted
+  // to rvalues, so don't do any conversion here.
   EvalResult.setFunctionPointer(S.Stk.pop<FunctionPointer>());
   return true;
 }
diff --git a/clang/lib/AST/Interp/EvalEmitter.h b/clang/lib/AST/Interp/EvalEmitter.h
index deb2ebc4e61fa..8159e489f168e 100644
--- a/clang/lib/AST/Interp/EvalEmitter.h
+++ b/clang/lib/AST/Interp/EvalEmitter.h
@@ -34,7 +34,8 @@ class EvalEmitter : public SourceMapper {
   using AddrTy = uintptr_t;
   using Local = Scope::Local;
 
-  EvaluationResult interpretExpr(const Expr *E);
+  EvaluationResult interpretExpr(const Expr *E,
+                                 bool ConvertResultToRValue = false);
   EvaluationResult interpretDecl(const VarDecl *VD);
 
   InterpState &getState() { return S; }
@@ -86,6 +87,8 @@ class EvalEmitter : public SourceMapper {
   InterpState S;
   /// Location to write the result to.
   EvaluationResult EvalResult;
+  /// Whether the result should be converted to an RValue.
+  bool ConvertResultToRValue = false;
 
   /// Temporaries which require storage.
   llvm::DenseMap<unsigned, std::unique_ptr<char[]>> Locals;
diff --git a/clang/lib/AST/Interp/EvaluationResult.h b/clang/lib/AST/Interp/EvaluationResult.h
index 2b9fc16f1a0ab..52a6c011e39e1 100644
--- a/clang/lib/AST/Interp/EvaluationResult.h
+++ b/clang/lib/AST/Interp/EvaluationResult.h
@@ -56,8 +56,8 @@ class EvaluationResult final {
   void setSource(DeclTy D) { Source = D; }
 
   void setValue(const APValue &V) {
+    // V could still be an LValue.
     assert(empty());
-    assert(!V.isLValue());
     Value = std::move(V);
     Kind = RValue;
   }
diff --git a/clang/test/AST/Interp/c.c b/clang/test/AST/Interp/c.c
index 53ce62fa1452e..0ecd4eec8390c 100644
--- a/clang/test/AST/Interp/c.c
+++ b/clang/test/AST/Interp/c.c
@@ -125,3 +125,16 @@ _Static_assert(sizeof(name2) == 0, ""); // expected-error {{failed}} \
                                         // expected-note {{evaluates to}} \
                                         // pedantic-expected-error {{failed}} \
                                         // pedantic-expected-note {{evaluates to}}
+
+void t14(void) {
+  int array[256] = { 0 }; // expected-note {{array 'array' declared here}} \
+                          // pedantic-expected-note {{array 'array' declared here}} \
+                          // ref-note {{array 'array' declared here}} \
+                          // pedantic-ref-note {{array 'array' declared here}}
+  const char b = -1;
+  int val = array[b]; // expected-warning {{array index -1 is before the beginning of the array}} \
+                      // pedantic-expected-warning {{array index -1 is before the beginning of the array}} \
+                      // ref-warning {{array index -1 is before the beginning of the array}} \
+                      // pedantic-ref-warning {{array index -1 is before the beginning of the array}}
+
+}
diff --git a/clang/test/Sema/warn-char-subscripts.c b/clang/test/Sema/warn-char-subscripts.c
index 0a012f68feae0..c2f7a3731d72c 100644
--- a/clang/test/Sema/warn-char-subscripts.c
+++ b/clang/test/Sema/warn-char-subscripts.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -Wchar-subscripts -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wchar-subscripts -fsyntax-only -verify %s -fexperimental-new-constant-interpreter
 
 void t1(void) {
   int array[1] = { 0 };

[AaronBallman]

LGTM!