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
