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

flag non-portable bitfields #20840

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

flag non-portable bitfields #20840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 43 additions & 18 deletions compiler/src/dmd/dsymbolsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7026,12 +7026,14 @@
AggregateDeclaration ad;
FieldState* fieldState;
bool isunion;
LINK linkage;

this(AggregateDeclaration ad, FieldState* fieldState, bool isunion) @safe
{
this.ad = ad;
this.fieldState = fieldState;
this.isunion = isunion;
this.linkage = LINK.d;
}

override void visit(Dsymbol d) {}
Expand Down Expand Up @@ -7114,8 +7116,8 @@

const sz = t.size(vd.loc);
assert(sz != SIZE_INVALID && sz < uint.max);
uint memsize = cast(uint)sz; // size of member
uint memalignsize = target.fieldalign(t); // size of member for alignment purposes
const memsize = cast(uint)sz; // size of member
const memalignsize = target.fieldalign(t); // size of member for alignment purposes
vd.offset = placeField(vd.loc,
fieldState.offset,
memsize, memalignsize, vd.alignment,
Expand Down Expand Up @@ -7153,8 +7155,8 @@

const sz = t.size(bfd.loc);
assert(sz != SIZE_INVALID && sz < uint.max);
uint memsize = cast(uint)sz; // size of member
uint memalignsize = target.fieldalign(t); // size of member for alignment purposes
const uint memsize = cast(uint)sz; // size of member
const uint memalignsize = target.fieldalign(t); // size of member for alignment purposes
if (log) printf(" memsize: %u memalignsize: %u\n", memsize, memalignsize);

if (bfd.fieldWidth == 0 && !anon)
Expand All @@ -7167,7 +7169,20 @@
assert(0, "unsupported bit-field style");

const isMicrosoftStyle = style == TargetC.BitFieldStyle.MS;
const contributesToAggregateAlignment = target.c.contributesToAggregateAlignment(bfd);

/**
* Indicates whether the specified bit-field contributes to the alignment
* of the containing aggregate.
* E.g., (not all) ARM ABIs do NOT ignore anonymous (incl. 0-length)
* bit-fields.
*/
const bool contributesToAggregateAlignment = isMicrosoftStyle || !bfd.isAnonymous; // sufficient for supported architectures
Copy link
Member

Choose a reason for hiding this comment

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

This is target specific though. Each compiler back-end makes its own decision about this.

Copy link
Member

Choose a reason for hiding this comment

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

For example, in GCC, back-ends set a PCC_BITFIELD_TYPE_MATTERS macro to notify us that type affects alignment. Then TARGET_ALIGN_ANON_BITFIELD if anonymous bitfields affect alignment as well.

Taking this away would likely regress data layouts on those back-ends.

Copy link
Member Author

Choose a reason for hiding this comment

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

What matters is which ones are actually different.


/* Flag bitfield non-portable layouts that differ between MicrosoftStyle and non-MicrosoftStyle
* unless it's C semantics.
*/
const checkPortable = !ad.isCsymbol() && linkage == LINK.d;
bool isPortable = true; // start out assuming portable

void startNewField()
{
Expand Down Expand Up @@ -7208,19 +7223,16 @@

if (bfd.fieldWidth == 0)
{
if (!isMicrosoftStyle && !isunion)
if (!isunion)
{
// Use type of zero width field to align to next field
fieldState.offset = (fieldState.offset + memalignsize - 1) & ~(memalignsize - 1);
ad.structsize = fieldState.offset;
}
else if (isMicrosoftStyle && fieldState.inFlight && !isunion)
{
// documentation says align to next int
//const alsz = cast(uint)Type.tint32.size();
const alsz = memsize; // but it really does this
fieldState.offset = (fieldState.offset + alsz - 1) & ~(alsz - 1);
ad.structsize = fieldState.offset;
if (!isMicrosoftStyle || fieldState.inFlight)
{
const alsz = isMicrosoftStyle
? memsize // documentation says align to next int but memsize is actually used
: memalignsize; // use type of zero width field to align to next field

Check warning on line 7232 in compiler/src/dmd/dsymbolsem.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/dsymbolsem.d#L7232 

Added line #L7232 was not covered by tests
fieldState.offset = (fieldState.offset + alsz - 1) & ~(alsz - 1);
ad.structsize = fieldState.offset;
}
}

fieldState.inFlight = false;
Expand All @@ -7229,7 +7241,7 @@

if (!fieldState.inFlight)
{
//printf("not in flight\n");
if (log) printf("not in flight\n");
startNewField();
}
else if (!isMicrosoftStyle)
Expand Down Expand Up @@ -7296,6 +7308,11 @@
fieldState.bitOffset = pastField;
}

if (checkPortable && !isPortable)
{
error(bfd.loc, "bitfield layout not portable among supported C compilers, wrap with `extern (C)` or `extern (C++)`");

Check warning on line 7313 in compiler/src/dmd/dsymbolsem.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/dsymbolsem.d#L7313 

Added line #L7313 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably mention other ABI's like Windows i.e.

Suggested change
error(bfd.loc, "bitfield layout not portable among supported C compilers, wrap with `extern (C)` or `extern (C++)`");
error(bfd.loc, "bitfield layout not portable among supported C compilers, wrap with extern (X) where X is C, C++, Windows, or System");

}

//printf("\t%s: offset = %d bitOffset = %d fieldWidth = %d memsize = %d\n", toChars(), offset, bitOffset, fieldWidth, memsize);
//printf("\t%s: memalignsize = %d\n", toChars(), memalignsize);
//printf(" addField '%s' to '%s' at offset %d, size = %d\n", toChars(), ad.toChars(), offset, memsize);
Expand All @@ -7315,6 +7332,14 @@
atd.include(null).foreachDsymbol( s => s.setFieldOffset(ad, fieldState, isunion) );
}

override void visit(LinkDeclaration atd)
{
LINK save = linkage;
linkage = atd.linkage;
atd.include(null).foreachDsymbol( s => s.setFieldOffset(ad, fieldState, isunion) );
linkage = save;
}

override void visit(AnonDeclaration anond)
{
//printf("\tAnonDeclaration::setFieldOffset %s %p\n", isunion ? "union" : "struct", anond);
Expand Down
1 change: 0 additions & 1 deletion compiler/src/dmd/frontend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6021,7 +6021,6 @@ struct TargetC final
uint8_t wchar_tsize;
Runtime runtime;
BitFieldStyle bitFieldStyle;
bool contributesToAggregateAlignment(BitFieldDeclaration* bfd);
TargetC() :
crtDestructorsSupported(true),
boolsize(),
Expand Down
18 changes: 0 additions & 18 deletions compiler/src/dmd/target.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1463,24 +1463,6 @@ struct TargetC
crtDestructorsSupported = false;
}
}

/**
* Indicates whether the specified bit-field contributes to the alignment
* of the containing aggregate.
* E.g., (not all) ARM ABIs do NOT ignore anonymous (incl. 0-length)
* bit-fields.
*/
extern (C++) bool contributesToAggregateAlignment(BitFieldDeclaration bfd)
{
if (bitFieldStyle == BitFieldStyle.MS)
return true;
if (bitFieldStyle == BitFieldStyle.Gcc_Clang)
{
// sufficient for DMD's currently supported architectures
return !bfd.isAnonymous();
}
assert(0);
}
}

////////////////////////////////////////////////////////////////////////////////
Expand Down
Loading