Bernd Edlinger <bernd.edlin...@hotmail.de> writes:
> On 08/18/18 12:40, Richard Sandiford wrote:
>> Bernd Edlinger <bernd.edlin...@hotmail.de> writes:
>>> Hi everybody,
>>>
>>> On 08/16/18 08:36, Bernd Edlinger wrote:
>>>> Jeff Law wrote:
>>>>> I wonder if the change to how we set up the initializers is ultimately
>>>>> changing the section those go into and ultimately causing an overflow of
>>>>> the .sdata section.
>>>>
>>>>
>>>> Yes, that is definitely the case.
>>>> Due to the -fmerge-all-constants option used
>>>> named arrays with brace initializer look like string initializers
>>>> and can go into the merge section if there are no embedded nul chars.
>>>> But the string constants can now be huge.
>>>>
>>>> See my other patch about string merging:
>>>> [PATCH] Handle not explicitly zero terminated strings in merge sections
>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00481.html
>>>>
>>>>
>>>> Can this section overflow?
>>>>
>>>
>>>
>>> could someone try out if this (untested) patch fixes the issue?
>>>
>>>
>>> Thanks,
>>> Bernd.
>>>
>>>
>>> 2018-08-18 Bernd Edlinger <bernd.edlin...@hotmail.de>
>>>
>>> * expmed.c (simple_mem_bitfield_p): Do shift right signed.
>>> * config/alpha/alpha.h (CONSTANT_ADDRESS_P): Avoid signed
>>> integer overflow.
>>>
>>> Index: gcc/config/alpha/alpha.h
>>> ===================================================================
>>> --- gcc/config/alpha/alpha.h (revision 263611)
>>> +++ gcc/config/alpha/alpha.h (working copy)
>>> @@ -678,7 +678,7 @@ enum reg_class {
>>>
>>> #define CONSTANT_ADDRESS_P(X) \
>>> (CONST_INT_P (X) \
>>> - && (unsigned HOST_WIDE_INT) (INTVAL (X) + 0x8000) < 0x10000)
>>> + && (UINTVAL (X) + 0x8000) < 0x10000)
>>>
>>> /* The macros REG_OK_FOR..._P assume that the arg is a REG rtx
>>> and check its validity for a certain class.
>>> Index: gcc/expmed.c
>>> ===================================================================
>>> --- gcc/expmed.c (revision 263611)
>>> +++ gcc/expmed.c (working copy)
>>> @@ -579,8 +579,12 @@ static bool
>>> simple_mem_bitfield_p (rtx op0, poly_uint64 bitsize, poly_uint64 bitnum,
>>> machine_mode mode, poly_uint64 *bytenum)
>>> {
>>> + poly_int64 ibit = bitnum;
>>> + poly_int64 ibyte;
>>> + if (!multiple_p (ibit, BITS_PER_UNIT, &ibyte))
>>> + return false;
>>> + *bytenum = ibyte;
>>> return (MEM_P (op0)
>>> - && multiple_p (bitnum, BITS_PER_UNIT, bytenum)
>>> && known_eq (bitsize, GET_MODE_BITSIZE (mode))
>>> && (!targetm.slow_unaligned_access (mode, MEM_ALIGN (op0))
>>> || (multiple_p (bitnum, GET_MODE_ALIGNMENT (mode))
>>
>> Do we have a genuinely negative bit offset here? Seems like the callers
>> would need to be updated if so, since the code is consistent in treating
>> the offset as unsigned.
>>
>
> Aehm, yes.
>
> The test case plural.i contains this:
>
> static const yytype_int8 yypgoto[] =
> {
> -10, -10, -1
> };
>
> static const yytype_uint8 yyr1[] =
> {
> 0, 16, 17, 18, 18, 18, 18, 18, 18, 18,
> 18, 18, 18, 18
> };
>
> yyn = yyr1[yyn];
>
> yystate = yypgoto[yyn - 16] + *yyssp;
>
>
> There will probably a reason why yyn can never be 0
> in yyn = yyr1[yyn]; but it is not really obvious.
>
> In plural.i.228t.optimized we have:
>
> pretmp_400 = yypgoto[-16];
> _385 = (int) pretmp_400;
> goto <bb 69>; [100.00%]
Ah, ok.
[...]
> (gdb) frame 26
> #26 0x000000000082f828 in expand_expr_real_1 (exp=<optimized out>,
> target=<optimized out>,
> tmode=<optimized out>, modifier=EXPAND_NORMAL, alt_rtl=0x0,
> inner_reference_p=<optimized out>)
> at ../../gcc-trunk/gcc/expr.c:10801
> 10801 ext_mode, ext_mode, reversep,
> alt_rtl);
> (gdb) list
> 10796 reversep = TYPE_REVERSE_STORAGE_ORDER (type);
> 10797
> 10798 op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp,
> 10799 (modifier == EXPAND_STACK_PARM
> 10800 ? NULL_RTX : target),
> 10801 ext_mode, ext_mode, reversep,
> alt_rtl);
> 10802
> 10803 /* If the result has a record type and the mode of OP0 is an
> 10804 integral mode then, if BITSIZE is narrower than this mode
> 10805 and this is for big-endian data, we must put the field
> (gdb) p bitpos
> $1 = {<poly_int_pod<1u, long>> = {coeffs = {-128}}, <No data fields>}
The get_inner_reference->store_field path in expand_assignment has:
/* Make sure bitpos is not negative, it can wreak havoc later. */
if (maybe_lt (bitpos, 0))
{
gcc_assert (offset == NULL_TREE);
offset = size_int (bits_to_bytes_round_down (bitpos));
bitpos = num_trailing_bits (bitpos);
}
So maybe this is what havoc looks like.
It's not my area, but I think we should be doing something similar for
the get_inner_reference->expand_bit_field path in expand_expr_real_1.
Haven't checked whether the offset == NULL_TREE assert would be
guaranteed there though.
> $5 = {<poly_int_pod<1u, unsigned long>> = {coeffs = {0x1ffffffffffffff0}},
> <No data fields>}
>
> The byte offset is completely wrong now, due to the bitnum was
> initially a negative integer and got converted to unsigned. At the
> moment when that is converted to byte offsets it is done wrong. I
> think it is too much to change everything to signed arithmetics, but
> at least when converting a bit pos to a byte pos, it should be done in
> signed arithmetics.
But then we'd mishandle a real 1ULL << 63 bitpos (say). Realise it's
unlikely in real code, but it'd still probably be possible to construct
a variant of the original test case in which the bias was
+0x1000000000000000 rather than -16.
Thanks,
Richard
