[Clang] Handle structs with inner structs and no fields #89126

bwendling · 2024-04-17T19:28:44Z

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++:

struct foo {
struct bar {
int Quantizermatrix[];
};
};

Here 'struct foo' has no fields.

Closes: #88931

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++: struct foo { struct bar { int Quantizermatrix[]; }; }; Here 'struct foo' has no fields.

llvmbot · 2024-04-17T19:29:15Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Bill Wendling (bwendling)

Changes

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++:

struct foo {
struct bar {
int Quantizermatrix[];
};
};

Here 'struct foo' has no fields.

Fixes: #88931

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

3 Files Affected:

(modified) clang/lib/CodeGen/CGBuiltin.cpp (+14-1)
(added) clang/test/CodeGen/attr-counted-by-pr88931.c (+22)
(added) clang/test/CodeGen/attr-counted-by-pr88931.cpp (+21)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index a05874e63c73c2..bee12c4469e7f7 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -829,6 +829,9 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
   unsigned FieldNo = 0;
   bool IsUnion = RD->isUnion();
 
+  if (RD->isImplicit())
+    return nullptr;
+
   for (const Decl *D : RD->decls()) {
     if (const auto *Field = dyn_cast<FieldDecl>(D);
         Field && (Name.empty() || Field->getNameAsString() == Name) &&
@@ -844,7 +847,17 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
       if (const FieldDecl *Field =
               FindFlexibleArrayMemberField(Ctx, Record, Name, Offset)) {
         const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(RD);
-        Offset += Layout.getFieldOffset(FieldNo);
+        if (Layout.getFieldCount()) {
+          // A struct that holds only an inner struct won't have any fields. E.g.
+          //
+          //     struct foo {
+          //         struct bar {
+          //             int count;
+          //             int array[];
+          //         };
+          //     };
+          Offset += Layout.getFieldOffset(FieldNo);
+        }
         return Field;
       }
 
diff --git a/clang/test/CodeGen/attr-counted-by-pr88931.c b/clang/test/CodeGen/attr-counted-by-pr88931.c
new file mode 100644
index 00000000000000..baa1bbc8397456
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr88931.c
@@ -0,0 +1,22 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -Wno-missing-declarations -emit-llvm -o - %s | FileCheck %s
+
+struct foo {
+  struct bar {
+    int count;
+    int array[] __attribute__((counted_by(count)));
+  };
+};
+
+void init(void * __attribute__((pass_dynamic_object_size(0))));
+
+// CHECK-LABEL: define dso_local void @test1(
+// CHECK-SAME: ptr noundef [[P:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[ARRAY:%.*]] = getelementptr inbounds i8, ptr [[P]], i64 4
+// CHECK-NEXT:    tail call void @init(ptr noundef nonnull [[ARRAY]], i64 noundef -1) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:    ret void
+//
+void test1(struct bar *p) {
+  init(p->array);
+}
diff --git a/clang/test/CodeGen/attr-counted-by-pr88931.cpp b/clang/test/CodeGen/attr-counted-by-pr88931.cpp
new file mode 100644
index 00000000000000..2a8cc1d07e50d9
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr88931.cpp
@@ -0,0 +1,21 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -Wall -emit-llvm -o - %s | FileCheck %s
+
+struct foo {
+  struct bar {
+    int array[];
+    bar();
+  };
+};
+
+void init(void * __attribute__((pass_dynamic_object_size(0))));
+
+// CHECK-LABEL: define dso_local void @_ZN3foo3barC1Ev(
+// CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(1) [[THIS:%.*]]) unnamed_addr #[[ATTR0:[0-9]+]] align 2 {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    tail call void @_Z4initPvU25pass_dynamic_object_size0(ptr noundef nonnull [[THIS]], i64 noundef -1) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:    ret void
+//
+foo::bar::bar() {
+  init(array);
+}

github-actions · 2024-04-17T19:31:31Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kees

Tests and logic adjustment looks good to me.

efriedma-quic · 2024-04-18T01:16:49Z

clang/lib/CodeGen/CGBuiltin.cpp

@@ -844,7 +847,18 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
      if (const FieldDecl *Field =


FieldNo and Layout are referring to fields of "RD"; the "Field" found in the recursive visit is a member of Record (or some subobject of Record). So the code is doing math on completely unrelated offsets. Checking getFieldCount is just masking the issue.

Maybe instead of looking for RecordDecls, this code should be looking for fields where the type of the field is an anonymous struct/union.

FieldNo and Layout are referring to fields of "RD"; the "Field" found in the recursive visit is a member of Record (or some subobject of Record). So the code is doing math on completely unrelated offsets. Checking getFieldCount is just masking the issue.

That's not the issue. What's happening is when an inner struct is declared/defined, we recurse within it to try to find the Field offset. If we do find it, then Offset has the offset value within Record. At this point, what we need is the offset up to the RecordDecl, but since those may or may not have a field number associated with them, we use the last FieldNo to get that offset.

A bit clearer:

struct foo { /* <- Passed in as RD */ struct bar { /* <- Not a field, so FieldNo isn't incremented. Recurse on struct bar */ int array[]; /* <- FAM found at offset 0 of struct bar */ }; /* <- Returning with the array FieldDecl, we want to add on any offset associated with the placement of the struct bar definition, but there are no FieldDecls, and so we can't call 'Layout.getFieldOffset()' */ };

This has obvious issues with virtual classes and the like, which is why C++ doesn't officially support FAMs (I believe it's an extension).

To be fair, I had the same question you had. I should document this better.

Maybe instead of looking for RecordDecls, this code should be looking for fields where the type of the field is an anonymous struct/union.

We want to look into all inner structs to find a FAM that may be lurking deep down within the bowels of the struct, which may involve non-anonymous structs.

I did several tests, and it looks like if there's an inner struct that's not accessible, that doesn't affect the offsets of fields outside of that inner struct.

struct foo { struct inner_1 { /* fields */ }; struct inner_2 { int a; /* __builtin_offsetof is 0 */ }; int b; /* __builtin_offsetof is 0 */ };

@efriedma-quic Do you have any thoughts?

if there's an inner struct that's not accessible, that doesn't affect the offsets of fields outside of that inner struct.

Yes, the only thing that's relevant to offsets in a struct is the fields. A struct definition inside another struct declaration has exactly the same semantics as a struct definition anywhere else (unless it's an anonymous struct/union).

Maybe also try the following testcase.

struct foo { int x,y,z; struct bar { int count; int array[] __attribute__((counted_by(count))); }; }; void init(void * __attribute__((pass_dynamic_object_size(0)))); void test1(struct bar *p) { init(p->array); }

efriedma-quic

We should probably apply the same fix to CountCountedByAttrs.

Along those lines, we should be able to handle:

struct bar {
  int count;
  int array[] __attribute__((counted_by(count)));
};
struct foo { struct bar x; };
void init(void * __attribute__((pass_dynamic_object_size(0))));
void test1(struct foo *p) {
  init(p);
}

clang/lib/CodeGen/CGBuiltin.cpp

efriedma-quic · 2024-04-18T21:10:37Z

clang/test/CodeGen/attr-counted-by-pr88931.c

+struct foo {
+  struct bar {
+    int count;
+    int array[] __attribute__((counted_by(count)));


Ideally, we should be able to compute the size here; would need to change the way we compute the "outer" type.

Can leave that for a followup, I guess.

We can, if handling struct bar on its own. It's just there's seemingly no way in C to access the fields in struct bar from struct foo...

The only reason foo is relevant in the first place is that getOuterLexicalRecordContext() skips over bar.

I see what you mean. I'd like to leave it for a followup, since this change is meant to fix an ICE.

bwendling · 2024-04-18T21:17:19Z

We should probably apply the same fix to CountCountedByAttrs.

Along those lines, we should be able to handle:
struct bar {
  int count;
  int array[] __attribute__((counted_by(count)));
};
struct foo { struct bar x; };
void init(void * __attribute__((pass_dynamic_object_size(0))));
void test1(struct foo *p) {
  init(p);
}

I can support that. I'll add it as a testcase.

efriedma-quic · 2024-04-18T22:11:15Z

clang/test/CodeGen/attr-counted-by-pr88931.c

+// CHECK-NEXT:    tail call void @init(ptr noundef [[P]], i64 noundef -1) #[[ATTR2]]
+// CHECK-NEXT:    ret void
+//
+void test2(struct foo *p) {


Supposed to be bux*?

Doh! Yes. Done.

efriedma-quic

LGTM

Please don't forget to fix CountCountedByAttrs... but it's fine if it's in a followup.

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++: struct foo { struct bar { int Quantizermatrix[]; }; }; Here 'struct foo' has no fields. Closes: llvm#88931 (cherry picked from commit c32712d)

efriedma-quic · 2024-04-19T21:43:06Z

clang/lib/CodeGen/CGBuiltin.cpp

+    return nullptr;
+
+  for (const FieldDecl *FD : RD->fields()) {
+    if ((Name.empty() || FD->getNameAsString() == Name) &&


Minor question I thought of looking at this one more time for the backport; why are we checking the name of the FieldDecl, instead just checking pointer equality? The caller has a FieldDecl, I think.

I think I did it this way to support searching from either a pointer to the whole struct or pointer to the FAM. I suppose running this when we know we're pointing to the FAM is a bit redundant. And yes, using a FieldDecl pointer instead is almost certainly better here.

I think what I want to do though is rework some of the code so that we support __builtin_dynamic_object_size for more than just pointing to either the FAM or struct. I'll make that change as well.

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++: struct foo { struct bar { int Quantizermatrix[]; }; }; Here 'struct foo' has no fields. Closes: llvm#88931

bwendling · 2024-04-25T20:51:36Z

@tstellar PR #90118 is the fix here.

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++: struct foo { struct bar { int Quantizermatrix[]; }; }; Here 'struct foo' has no fields. Closes: #88931

A struct that declares an inner struct, but no fields, won't have a field count. So getting the offset of the inner struct fails. This happens in both C and C++: struct foo { struct bar { int Quantizermatrix[]; }; }; Here 'struct foo' has no fields. Closes: llvm#88931 (cherry picked from commit c32712d)

bwendling requested review from nickdesaulniers, AaronBallman and efriedma-quic April 17, 2024 19:28

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 17, 2024

Reformat

fbb22e6

kees self-requested a review April 18, 2024 00:31

kees approved these changes Apr 18, 2024

View reviewed changes

efriedma-quic reviewed Apr 18, 2024

View reviewed changes

Improve calculation by focusing only on FieldDecls.

4c4046f

efriedma-quic reviewed Apr 18, 2024

View reviewed changes

bwendling added 4 commits April 18, 2024 14:18

Simplify

350550b

Fix small error

d3eda51

Improve testcase.

dad1c32

Add new testcase

1613ce6

efriedma-quic reviewed Apr 18, 2024

View reviewed changes

Use correct struct.

f9b3bf3

efriedma-quic approved these changes Apr 19, 2024

View reviewed changes

Merge branch 'llvm:main' into pr88931

1b79233

bwendling merged commit c32712d into llvm:main Apr 19, 2024
3 of 4 checks passed

bwendling deleted the pr88931 branch April 19, 2024 19:48

efriedma-quic reviewed Apr 19, 2024

View reviewed changes

bwendling mentioned this pull request Apr 25, 2024

Fix for merging PR #89456 into llvm 18.X #90118

Closed

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Handle structs with inner structs and no fields #89126

[Clang] Handle structs with inner structs and no fields #89126

bwendling commented Apr 17, 2024 •

edited

Loading

llvmbot commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading

kees left a comment

efriedma-quic Apr 18, 2024

efriedma-quic Apr 18, 2024

bwendling Apr 18, 2024

bwendling Apr 18, 2024

efriedma-quic Apr 18, 2024 •

edited

Loading

efriedma-quic left a comment

efriedma-quic Apr 18, 2024

bwendling Apr 18, 2024

efriedma-quic Apr 18, 2024

bwendling Apr 19, 2024

bwendling commented Apr 18, 2024

efriedma-quic Apr 18, 2024

bwendling Apr 19, 2024

efriedma-quic left a comment

efriedma-quic Apr 19, 2024

bwendling Apr 19, 2024

bwendling commented Apr 25, 2024

		@@ -844,7 +847,18 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
		if (const FieldDecl *Field =

[Clang] Handle structs with inner structs and no fields #89126

[Clang] Handle structs with inner structs and no fields #89126

Conversation

bwendling commented Apr 17, 2024 • edited Loading

llvmbot commented Apr 17, 2024 • edited Loading

github-actions bot commented Apr 17, 2024 • edited Loading

kees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efriedma-quic Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwendling commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwendling commented Apr 25, 2024

bwendling commented Apr 17, 2024 •

edited

Loading

llvmbot commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading

efriedma-quic Apr 18, 2024 •

edited

Loading