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.
jeff
