Skip to content
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

Jump to bottom

Fix: isZeroInit does not take into account unions #16858

Merged
merged 6 commits into from
Oct 5, 2024
Merged

Fix: isZeroInit does not take into account unions #16858

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cbb7cb6
Bugzilla 24776 - Struct with anonymous union has wrong isZeroInit
ntrel Sep 20, 2024
261044e
Fix Bugzilla 23841 - isZeroInit does not take into account unions
ntrel Sep 20, 2024
18790f6
Undo fix for anonymous union, breaks other code
ntrel Sep 21, 2024
3d2ac20
Ignore zero-sized fields
ntrel Sep 23, 2024
179da5d
Fix Bugzilla 24776 - Struct with anonymous union has wrong isZeroInit
ntrel Sep 23, 2024
e606fad
Fix checking first member of union after zero size field
ntrel Sep 24, 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
5 changes: 4 additions & 1 deletion compiler/src/dmd/dstruct.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,11 @@ extern (C++) class StructDeclaration : AggregateDeclaration

// Determine if struct is all zeros or not
zeroInit = true;
foreach (vd; fields)
foreach (i, vd; fields)
{
// union init is same as first member
if (i == 1 && isUnionDeclaration())
break;
if (vd._init)
{
if (vd._init.isVoidInitializer())
Expand Down
36 changes: 36 additions & 0 deletions compiler/test/compilable/isZeroInit.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,39 @@ static if (is(Vector!(int[4])))
static assert(__traits(isZeroInit, Holder!(Vector!(int[4]), 0)));
static assert(!__traits(isZeroInit, Holder!(Vector!(int[4]), 1)));
}

// https://issues.dlang.org/show_bug.cgi?id=24776
struct S6 {
union {
int i1;
float f1;
}
}
//FIXME
//static assert(__traits(isZeroInit, S6));

struct S7
{
union {
float f2;
int i2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the current problem (CI errors) would be uncovered by adding a regular zero-initialized field to S7 here, before the anonymous union. That shows that you cannot simply break out of the loop if the 2nd field happens to be overlapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. There doesn't seem to be a way to detect anonymous unions easily at the semantic stage. So this no longer fixes anonymous unions but does fix named unions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. It was weird that the CI didn't seem to be reporting clear errors about what had broken.

Copy link
Contributor

@kinke kinke Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There doesn't seem to be a way to detect anonymous unions easily at the semantic stage.

Perhaps checking whether the byte offset is the same as the previous field's - and whether the field is overlapped, due to bitfields. Edit: Oh, and that the previous field isn't empty (T[0]).

Copy link
Member

Choose a reason for hiding this comment

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

Yes. There doesn't seem to be a way to detect anonymous unions easily at the semantic stage. So this no longer fixes anonymous unions but does fix named unions.

FYI fields is a flattened array of all found field vars, but members is the full parse tree. There's an extra cost to traverse recursively over all members though.

Copy link
Contributor Author

@ntrel ntrel Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks both for the info. Perhaps I will look at fixing anonymous unions in another pull.

and that the previous field isn't empty (T[0]).

I didn't realize that, taking account of it (seems to have) fixed this pull.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing anonymous unions

Now done!

}
static assert(!__traits(isZeroInit, S7));

// https://issues.dlang.org/show_bug.cgi?id=23841
union U
{
float x = 0;
float y;
}

static assert(__traits(isZeroInit, U));

union U2
{
float x;
int y;
}

static assert(!__traits(isZeroInit, U2));
Loading