* [PATCH] Fix handling of variable length fields in structures (PR c/82210)
@ 2018-02-09 6:22 Jakub Jelinek
2018-02-13 18:40 ` Jeff Law
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2018-02-09 6:22 UTC (permalink / raw)
To: Richard Biener, Jeff Law; +Cc: gcc-patches
Hi!
When placing a variable length field into a structure, we need to update
rli->offset_align for the next field. We do:
rli->offset_align = MIN (rli->offset_align, desired_align);
which updates it according to the start of that VLA field, the problem is
that if the field doesn't have a size that is a multiple of this alignment
rli->offset_align will not reflect properly the alignment of the end of that
field. E.g. on the testcase, we have a VLA array aligned as a whole (the
field itself) to 16 bytes / 128 bits, so rli->offset_align remains 128.
The array has element size 2 bytes / 16 bits, times function argument,
so the end of the field is worst case aligned just to 16 bits; if we keep
rli->offset_align as 128 for the next field, then DECL_OFFSET_ALIGN is too
large. DECL_FIELD_OFFSET documented as:
/* In a FIELD_DECL, this is the field position, counting in bytes, of the
DECL_OFFSET_ALIGN-bit-sized word containing the bit closest to the beginning
of the structure. */
and when gimplifying COMPONENT_REFs with that field we:
tree offset = unshare_expr (component_ref_field_offset (t));
tree field = TREE_OPERAND (t, 1);
tree factor
= size_int (DECL_OFFSET_ALIGN (field) / BITS_PER_UNIT);
/* Divide the offset by its alignment. */
offset = size_binop_loc (loc, EXACT_DIV_EXPR, offset, factor);
and later on multiply it again by DECL_OFFSET_ALIGN. The EXACT_DIV_EXPR
isn't exact.
Fixed by lowering the rli->offset_align if the size isn't a multiple of
the align. We don't have a multiple_of_p variant that would compute
highest power of two number the expression is known to be a multiple of,
so I'm just checking the most common case, where the size is a multiple
of the starting alignment, and otherwise just compute it very
conservatively. This will be lower than necessary say for
__attribute__((aligned (16))) short field[2 * size];
- just 16 bits instead of 32. In theory we could do a binary search
on power of two numbers in between that high initial rli->offset_align
for which the first multiple_of_p failed, and the conservative guess
we do to improve it. If you think it is worth it, I can code it up.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2018-02-09 Jakub Jelinek <jakub@redhat.com>
PR c/82210
* stor-layout.c (place_field): For variable length fields, adjust
offset_align afterwards not just based on the field's alignment,
but also on the size.
* gcc.c-torture/execute/pr82210.c: New test.
--- gcc/stor-layout.c.jj 2018-01-16 16:07:57.000000000 +0100
+++ gcc/stor-layout.c 2018-02-08 13:48:32.380582662 +0100
@@ -1622,6 +1622,30 @@ place_field (record_layout_info rli, tre
= size_binop (PLUS_EXPR, rli->offset, DECL_SIZE_UNIT (field));
rli->bitpos = bitsize_zero_node;
rli->offset_align = MIN (rli->offset_align, desired_align);
+
+ if (!multiple_of_p (bitsizetype, DECL_SIZE (field),
+ bitsize_int (rli->offset_align)))
+ {
+ tree type = strip_array_types (TREE_TYPE (field));
+ /* The above adjusts offset_align just based on the start of the
+ field. The field might not have a size that is a multiple of
+ that offset_align though. If the field is an array of fixed
+ sized elements, assume there can be any multiple of those
+ sizes. If it is a variable length aggregate or array of
+ variable length aggregates, assume worst that the end is
+ just BITS_PER_UNIT aligned. */
+ if (TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
+ {
+ if (TREE_INT_CST_LOW (TYPE_SIZE (type)))
+ {
+ unsigned HOST_WIDE_INT sz
+ = least_bit_hwi (TREE_INT_CST_LOW (TYPE_SIZE (type)));
+ rli->offset_align = MIN (rli->offset_align, sz);
+ }
+ }
+ else
+ rli->offset_align = MIN (rli->offset_align, BITS_PER_UNIT);
+ }
}
else if (targetm.ms_bitfield_layout_p (rli->t))
{
--- gcc/testsuite/gcc.c-torture/execute/pr82210.c.jj 2018-02-08 13:59:37.247901958 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr82210.c 2018-02-08 13:59:14.185912469 +0100
@@ -0,0 +1,26 @@
+/* PR c/82210 */
+
+void
+foo (int size)
+{
+ int i;
+ struct S {
+ __attribute__((aligned (16))) struct T { short c; } a[size];
+ int b[size];
+ } s;
+
+ for (i = 0; i < size; i++)
+ s.a[i].c = 0x1234;
+ for (i = 0; i < size; i++)
+ s.b[i] = 0;
+ for (i = 0; i < size; i++)
+ if (s.a[i].c != 0x1234 || s.b[i] != 0)
+ __builtin_abort ();
+}
+
+int
+main ()
+{
+ foo (15);
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] Fix handling of variable length fields in structures (PR c/82210)
2018-02-09 6:22 [PATCH] Fix handling of variable length fields in structures (PR c/82210) Jakub Jelinek
@ 2018-02-13 18:40 ` Jeff Law
0 siblings, 0 replies; 2+ messages in thread
From: Jeff Law @ 2018-02-13 18:40 UTC (permalink / raw)
To: Jakub Jelinek, Richard Biener; +Cc: gcc-patches
On 02/08/2018 11:22 PM, Jakub Jelinek wrote:
> Hi!
>
> When placing a variable length field into a structure, we need to update
> rli->offset_align for the next field. We do:
> rli->offset_align = MIN (rli->offset_align, desired_align);
> which updates it according to the start of that VLA field, the problem is
> that if the field doesn't have a size that is a multiple of this alignment
> rli->offset_align will not reflect properly the alignment of the end of that
> field. E.g. on the testcase, we have a VLA array aligned as a whole (the
> field itself) to 16 bytes / 128 bits, so rli->offset_align remains 128.
> The array has element size 2 bytes / 16 bits, times function argument,
> so the end of the field is worst case aligned just to 16 bits; if we keep
> rli->offset_align as 128 for the next field, then DECL_OFFSET_ALIGN is too
> large. DECL_FIELD_OFFSET documented as:
> /* In a FIELD_DECL, this is the field position, counting in bytes, of the
> DECL_OFFSET_ALIGN-bit-sized word containing the bit closest to the beginning
> of the structure. */
> and when gimplifying COMPONENT_REFs with that field we:
> tree offset = unshare_expr (component_ref_field_offset (t));
> tree field = TREE_OPERAND (t, 1);
> tree factor
> = size_int (DECL_OFFSET_ALIGN (field) / BITS_PER_UNIT);
>
> /* Divide the offset by its alignment. */
> offset = size_binop_loc (loc, EXACT_DIV_EXPR, offset, factor);
> and later on multiply it again by DECL_OFFSET_ALIGN. The EXACT_DIV_EXPR
> isn't exact.
>
> Fixed by lowering the rli->offset_align if the size isn't a multiple of
> the align. We don't have a multiple_of_p variant that would compute
> highest power of two number the expression is known to be a multiple of,
> so I'm just checking the most common case, where the size is a multiple
> of the starting alignment, and otherwise just compute it very
> conservatively. This will be lower than necessary say for
> __attribute__((aligned (16))) short field[2 * size];
> - just 16 bits instead of 32. In theory we could do a binary search
> on power of two numbers in between that high initial rli->offset_align
> for which the first multiple_of_p failed, and the conservative guess
> we do to improve it. If you think it is worth it, I can code it up.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-02-09 Jakub Jelinek <jakub@redhat.com>
>
> PR c/82210
> * stor-layout.c (place_field): For variable length fields, adjust
> offset_align afterwards not just based on the field's alignment,
> but also on the size.
>
> * gcc.c-torture/execute/pr82210.c: New test.
OK.
jeff
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-02-13 18:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-09 6:22 [PATCH] Fix handling of variable length fields in structures (PR c/82210) Jakub Jelinek
2018-02-13 18:40 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).