On 08/20/18 16:54, Jeff Law wrote:
> On 08/20/2018 08:52 AM, Bernd Edlinger wrote:
>> On 08/20/18 16:16, Jeff Law wrote:
>>> On 08/20/2018 07:28 AM, Richard Biener wrote:
>>>> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
>>>>
>>>>> On 08/20/18 12:41, Richard Biener wrote:
>>>>>> On Sun, 19 Aug 2018, Bernd Edlinger wrote:
>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>>
>>>>>>> This fixes a wrong code issue in expand_expr_real_1 which happens
>>>>>>> because
>>>>>>> a negative bitpos is actually able to reach extract_bit_field which
>>>>>>> does all computations with poly_uint64, thus the offset
>>>>>>> 0x1ffffffffffffff0.
>>>>>>>
>>>>>>> To avoid that I propose to use Jakub's r204444 patch from the
>>>>>>> expand_assignment
>>>>>>> also in the expand_expr_real_1.
>>>>>>>
>>>>>>> This is a rather unlikely thing to happen, as there are lots of checks
>>>>>>> that are
>>>>>>> of course all target dependent between the get_inner_reference and the
>>>>>>> actual extract_bit_field call, and all other code paths may or may not
>>>>>>> have a problem
>>>>>>> with negative bit offsets. Most don't have a problem, but the bitpos
>>>>>>> needs to be
>>>>>>> folded into offset before it is used, therefore it is necessary to
>>>>>>> handle the negative
>>>>>>> bitpos very far away from the extract_bit_field call. Doing that later
>>>>>>> is IMHO not
>>>>>>> possible.
>>>>>>>
>>>>>>> The fix in CONSTANT_ADDRESS_P is actually unrelated, and I only spotted
>>>>>>> it because
>>>>>>> this macro is used in alpha_legitimize_address which is of course what
>>>>>>> one looks
>>>>>>> at first if something like that happens.
>>>>>>>
>>>>>>> I think even with this bogus offset it should not have caused a linker
>>>>>>> error, so there
>>>>>>> is probably a second problem in the *movdi code pattern of the
>>>>>>> alpha.md, because it
>>>>>>> should be split into instructions that don't cause a link error.
>>>>>>>
>>>>>>> Once the target is fixed to split the impossible assembler instruction,
>>>>>>> the test case
>>>>>>> will probably no longer be able to detect the pattern in the assembly.
>>>>>>>
>>>>>>> Therefore the test case looks both at the assembler output and the
>>>>>>> expand rtl dump
>>>>>>> to spot the bogus offset. I only check the first 12 digits of the
>>>>>>> bogus constant,
>>>>>>> because it is actually dependent on the target configuration:
>>>>>>>
>>>>>>> I built first a cross-compiler without binutils, and it did used a
>>>>>>> slightly different
>>>>>>> offset of 0x2000000000000000, (configured with:
>>>>>>> --target=alpha-linux-gnu --enable-languages=c
>>>>>>> --disable-libgcc --disable-libssp --disable-libquadmath
>>>>>>> --disable-libgomp --disable-libatomic)
>>>>>>> when the binutils are installed at where --prefix points, the offset is
>>>>>>> 0x1ffffffffffffff0.
>>>>>>>
>>>>>>> Regarding the alpha target, I could not do more than build a cross
>>>>>>> compiler and run
>>>>>>> make check-gcc-c RUNTESTFLAGS="alpha.exp=pr86984.c".
>>>>>>>
>>>>>>>
>>>>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>>>>> Is it OK for trunk?
>>>>>>
>>>>>> Hmm, I don't remember why we are inconsistent in get_inner_reference
>>>>>> with respect to negative bitpos. I think we should be consistent
>>>>>> here and may not be only by accident? That is, does
>>>>>>
>>>>>> diff --git a/gcc/expr.c b/gcc/expr.c
>>>>>> index c071be67783..9dc78587136 100644
>>>>>> --- a/gcc/expr.c
>>>>>> +++ b/gcc/expr.c
>>>>>> @@ -7272,7 +7272,7 @@ get_inner_reference (tree exp, poly_int64_pod
>>>>>> *pbitsize,
>>>>>> TYPE_PRECISION (sizetype));
>>>>>> tem <<= LOG2_BITS_PER_UNIT;
>>>>>> tem += bit_offset;
>>>>>> - if (tem.to_shwi (pbitpos))
>>>>>> + if (tem.to_shwi (pbitpos) && !maybe_lt (*pbitpos, 0))
>>>>>> *poffset = offset = NULL_TREE;
>>>>>> }
>>>>>>
>>>>>> fix the issue?
>>>>>>
>>>>>
>>>>> Yes, at first sight, however, I was involved at PR 58970,
>>>>> see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58970
>>>>>
>>>>> and I proposed a similar patch, which was objected by Jakub:
>>>>>
>>>>> see comment #25 of PR 58970:
>>>>> "Jakub Jelinek 2013-11-05 19:41:12 UTC
>>>>>
>>>>> (In reply to Bernd Edlinger from comment #24)
>>>>>> Created attachment 31169 [details]
>>>>>> https://gcc.gnu.org/bugzilla/attachment.cgi?id=31169
>>>>>> Another (better) attempt at fixing this ICE.
>>>>>>
>>>>>> This avoids any negative bitpos from get_inner_reference.
>>>>>> These negative bitpos values are just _very_ dangerous all the
>>>>>> way down to expmed.c
>>>>>
>>>>> I disagree that it is better, you are forgetting get_inner_reference has
>>>>> other > 20 callers outside of expansion and there is no reason why
>>>>> negative bitpos would be a problem in those cases."
>>>>>
>>>>> So that is what Jakub said at that time, and with the
>>>>> suggested change in get_inner_reference,
>>>>> this part of the r204444 change would be effectively become superfluous:
>>>>>
>>>>> @@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem
>>>>> tem = get_inner_reference (to, &bitsize, &bitpos, &offset,
>>>>> &mode1,
>>>>> &unsignedp, &volatilep, true);
>>>>>
>>>>> + /* Make sure bitpos is not negative, it can wreak havoc later. */
>>>>> + if (bitpos < 0)
>>>>> + {
>>>>> + gcc_assert (offset == NULL_TREE);
>>>>> + offset = size_int (bitpos >> (BITS_PER_UNIT == 8
>>>>> + ? 3 : exact_log2
>>>>> (BITS_PER_UNIT)));
>>>>> + bitpos &= BITS_PER_UNIT - 1;
>>>>> + }
>>>>> +
>>>>> if (TREE_CODE (to) == COMPONENT_REF
>>>>> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
>>>>> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos,
>>>>> &offset);
>>>>>
>>>>> and should be reverted. I did not really like it then, but I'd respect
>>>>> Jakub's advice.
>>>>
>>>> Hmm. I agree that there are other callers and yes, replicating Jakubs
>>>> fix is certainly more safe. Still it's somewhat inconsistent in that
>>>> get_inner_reference already has code to mitigate negative bitpos, but
>>>> only in the case 'offset' wasn't just an INTEGER_CST ...
>>>>
>>>> So your patch is OK (please change the gcc_asserts to
>>>> gcc_checking_asserts though to avoid ICEing for release compilers).
>>>>
>>>> We still might want to revisit get_inner_reference (and make its
>>>> bitpos unsigned then!)
>>> Given this is keeping my tester from running on alpha, I'm going to make
>>> the adjustment to Bernd's patch and commit it momentarily.
>>>
>>
>> Hi Jeff,
>>
>> is there a reason why this gcc_assert should not be a gcc_checking_assert?
>>
>> @@ -7046,6 +7047,7 @@
>> }
>>
>> /* 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);
> Nope. I must have missed it. Go ahead and commit it as obvious.
>
> jeff
>
Okay, committed as r263665:
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog (revision 263664)
+++ gcc/ChangeLog (revision 263665)
@@ -1,5 +1,9 @@
2018-08-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
+ * expr.c (store_field): Change gcc_assert to gcc_checking_assert.
+
+2018-08-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
+
PR target/86984
* expr.c (expand_assignment): Assert that bitpos is positive.
(store_field): Likewise
Index: gcc/expr.c
===================================================================
--- gcc/expr.c (revision 263664)
+++ gcc/expr.c (revision 263665)
@@ -7047,7 +7047,7 @@
}
/* Store the value in the bitfield. */
- gcc_assert (known_ge (bitpos, 0));
+ gcc_checking_assert (known_ge (bitpos, 0));
store_bit_field (target, bitsize, bitpos,
bitregion_start, bitregion_end,
mode, temp, reverse);
Bernd.
