https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52991
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jakub at gcc dot gnu.org
--- Comment #27 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The #c26 tests in a single testcase:
#ifdef _WIN32
# ifdef _MSC_VER
# define PACK(typeDec) __pragma( pack(push, 1) ) typeDec __pragma( pack(pop) )
# else
# define PACK(typeDec) typeDec __attribute__((__packed__,ms_struct))
# endif
#else
# define PACK(typeDec) typeDec __attribute__((__packed__))
#endif
#ifdef _MSC_VER
# define ALIGN(typeDec, n) __declspec(align(n)) typeDec
#else
# define ALIGN(typeDec, n) typeDec __attribute__((aligned(n)))
#endif
#define assert_cc(expr) extern char c[(expr) ? 1 : -1]
#define offsetof(x, y) __builtin_offsetof (x, y)
PACK(struct test_sp1 {
int a;
short b;
int c;
char d;
});
assert_cc(sizeof(struct test_sp1) == 11);
assert_cc(offsetof(struct test_sp1, a) == 0);
assert_cc(offsetof(struct test_sp1, b) == 4);
assert_cc(offsetof(struct test_sp1, c) == 6);
assert_cc(offsetof(struct test_sp1, d) == 10);
PACK(struct test_sp3 {
int a;
ALIGN(short b, 8);
int c;
char d;
});
assert_cc(sizeof(struct test_sp3) == 16);
assert_cc(offsetof(struct test_sp3, a) == 0);
assert_cc(offsetof(struct test_sp3, b) == 8);
assert_cc(offsetof(struct test_sp3, c) == 10);
assert_cc(offsetof(struct test_sp3, d) == 14);
struct test_s4 {
int a;
short b;
int c:15;
char d;
};
#ifdef _WIN32
assert_cc(sizeof(struct test_s4) == 16);
assert_cc(offsetof(struct test_s4, a) == 0);
assert_cc(offsetof(struct test_s4, b) == 4);
assert_cc(offsetof(struct test_s4, d) == 12);
#else
assert_cc(sizeof(struct test_s4) == 12);
assert_cc(offsetof(struct test_s4, a) == 0);
assert_cc(offsetof(struct test_s4, b) == 4);
assert_cc(offsetof(struct test_s4, d) == 8);
#endif
but rather with r114364 which revamped the ms bitfield layout completely.
See https://gcc.gnu.org/ml/gcc-patches/2006-04/msg01064.html for more info.
The above testcase indeed passes with -D_WIN32 -mms-bitfields when built with
GCC 4.1 or earlier.
Ignoring the type_align bump in place_field for DECL_PACKED fields sounds
reasonable, just the formatting is wrong (needs a space in between DECL_PACKED
and (field)).
In the second hunk, the formatting is also wrong (e.g. a single statement body
of else shouldn't be wrapped with {}s around it), but more importantly it is
unclear why that would be the right thing, e.g. why is ignoring
maximum_field_alignment the right thing etc.
So, for the first two hunks in #c26 I'd go with:
--- gcc/stor-layout.c.jj 2018-02-22 14:35:33.135216198 +0100
+++ gcc/stor-layout.c 2018-02-27 15:14:03.271387867 +0100
@@ -1038,7 +1038,7 @@ update_alignment_for_field (record_layou
the type, except that for zero-size bitfields this only
applies if there was an immediately prior, nonzero-size
bitfield. (That's the way it is, experimentally.) */
- if ((!is_bitfield && !DECL_PACKED (field))
+ if (!is_bitfield
|| ((DECL_SIZE (field) == NULL_TREE
|| !integer_zerop (DECL_SIZE (field)))
? !DECL_PACKED (field)
@@ -1047,7 +1047,10 @@ update_alignment_for_field (record_layou
&& ! integer_zerop (DECL_SIZE (rli->prev_field)))))
{
unsigned int type_align = TYPE_ALIGN (type);
- type_align = MAX (type_align, desired_align);
+ if (!is_bitfield && DECL_PACKED (field))
+ type_align = desired_align;
+ else
+ type_align = MAX (type_align, desired_align);
if (maximum_field_alignment != 0)
type_align = MIN (type_align, maximum_field_alignment);
rli->record_align = MAX (rli->record_align, type_align);
@@ -1555,7 +1558,8 @@ place_field (record_layout_info rli, tre
}
/* Now align (conventionally) for the new type. */
- type_align = TYPE_ALIGN (TREE_TYPE (field));
+ if (! DECL_PACKED (field))
+ type_align = TYPE_ALIGN (TREE_TYPE (field));
if (maximum_field_alignment != 0)
type_align = MIN (type_align, maximum_field_alignment);
instead.
The #c26 third hunk is I presume instead something that was probably broken
only in GCC 4.6 with r184409 aka PR52238 fix. That
if (targetm.ms_bitfield_layout_p (rli->t))
rli->prev_field = NULL;
in that patch looks wrong to me, you lose all the special handling of
rli->prev_field that way.
Before trying to deal with that, I think it is important to figure out what VC
does for the case when a bitfield is followed by another bitfield with the same
underlying type size, but higher alignment.
E.g. how is:
struct S {
int a : 2;
__declspec(align(8)) int b : 2;
int c : 28;
__declspec(align(16)) int d : 2;
int e : 30;
} s;
int a = sizeof (struct S);
void f1 (int x) { s.a = x; }
void f2 (int x) { s.b = x; }
void f3 (int x) { s.c = x; }
void f4 (int x) { s.d = x; }
void f5 (int x) { s.e = x; }
compiled by VC? Godbolt.org seems to be down right now, checking some clang
version with -fms-extensions -mms-bitfields shows that the differences in type
alignments don't break the bitfield packs, a, b, c are all packed into a single
32-bit word at offset 0, then d and e are packed into another 32-bit word at
offset 16 (so the alignment is honored for the first bitfield in the pack).
If that is what VC does, then one way to deal with this would be to replace:
if (known_align < desired_align)
with
if (known_align < desired_align
&& (!targetm.ms_bitfield_layout_p (rli->t)
|| rli->prev_field == NULL))
drop the
if (targetm.ms_bitfield_layout_p (rli->t))
rli->prev_field = NULL;
and deal with the further alignment (basically duplicate the if (known_align <
desired_align body)) later.
