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!) Richard. > > Bernd. > > > > > Richard. > > > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
