This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] convert braced initializers to strings (PR 71625)
On 08/18/18 18:46, Richard Sandiford wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> On 08/18/18 12:40, Richard Sandiford wrote:
>>> Bernd Edlinger <bernd.edlinger@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.edlinger@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.
>
Yes, I come to the same conclusion.
The offset==NULL assertion reflects the logic around the "wreak havoc" comment
in get_inner_reference, so that is guaranteed to hold, at least immediately
after get_inner_reference returns.
>> $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.
>
Yes, I see, thanks.
Additionally I think we need some assertions, when signed bit quantities
are passed to store_bit_field and expand_bit_field.
Thanks
Bernd.
2018-08-19 Bernd Edlinger <bernd.edlinger@hotmail.de>
* expr.c (expand_assignment): Assert that bitpos is positive.
(store_field): Likewise
(expand_expr_real_1): Make sure that bitpos is positive.
Index: gcc/expr.c
===================================================================
--- gcc/expr.c (Revision 263644)
+++ gcc/expr.c (Arbeitskopie)
@@ -5270,6 +5270,7 @@ expand_assignment (tree to, tree from, bool nontem
MEM_VOLATILE_P (to_rtx) = 1;
}
+ gcc_assert (known_ge (bitpos, 0));
if (optimize_bitfield_assignment_op (bitsize, bitpos,
bitregion_start, bitregion_end,
mode1, to_rtx, to, from,
@@ -7046,6 +7047,7 @@ store_field (rtx target, poly_int64 bitsize, poly_
}
/* Store the value in the bitfield. */
+ gcc_assert (known_ge (bitpos, 0));
store_bit_field (target, bitsize, bitpos,
bitregion_start, bitregion_end,
mode, temp, reverse);
@@ -10545,6 +10547,14 @@ expand_expr_real_1 (tree exp, rtx target, machine_
mode2
= CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
+ /* 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);
+ }
+
/* If we have either an offset, a BLKmode result, or a reference
outside the underlying object, we must force it to memory.
Such a case can occur in Ada if we have unchecked conversion
@@ -10795,6 +10805,7 @@ expand_expr_real_1 (tree exp, rtx target, machine_
&& GET_MODE_CLASS (ext_mode) == MODE_INT)
reversep = TYPE_REVERSE_STORAGE_ORDER (type);
+ gcc_assert (known_ge (bitpos, 0));
op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp,
(modifier == EXPAND_STACK_PARM
? NULL_RTX : target),