Skip to content

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

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.

Sign up for GitHub

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 9 commits into from
Apr 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
36ddb58
[Clang] Handle structs with inner structs and no fields
bwendling Apr 17, 2024
fbb22e6
Reformat
bwendling Apr 17, 2024
4c4046f
Improve calculation by focusing only on FieldDecls.
bwendling Apr 18, 2024
350550b
Simplify
bwendling Apr 18, 2024
d3eda51
Fix small error
bwendling Apr 18, 2024
dad1c32
Improve testcase.
bwendling Apr 18, 2024
1613ce6
Add new testcase
bwendling Apr 18, 2024
f9b3bf3
Use correct struct.
bwendling Apr 19, 2024
1b79233
Merge branch 'llvm:main' into pr88931
bwendling Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion clang/lib/CodeGen/CGBuiltin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) &&
Expand All @@ -844,7 +847,18 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
if (const FieldDecl *Field =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

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;
}

Expand Down
22 changes: 22 additions & 0 deletions clang/test/CodeGen/attr-counted-by-pr88931.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
};

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);
}
21 changes: 21 additions & 0 deletions clang/test/CodeGen/attr-counted-by-pr88931.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
Loading