[clang][Interp] IndirectMember initializers #69900

tbaederr · 2023-10-23T07:30:28Z

We need to look at the chain of declarations to initialize the right field.

llvmbot · 2023-10-23T07:32:14Z

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

We need to look at the chain of declarations to initialize the right field.

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

4 Files Affected:

(modified) clang/lib/AST/Interp/ByteCodeStmtGen.cpp (+44-23)
(modified) clang/lib/AST/Interp/Interp.h (+5-2)
(modified) clang/lib/AST/Interp/Opcodes.td (+5-1)
(modified) clang/test/AST/Interp/records.cpp (+60)

diff --git a/clang/lib/AST/Interp/ByteCodeStmtGen.cpp b/clang/lib/AST/Interp/ByteCodeStmtGen.cpp
index 509abe3ae867f93..c22bbd8623e5326 100644
--- a/clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -142,6 +142,27 @@ bool ByteCodeStmtGen<Emitter>::visitFunc(const FunctionDecl *F) {
   // Classify the return type.
   ReturnType = this->classify(F->getReturnType());
 
+  auto emitFieldInitializer = [&](const Record::Field *F, unsigned FieldOffset,
+                                  const Expr *InitExpr) -> bool {
+    if (std::optional<PrimType> T = this->classify(InitExpr)) {
+      if (!this->visit(InitExpr))
+        return false;
+
+      if (F->isBitField())
+        return this->emitInitThisBitField(*T, F, FieldOffset, InitExpr);
+      return this->emitInitThisField(*T, FieldOffset, InitExpr);
+    }
+    // Non-primitive case. Get a pointer to the field-to-initialize
+    // on the stack and call visitInitialzer() for it.
+    if (!this->emitGetPtrThisField(FieldOffset, InitExpr))
+      return false;
+
+    if (!this->visitInitializer(InitExpr))
+      return false;
+
+    return this->emitPopPtr(InitExpr);
+  };
+
   // Emit custom code if this is a lambda static invoker.
   if (const auto *MD = dyn_cast<CXXMethodDecl>(F);
       MD && MD->isLambdaStaticInvoker())
@@ -162,29 +183,8 @@ bool ByteCodeStmtGen<Emitter>::visitFunc(const FunctionDecl *F) {
       if (const FieldDecl *Member = Init->getMember()) {
         const Record::Field *F = R->getField(Member);
 
-        if (std::optional<PrimType> T = this->classify(InitExpr)) {
-          if (!this->visit(InitExpr))
-            return false;
-
-          if (F->isBitField()) {
-            if (!this->emitInitThisBitField(*T, F, InitExpr))
-              return false;
-          } else {
-            if (!this->emitInitThisField(*T, F->Offset, InitExpr))
-              return false;
-          }
-        } else {
-          // Non-primitive case. Get a pointer to the field-to-initialize
-          // on the stack and call visitInitialzer() for it.
-          if (!this->emitGetPtrThisField(F->Offset, InitExpr))
-            return false;
-
-          if (!this->visitInitializer(InitExpr))
-            return false;
-
-          if (!this->emitPopPtr(InitExpr))
-            return false;
-        }
+        if (!emitFieldInitializer(F, F->Offset, InitExpr))
+          return false;
       } else if (const Type *Base = Init->getBaseClass()) {
         // Base class initializer.
         // Get This Base and call initializer on it.
@@ -198,6 +198,27 @@ bool ByteCodeStmtGen<Emitter>::visitFunc(const FunctionDecl *F) {
           return false;
         if (!this->emitInitPtrPop(InitExpr))
           return false;
+      } else if (const IndirectFieldDecl *IFD = Init->getIndirectMember()) {
+        assert(IFD->getChainingSize() >= 2);
+
+        unsigned NestedFieldOffset = 0;
+        const Record::Field *NestedField = nullptr;
+        for (const NamedDecl *ND : IFD->chain()) {
+          // FIXME: Can this *not* be a FieldDecl?
+          const FieldDecl *FD = cast<FieldDecl>(ND);
+          const Record *FieldRecord =
+              this->P.getOrCreateRecord(FD->getParent());
+          assert(FieldRecord);
+
+          NestedField = FieldRecord->getField(FD);
+          assert(NestedField);
+
+          NestedFieldOffset += NestedField->Offset;
+        }
+        assert(NestedField);
+
+        if (!emitFieldInitializer(NestedField, NestedFieldOffset, InitExpr))
+          return false;
       } else {
         assert(Init->isDelegatingInitializer());
         if (!this->emitThis(InitExpr))
diff --git a/clang/lib/AST/Interp/Interp.h b/clang/lib/AST/Interp/Interp.h
index 7ef1e344224a3c3..b2777c8a007ea7f 100644
--- a/clang/lib/AST/Interp/Interp.h
+++ b/clang/lib/AST/Interp/Interp.h
@@ -1069,15 +1069,18 @@ bool InitThisField(InterpState &S, CodePtr OpPC, uint32_t I) {
   return true;
 }
 
+// FIXME: The Field pointer here is too much IMO and we could instead just
+// pass an Offset + BitWidth pair.
 template <PrimType Name, class T = typename PrimConv<Name>::T>
-bool InitThisBitField(InterpState &S, CodePtr OpPC, const Record::Field *F) {
+bool InitThisBitField(InterpState &S, CodePtr OpPC, const Record::Field *F,
+                      uint32_t FieldOffset) {
   assert(F->isBitField());
   if (S.checkingPotentialConstantExpression())
     return false;
   const Pointer &This = S.Current->getThis();
   if (!CheckThis(S, OpPC, This))
     return false;
-  const Pointer &Field = This.atField(F->Offset);
+  const Pointer &Field = This.atField(FieldOffset);
   const auto &Value = S.Stk.pop<T>();
   Field.deref<T>() = Value.truncate(F->Decl->getBitWidthValue(S.getCtx()));
   Field.initialize();
diff --git a/clang/lib/AST/Interp/Opcodes.td b/clang/lib/AST/Interp/Opcodes.td
index e1e7e5e2efbb059..709de4755af82e1 100644
--- a/clang/lib/AST/Interp/Opcodes.td
+++ b/clang/lib/AST/Interp/Opcodes.td
@@ -416,7 +416,11 @@ def InitThisField : AccessOpcode;
 // [Value] -> []
 def InitThisFieldActive : AccessOpcode;
 // [Value] -> []
-def InitThisBitField : BitFieldOpcode;
+def InitThisBitField : Opcode {
+  let Types = [AluTypeClass];
+  let Args = [ArgRecordField, ArgUint32];
+  let HasGroup = 1;
+}
 // [Pointer, Value] -> []
 def InitField : AccessOpcode;
 // [Pointer, Value] -> []
diff --git a/clang/test/AST/Interp/records.cpp b/clang/test/AST/Interp/records.cpp
index a2e878f6132d0ac..f30722a2ef146ff 100644
--- a/clang/test/AST/Interp/records.cpp
+++ b/clang/test/AST/Interp/records.cpp
@@ -1105,3 +1105,63 @@ namespace DelegatingConstructors {
   static_assert(d4.a == 10, "");
   static_assert(d4.b == 12, "");
 }
+
+namespace IndirectFieldInit {
+#if __cplusplus >= 202002L
+  /// Primitive.
+  struct Nested1 {
+    struct {
+      int first;
+    };
+    int x;
+    constexpr Nested1(int x) : first(12), x() { x = 4; }
+    constexpr Nested1() : Nested1(42) {}
+  };
+  constexpr Nested1 N1{};
+  static_assert(N1.first == 12, "");
+
+  /// Composite.
+  struct Nested2 {
+    struct First { int x = 42; };
+    struct {
+      First first;
+    };
+    int x;
+    constexpr Nested2(int x) : first(12), x() { x = 4; }
+    constexpr Nested2() : Nested2(42) {}
+  };
+  constexpr Nested2 N2{};
+  static_assert(N2.first.x == 12, "");
+
+  /// Bitfield.
+  struct Nested3 {
+    struct {
+      unsigned first : 2;
+    };
+    int x;
+    constexpr Nested3(int x) : first(3), x() { x = 4; }
+    constexpr Nested3() : Nested3(42) {}
+  };
+
+  constexpr Nested3 N3{};
+  static_assert(N3.first == 3, "");
+
+  /// Test that we get the offset right if the
+  /// record has a base.
+  struct Nested4Base {
+    int a;
+    int b;
+    char c;
+  };
+  struct Nested4 : Nested4Base{
+    struct {
+      int first;
+    };
+    int x;
+    constexpr Nested4(int x) : first(123), x() { a = 1; b = 2; c = 3; x = 4; }
+    constexpr Nested4() : Nested4(42) {}
+  };
+  constexpr Nested4 N4{};
+  static_assert(N4.first == 123, "");
+#endif
+}

tbaederr · 2023-11-05T11:28:05Z

Ping

tbaederr · 2023-11-14T05:09:07Z

Ping

tbaederr · 2023-11-20T05:15:53Z

Ping

tbaederr · 2023-11-28T07:59:04Z

Ping

tbaederr · 2023-12-08T14:05:53Z

Ping

tbaederr · 2023-12-14T09:13:21Z

Ping

tbaederr · 2023-12-30T19:57:21Z

Ping

tbaederr · 2024-01-08T07:33:54Z

Ping

tbaederr · 2024-01-15T08:51:27Z

Ping

AaronBallman · 2024-01-15T14:05:37Z

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

+        unsigned NestedFieldOffset = 0;
+        const Record::Field *NestedField = nullptr;
+        for (const NamedDecl *ND : IFD->chain()) {
+          // FIXME: Can this *not* be a FieldDecl?


Yes, it could be an IndirectFieldDecl, for example. You should add some tests using anonymous structures and unions where the members are indirectly injected into the parent structure.

The current interpreter uses cast<FieldDecl> as well, so this should be fine. Unfortunately, unions aren't handled at all in the new interpreter right now, so that could be a FIXME comment at best.

You can run into the same with anonymous structures: https://godbolt.org/z/9565Ea5qo

Nice, thanks for the test case.

AaronBallman

LGTM assuming no testing surprises come up

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

clang/test/AST/Interp/records.cpp

tbaederr force-pushed the indirect-field-init branch from 5b48d59 to 214cb36 Compare October 23, 2023 07:30

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 23, 2023

tbaederr force-pushed the indirect-field-init branch from 214cb36 to af4c29c Compare October 29, 2023 05:22

AaronBallman reviewed Jan 15, 2024

View reviewed changes

tbaederr added 2 commits January 15, 2024 17:13

[clang][Interp] IndirectMember initializers

58ff884

add another test case

92e5857

tbaederr force-pushed the indirect-field-init branch from af4c29c to 92e5857 Compare January 16, 2024 16:34

Remove fixme comment

c26fae9

AaronBallman approved these changes Jan 16, 2024

View reviewed changes

clang/lib/AST/Interp/ByteCodeStmtGen.cpp Outdated Show resolved Hide resolved

clang/test/AST/Interp/records.cpp Show resolved Hide resolved

tbaederr added 2 commits January 16, 2024 17:50

Address review comments

3051c09

Expand on last test case

25dda3a

tbaederr merged commit 819bd9e into llvm:main Jan 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][Interp] IndirectMember initializers #69900

[clang][Interp] IndirectMember initializers #69900

tbaederr commented Oct 23, 2023

llvmbot commented Oct 23, 2023

tbaederr commented Nov 5, 2023

tbaederr commented Nov 14, 2023

tbaederr commented Nov 20, 2023

tbaederr commented Nov 28, 2023

tbaederr commented Dec 8, 2023

tbaederr commented Dec 14, 2023

tbaederr commented Dec 30, 2023

tbaederr commented Jan 8, 2024

tbaederr commented Jan 15, 2024

AaronBallman Jan 15, 2024

tbaederr Jan 15, 2024

AaronBallman Jan 16, 2024

tbaederr Jan 16, 2024

AaronBallman left a comment

[clang][Interp] IndirectMember initializers #69900

[clang][Interp] IndirectMember initializers #69900

Conversation

tbaederr commented Oct 23, 2023

llvmbot commented Oct 23, 2023

tbaederr commented Nov 5, 2023

tbaederr commented Nov 14, 2023

tbaederr commented Nov 20, 2023

tbaederr commented Nov 28, 2023

tbaederr commented Dec 8, 2023

tbaederr commented Dec 14, 2023

tbaederr commented Dec 30, 2023

tbaederr commented Jan 8, 2024

tbaederr commented Jan 15, 2024

AaronBallman Jan 15, 2024

Choose a reason for hiding this comment

tbaederr Jan 15, 2024

Choose a reason for hiding this comment

AaronBallman Jan 16, 2024

Choose a reason for hiding this comment

tbaederr Jan 16, 2024

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment