This is an archive of the discontinued LLVM Phabricator instance.

Differential D14872

PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type
ClosedPublic

Authored by DmitryPolukhin on Nov 20 2015, 6:15 AM.

Details

Reviewers
aaron.ballman
rjmccall
rsmith
Commits
rG830dfccfb2c0: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type…
rC254596: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type…
rL254596: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type…
Summary

This CL is for discussion how to better fix bit-filed layout compatibility issue with GCC (see PR25575 for test case and more details). Current clang behavior is compatible with GCC 4.1-4.3 series but it was fixed in 4.4+. Ignoring packed attribute looks very odd and because it was also fixed in GCC 4.4+, it makes sense also fix it in clang.

Open questions:

  1. Should we add warning about changes in layout for packed bit-fileds of char type? GCC has it "note: offset of packed bit-field ‘b’ has changed in GCC 4.4" will add if needed.
  1. This CL completely removes warning "packed attribute ignored for field of type char". Should we keep it but apply only to normal fields (i.e. not bit-fields)?

This CL removes the warning because IMHO it makes no sense if we fix bit-field case to match GCC 4.4+. Packed attribute was actually ignored only for bit-field. For all other cases it is not ignored but reported only when alignment is already fulfilled (i.e. attribute just is not required and doesn't change anything). But there are tons of cases when packed attribute doesn't change anything but warning is not emitted, for example:
struct S1 {

char a;
char b __attribute((packed)); // warning: 'packed' attribute ignored for field of type 'char'
char c;

};

extern int a[sizeof(struct S1) == 3 ? 1 : -1];
extern int b[__alignof(struct S1) == 1 ? 1 : -1];

struct S2 {

short a;
short b __attribute((packed)); // no warning
short c;

};

extern int c[sizeof(struct S2) == 6 ? 1 : -1];
extern int d[__alignof(struct S2) == 2 ? 1 : -1];

In both cases you can remove __attribute((packed)) and program behavior will not change. But warning is generated only in the first case.

Diff Detail

Event Timeline

DmitryPolukhin updated this revision to Diff 40772.Nov 20 2015, 6:15 AM
DmitryPolukhin retitled this revision from to PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type.
DmitryPolukhin updated this object.
DmitryPolukhin added a reviewer: aaron.ballman.
DmitryPolukhin added a subscriber: cfe-commits.
DmitryPolukhin added a comment.Nov 23 2015, 7:05 AM

It seems that check for type alignment <= 8 was there practically forever http://llvm.org/viewvc/llvm-project/cfe/trunk/Sema/SemaDecl.cpp?r1=47197&r2=47196&pathrev=47197 and there is no good explanation why it was implemented. Subsequent changes only add more condition to exclude cases when it shouldn't be reported.

hfinkel added a subscriber: hfinkel.Nov 24 2015, 3:45 PM

Please upload this patch with full context, see: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Should we add warning about changes in layout for packed bit-fileds of char type? GCC has it "note: offset of packed bit-field ‘b’ has changed in GCC 4.4" will add if needed.

Warning about this is a good idea. We did something similar when we (re-)introduced sync_fetch_and_nand, see:

def warn_sync_fetch_and_nand_semantics_change : Warning<
  "the semantics of this intrinsic changed with GCC "
  "version 4.4 - the newer semantics are provided here">,
  InGroup<DiagGroup<"sync-fetch-and-nand-semantics-changed">>;
DmitryPolukhin updated this revision to Diff 41131.Nov 25 2015, 4:37 AM

Added warning about semantic change + uploaded context. PTAL

hfinkel added inline comments.Nov 25 2015, 1:53 PM
include/clang/Basic/DiagnosticSemaKinds.td
2783

Calling this "a semantic" reads oddly to me. This sounds better to me:

def note_attribute_packed_for_bitfield_offset_changed : Warning<
  "the offset assigned to packed bit-field member %0 has changed with GCC version 4.4 - "
  "the newer offset is used here">,
  InGroup<DiagGroup<"attribute-packed-bitfield-offset-changed">>;
lib/Sema/SemaDeclAttr.cpp
1040

This comment is now out of date?

DmitryPolukhin updated this revision to Diff 41222.Nov 26 2015, 2:47 AM
DmitryPolukhin marked 2 inline comments as done.

Changed note text message + fixed outdated comment.

DmitryPolukhin added reviewers: rsmith, rjmccall.Dec 1 2015, 4:14 AM

This is another GCC ABI compatibility issue. If there is no more comments, could someone please approve this CL?

aaron.ballman added inline comments.Dec 1 2015, 5:54 AM
lib/Sema/SemaDeclAttr.cpp
1046

No need to call getDeclName(), the diagnostic can accept a NamedDecl directly (which properly quotes the name in the diagnostic).

DmitryPolukhin updated this revision to Diff 41510.Dec 1 2015, 7:27 AM
DmitryPolukhin marked an inline comment as done.

Don't call getDeclName() that it is not required. PTAL

rjmccall added inline comments.Dec 1 2015, 11:49 AM
include/clang/Basic/DiagnosticSemaKinds.td
2790

No, this diagnostic shouldn't make such an affirmative statement about the offset changing. I would instead suggest:

warning: 'packed' attribute was ignored on bit-fields with alignment 1 in older versions of GCC and Clang
DmitryPolukhin updated this revision to Diff 41597.Dec 2 2015, 1:16 AM
DmitryPolukhin marked an inline comment as done.

Warning text updated, PTAL.

aaron.ballman added inline comments.Dec 2 2015, 7:56 AM
include/clang/Basic/DiagnosticSemaKinds.td
2788

Would this read better as "on bit-fields with single-byte alignment" instead of "on bit-fields with alignment 1"?

test/Sema/struct-packed-align.c
155

"ignores uses" -> "uses"

DmitryPolukhin updated this revision to Diff 41648.Dec 2 2015, 10:53 AM
DmitryPolukhin marked 2 inline comments as done.

Fixed warning and comment.

rjmccall edited edge metadata.Dec 2 2015, 11:04 AM

LGTM, thanks!

aaron.ballman accepted this revision.Dec 2 2015, 11:15 AM
aaron.ballman edited edge metadata.

LGTM! Thank you!

This revision is now accepted and ready to land.Dec 2 2015, 11:15 AM
Closed by commit rL254596: PR25575: Make GCC 4.4+ comatible layout for packed bit-fileds of char type… (authored by ABataev). · Explain WhyDec 3 2015, 1:37 AM
This revision was automatically updated to reflect the committed changes.
jevinskie added a subscriber: jevinskie.Dec 3 2015, 11:41 AM

Revision Contents

PathSize
include/
clang/
Basic/
7 lines
lib/
Sema/
15 lines
test/
Sema/
23 lines

Diff 41648

include/clang/Basic/DiagnosticSemaKinds.td

Context not available.
"cast to %1 from smaller integer type %0">, "cast to %1 from smaller integer type %0">,
InGroup<IntToVoidPointerCast>; InGroup<IntToVoidPointerCast>;
def warn_attribute_ignored_for_field_of_type : Warning< def warn_attribute_packed_for_bitfield : Warning<
"%0 attribute ignored for field of type %1">, "'packed' attribute was ignored on bit-fields with single-byte alignment "
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

Would this read better as "on bit-fields with single-byte alignment" instead of "on bit-fields with alignment 1"?

aaron.ballman: Would this read better as "on bit-fields with single-byte alignment" instead of "on bit-fields…
InGroup<IgnoredAttributes>; "in older versions of GCC and Clang">,
InGroup<DiagGroup<"attribute-packed-for-bitfield">>;
rjmccallUnsubmitted
Done
ReplyInline Actions

No, this diagnostic shouldn't make such an affirmative statement about the offset changing. I would instead suggest:

warning: 'packed' attribute was ignored on bit-fields with alignment 1 in older versions of GCC and Clang
rjmccall: No, this diagnostic shouldn't make such an affirmative statement about the offset changing. I…
def warn_transparent_union_attribute_field_size_align : Warning< def warn_transparent_union_attribute_field_size_align : Warning<
"%select{alignment|size}0 of field %1 (%2 bits) does not match the " "%select{alignment|size}0 of field %1 (%2 bits) does not match the "
"%select{alignment|size}0 of the first field in transparent union; " "%select{alignment|size}0 of the first field in transparent union; "
Context not available.

lib/Sema/SemaDeclAttr.cpp

Context not available.
TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context, TD->addAttr(::new (S.Context) PackedAttr(Attr.getRange(), S.Context,
Attr.getAttributeSpellingListIndex())); Attr.getAttributeSpellingListIndex()));
else if (FieldDecl *FD = dyn_cast<FieldDecl>(D)) { else if (FieldDecl *FD = dyn_cast<FieldDecl>(D)) {
// If the alignment is less than or equal to 8 bits, the packed attribute // Report warning about changed offset in the newer compiler versions.
// has no effect.
if (!FD->getType()->isDependentType() && if (!FD->getType()->isDependentType() &&
hfinkelUnsubmitted
Done
ReplyInline Actions

This comment is now out of date?

hfinkel: This comment is now out of date?
!FD->getType()->isIncompleteType() && !FD->getType()->isIncompleteType() && FD->isBitField() &&
S.Context.getTypeAlign(FD->getType()) <= 8) S.Context.getTypeAlign(FD->getType()) <= 8)
S.Diag(Attr.getLoc(), diag::warn_attribute_ignored_for_field_of_type) S.Diag(Attr.getLoc(), diag::warn_attribute_packed_for_bitfield);
<< Attr.getName() << FD->getType();
else FD->addAttr(::new (S.Context) PackedAttr(
FD->addAttr(::new (S.Context) Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

No need to call getDeclName(), the diagnostic can accept a NamedDecl directly (which properly quotes the name in the diagnostic).

aaron.ballman: No need to call getDeclName(), the diagnostic can accept a NamedDecl directly (which properly…
PackedAttr(Attr.getRange(), S.Context,
Attr.getAttributeSpellingListIndex()));
} else } else
S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName(); S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
} }
Context not available.

test/Sema/struct-packed-align.c

// RUN: %clang_cc1 %s -fsyntax-only -verify // RUN: %clang_cc1 %s -fsyntax-only -verify
// expected-no-diagnostics // RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
// Packed structs. // Packed structs.
struct s { struct s {
Context not available.
extern int n1[sizeof(struct nS) == 9 ? 1 : -1]; extern int n1[sizeof(struct nS) == 9 ? 1 : -1];
extern int n2[__alignof(struct nS) == 1 ? 1 : -1]; extern int n2[__alignof(struct nS) == 1 ? 1 : -1];
#endif #endif
// Packed attribute shouldn't be ignored for bit-field of char types.
// Note from GCC reference manual: The 4.1, 4.2 and 4.3 series of GCC ignore
// the packed attribute on bit-fields of type char. This has been fixed in
// GCC 4.4 but the change can lead to differences in the structure layout.
// See the documentation of -Wpacked-bitfield-compat for more information.
struct packed_chars {
char a:4;
char b:8 __attribute__ ((packed));
// expected-warning@-1 {{'packed' attribute was ignored on bit-fields with alignment 1 in older versions of GCC and Clang}}
char c:4;
};
#if defined(_WIN32)
// On Windows clang uses MSVC compatible layout in this case.
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

"ignores uses" -> "uses"

aaron.ballman: "ignores uses" -> "uses"
extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
#else
extern int o1[sizeof(struct packed_chars) == 2 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
#endif
Context not available.