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);
Bernd.
