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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix: isZeroInit does not take into account unions #16858

Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion compiler/src/dmd/aggregate.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ extern (C++) abstract class AggregateDeclaration : ScopeDsymbol
StorageClass storage_class; ///
uint structsize; /// size of struct
uint alignsize; /// size of struct for alignment purposes
VarDeclarations fields; /// VarDeclaration fields
VarDeclarations fields; /// VarDeclaration fields including flattened AnonDeclaration members
Dsymbol deferred; /// any deferred semantic2() or semantic3() symbol

/// specifies whether this is a D, C++, Objective-C or anonymous struct/class/interface
Expand Down
14 changes: 10 additions & 4 deletions 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,18 @@ extern (C++) class StructDeclaration : AggregateDeclaration

// Determine if struct is all zeros or not
zeroInit = true;
auto lastOffset = -1;
foreach (vd; fields)
{
// First skip zero sized fields
if (vd.type.size(vd.loc) == 0)
continue;

// only consider first sized member of an (anonymous) union
if (vd.overlapped && vd.offset == lastOffset)
continue;
lastOffset = vd.offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! - I meant the previous field's size though; I guess this fails (wrongly treated as zero-initialized):

struct {
    int[0] dummy; // same offset as anon union, but doesn't overlap; should be ignored anyway for zero-init check
    union {
        float f; // is the first member of the anon union and must be checked
        int i;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

[So a simple early if (vd.type.size(vd.loc) == 0) continue; at the iteration start should do the job, shortcutting the check for 0-length static arrays and not setting lastOffset, so that the first union member is always checked.]

Copy link
Contributor Author

@ntrel ntrel Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oops, thanks. Added that test. It is a lot better, but there are (less common) cases where it wrongly doesn't zero init:

union U3
{
    int y;
    struct {
        float z, w; // z ignored but w has different offset to x
    }
}

If this is merged, I'll file that case separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oof yeah, no idea how we'd easily fix those cases.


if (vd._init)
{
if (vd._init.isVoidInitializer())
Expand All @@ -360,10 +370,6 @@ extern (C++) class StructDeclaration : AggregateDeclaration
*/
continue;

// Zero size fields are zero initialized
if (vd.type.size(vd.loc) == 0)
continue;

// Examine init to see if it is all 0s.
auto exp = vd.getConstInitializer();
if (!exp || !_isZeroInit(exp))
Expand Down
42 changes: 42 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,45 @@ 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;
}
}
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));

struct S8 {
int[0] dummy; // same offset as anon union, but doesn't overlap; should be ignored anyway for zero-init check
union {
float f; // is the first member of the anon union and must be checked
int i;
}
}
static assert(!__traits(isZeroInit, S8));
Loading