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 all 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
25 changes: 14 additions & 11 deletions clang/lib/CodeGen/CGBuiltin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,29 +826,32 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
ASTContext &Ctx, const RecordDecl *RD, StringRef Name, uint64_t &Offset) {
const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
getLangOpts().getStrictFlexArraysLevel();
unsigned FieldNo = 0;
bool IsUnion = RD->isUnion();
uint32_t FieldNo = 0;

for (const Decl *D : RD->decls()) {
if (const auto *Field = dyn_cast<FieldDecl>(D);
Field && (Name.empty() || Field->getNameAsString() == Name) &&
if (RD->isImplicit())
return nullptr;

for (const FieldDecl *FD : RD->fields()) {
if ((Name.empty() || FD->getNameAsString() == Name) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

Decl::isFlexibleArrayMemberLike(
Ctx, Field, Field->getType(), StrictFlexArraysLevel,
Ctx, FD, FD->getType(), StrictFlexArraysLevel,
/*IgnoreTemplateOrMacroSubstitution=*/true)) {
const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(RD);
Offset += Layout.getFieldOffset(FieldNo);
return Field;
return FD;
}

if (const auto *Record = dyn_cast<RecordDecl>(D))
if (const FieldDecl *Field =
FindFlexibleArrayMemberField(Ctx, Record, Name, Offset)) {
QualType Ty = FD->getType();
if (Ty->isRecordType()) {
if (const FieldDecl *Field = FindFlexibleArrayMemberField(
Ctx, Ty->getAsRecordDecl(), Name, Offset)) {
const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(RD);
Offset += Layout.getFieldOffset(FieldNo);
return Field;
}
}

if (!IsUnion && isa<FieldDecl>(D))
if (!RD->isUnion())
++FieldNo;
}

Expand Down
40 changes: 40 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,40 @@
// 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 {
int x,y,z;
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);
}

struct mux {
int count;
int array[] __attribute__((counted_by(count)));
};

struct bux { struct mux x; };

// CHECK-LABEL: define dso_local void @test2(
// CHECK-SAME: ptr noundef [[P:%.*]]) local_unnamed_addr #[[ATTR0]] {
// CHECK-NEXT: entry:
// CHECK-NEXT: tail call void @init(ptr noundef [[P]], i64 noundef -1) #[[ATTR2]]
// CHECK-NEXT: ret void
//
void test2(struct bux *p) {
init(p);
}
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