[clang] [clang][bytecode] Reject constexpr-unknown values from comparisons (PR #133701)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

<details>
<summary>Changes</summary>



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


4 Files Affected:

- (modified) clang/lib/AST/ByteCode/Interp.cpp (+13-5) 
- (modified) clang/lib/AST/ByteCode/Interp.h (+7) 
- (added) clang/test/AST/ByteCode/codegen-constexpr-unknown.cpp (+72) 
- (removed) clang/test/AST/ByteCode/codegen-mutable-read.cpp (-36) 


``````````diff
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index 187477713bef8..0acfe01a42410 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -302,6 +302,17 @@ void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC,
     TYPE_SWITCH(Ty, S.Stk.discard<T>());
 }
 
+// FIXME: Instead of using this fairly expensive test, we should
+// just mark constexpr-unknown values when creating them.
+bool isConstexprUnknown(const Pointer &P) {
+  if (!P.isBlockPointer())
+    return false;
+  if (P.isDummy())
+    return false;
+  const VarDecl *VD = P.block()->getDescriptor()->asVarDecl();
+  return VD && VD->hasLocalStorage();
+}
+
 bool CheckBCPResult(InterpState &S, const Pointer &Ptr) {
   if (Ptr.isDummy())
     return false;
@@ -607,11 +618,8 @@ bool CheckMutable(InterpState &S, CodePtr OpPC, const Pointer &Ptr) {
     // variables in Compiler.cpp:visitDeclRef. Revisiting a so far
     // unknown variable will get the same EvalID and we end up allowing
     // reads from mutable members of it.
-    if (!S.inConstantContext()) {
-      if (const VarDecl *VD = Ptr.block()->getDescriptor()->asVarDecl();
-          VD && VD->hasLocalStorage())
-        return false;
-    }
+    if (!S.inConstantContext() && isConstexprUnknown(Ptr))
+      return false;
     return true;
   }
 
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index ee4139fbc9530..938077a9f10ae 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -173,6 +173,8 @@ static bool handleOverflow(InterpState &S, CodePtr OpPC, const T &SrcValue) {
 bool handleFixedPointOverflow(InterpState &S, CodePtr OpPC,
                               const FixedPoint &FP);
 
+bool isConstexprUnknown(const Pointer &P);
+
 enum class ShiftDir { Left, Right };
 
 /// Checks if the shift operation is legal.
@@ -1062,6 +1064,11 @@ inline bool CmpHelperEQ<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
     }
   }
 
+  if (!S.inConstantContext()) {
+    if (isConstexprUnknown(LHS) || isConstexprUnknown(RHS))
+      return false;
+  }
+
   if (Pointer::hasSameBase(LHS, RHS)) {
     unsigned VL = LHS.getByteOffset();
     unsigned VR = RHS.getByteOffset();
diff --git a/clang/test/AST/ByteCode/codegen-constexpr-unknown.cpp b/clang/test/AST/ByteCode/codegen-constexpr-unknown.cpp
new file mode 100644
index 0000000000000..f62117d5f7bec
--- /dev/null
+++ b/clang/test/AST/ByteCode/codegen-constexpr-unknown.cpp
@@ -0,0 +1,72 @@
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -fcxx-exceptions -o - %s                                         | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -fcxx-exceptions -o - %s -fexperimental-new-constant-interpreter | FileCheck %s
+
+
+/// In the if expression below, the read from s.i should fail.
+/// If it doesn't, and we actually read the value 0, the call to
+/// func() will always occur, resuliting in a runtime failure.
+
+struct S {
+  mutable int i = 0;
+};
+
+void func() {
+  __builtin_abort();
+};
+
+void setI(const S &s) {
+  s.i = 12;
+}
+
+int main() {
+  const S s;
+
+  setI(s);
+
+  if (s.i == 0)
+    func();
+
+  return 0;
+}
+
+// CHECK: define dso_local noundef i32 @main()
+// CHECK: br
+// CHECK: if.then
+// CHECK: if.end
+// CHECK: ret i32 0
+
+
+/// Similarly, here we revisit the BindingDecl.
+struct F { int x; };
+int main2() {
+  const F const s{99};
+  const auto& [r1] = s;
+  if (&r1 != &s.x)
+    __builtin_abort();
+  return 0;
+}
+// CHECK: define dso_local noundef i32 @_Z5main2v()
+// CHECK: br
+// CHECK: if.then
+// CHECK: if.end
+// CHECK: ret i32 0
+
+/// The comparison here should work and return 0.
+class X {
+public:
+  X();
+  X(const X&);
+  X(const volatile X &);
+  ~X();
+};
+extern X OuterX;
+X test24() {
+  X x;
+  if (&x == &OuterX)
+    throw 0;
+  return x;
+}
+
+// CHECK: define dso_local void @_Z6test24v
+// CHECK-NOT: lpad
+// CHECK-NOT: eh.resume
diff --git a/clang/test/AST/ByteCode/codegen-mutable-read.cpp b/clang/test/AST/ByteCode/codegen-mutable-read.cpp
deleted file mode 100644
index afa46d34b0673..0000000000000
--- a/clang/test/AST/ByteCode/codegen-mutable-read.cpp
+++ /dev/null
@@ -1,36 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s                                         | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s -fexperimental-new-constant-interpreter | FileCheck %s
-
-
-/// In the if expression below, the read from s.i should fail.
-/// If it doesn't, and we actually read the value 0, the call to
-/// func() will always occur, resuliting in a runtime failure.
-
-struct S {
-  mutable int i = 0;
-};
-
-void func() {
-  __builtin_abort();
-};
-
-void setI(const S &s) {
-  s.i = 12;
-}
-
-int main() {
-  const S s;
-
-  setI(s);
-
-  if (s.i == 0)
-    func();
-
-  return 0;
-}
-
-// CHECK: define dso_local noundef i32 @main()
-// CHECK: br
-// CHECK: if.then
-// CHECK: if.end
-// CHECK: ret i32 0

``````````

</details>


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