Re: [PATCH] Fix handling of variable length fields in structures (PR c/82210)

[Jeff Law <law@redhat.com>]

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