Skip to content

[clang][bytecode] Fix activating nested unions #147338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

tbaederr
Copy link
Contributor

@tbaederr tbaederr commented Jul 7, 2025

When activating the new pointer, we need to de-activate pointers of all parent unions, not just the topmost one.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:bytecode Issues for the clang bytecode constexpr interpreter labels Jul 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 7, 2025

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

When activating the new pointer, we need to de-activate pointers of all parent unions, not just the topmost one.


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

2 Files Affected:

  • (modified) clang/lib/AST/ByteCode/Pointer.cpp (+14-22)
  • (modified) clang/test/AST/ByteCode/cxx23.cpp (+45)
diff --git a/clang/lib/AST/ByteCode/Pointer.cpp b/clang/lib/AST/ByteCode/Pointer.cpp
index e915316272178..3ddae950926a0 100644
--- a/clang/lib/AST/ByteCode/Pointer.cpp
+++ b/clang/lib/AST/ByteCode/Pointer.cpp
@@ -525,33 +525,25 @@ void Pointer::activate() const {
   Pointer UnionPtr = getBase();
   while (!UnionPtr.isRoot() && UnionPtr.inUnion())
     UnionPtr = UnionPtr.getBase();
-
   assert(UnionPtr.getFieldDesc()->isUnion());
-  const Record *UnionRecord = UnionPtr.getRecord();
-
-  // The direct child pointer of the union that's on the path from
-  // this pointer to the union.
-  Pointer ChildPtr = *this;
-  assert(ChildPtr != UnionPtr);
-  while (true) {
-    if (ChildPtr.getBase() == UnionPtr)
-      break;
-    ChildPtr = ChildPtr.getBase();
-  }
-  assert(ChildPtr.getBase() == UnionPtr);
-
-  for (const Record::Field &F : UnionRecord->fields()) {
-    Pointer FieldPtr = UnionPtr.atField(F.Offset);
-    if (FieldPtr == ChildPtr) {
-      // No need to deactivate, will be activated in the next loop.
-    } else {
-      deactivate(FieldPtr);
-    }
-  }
 
   Pointer B = *this;
   while (B != UnionPtr) {
     activate(B);
+
+    // When walking up the pointer chain, deactivate
+    // all union child pointers that aren't on our path.
+    if (!B.isRoot()) {
+      Pointer BasePtr = B.getBase();
+      if (const Record *BR = BasePtr.getRecord(); BR && BR->isUnion()) {
+        for (const Record::Field &F : BR->fields()) {
+          Pointer FieldPtr = BasePtr.atField(F.Offset);
+          if (FieldPtr != B)
+            deactivate(FieldPtr);
+        }
+      }
+    }
+
     B = B.getBase();
   }
 }
diff --git a/clang/test/AST/ByteCode/cxx23.cpp b/clang/test/AST/ByteCode/cxx23.cpp
index 417d35dbca946..2856b872d44ab 100644
--- a/clang/test/AST/ByteCode/cxx23.cpp
+++ b/clang/test/AST/ByteCode/cxx23.cpp
@@ -4,6 +4,16 @@
 // RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected20,all,all20 %s -fexperimental-new-constant-interpreter
 // RUN: %clang_cc1 -std=c++23 -fsyntax-only -fcxx-exceptions -verify=expected23,all,all23 %s -fexperimental-new-constant-interpreter
 
+
+#define assert_active(F)   if (!__builtin_is_within_lifetime(&F)) (1/0);
+#define assert_inactive(F) if ( __builtin_is_within_lifetime(&F)) (1/0);
+
+inline constexpr void* operator new(__SIZE_TYPE__, void* p) noexcept { return p; }
+namespace std {
+template<typename T, typename... Args>
+constexpr T* construct_at(T* p, Args&&... args) { return ::new((void*)p) T(static_cast<Args&&>(args)...); }
+}
+
 constexpr int f(int n) {  // all20-error {{constexpr function never produces a constant expression}}
   static const int m = n; // all-note {{control flows through the definition of a static variable}} \
                           // all20-note {{control flows through the definition of a static variable}} \
@@ -328,3 +338,38 @@ namespace VoidCast {
   constexpr int *f = (int*)(void*)b; // all-error {{must be initialized by a constant expression}} \
                                      // all-note {{cast from 'void *' is not allowed in a constant expression}}
 }
+
+#if __cplusplus >= 202302L
+namespace NestedUnions {
+  consteval bool test_nested() {
+    union {
+      union { int i; char c; } u;
+      long l;
+    };
+    std::construct_at(&l);
+    assert_active(l);
+    assert_inactive(u);
+
+    std::construct_at(&u);
+    assert_active(u);
+    assert_inactive(l);
+    assert_active(u.i);
+    assert_inactive(u.c);
+
+    std::construct_at(&u.i);
+    assert_active(u);
+    assert_inactive(u.c);
+
+
+    std::construct_at(&u.c);
+    assert_active(u);
+    assert_inactive(u.i);
+    assert_active(u.c);
+    assert_inactive(l);
+    return true;
+  }
+  static_assert(test_nested());
+
+
+}
+#endif

When activating the new pointer, we need to de-activate pointers of
all parent unions, not just the topmost one.
@tbaederr tbaederr merged commit 36dd61f into llvm:main Jul 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:bytecode Issues for the clang bytecode constexpr interpreter clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants