This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
- From: Richard Biener <rguenther at suse dot de>
- To: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Richard Henderson <rth at twiddle dot net>, Jeff Law <law at redhat dot com>, Jakub Jelinek <jakub at redhat dot com>
- Date: Mon, 20 Aug 2018 12:41:54 +0200 (CEST)
- Subject: Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
- References: <AM5PR0701MB2657456335DF1CF6267D1BAFE4330@AM5PR0701MB2657.eurprd07.prod.outlook.com>
On Sun, 19 Aug 2018, Bernd Edlinger wrote:
> 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
> 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
@@ -7272,7 +7272,7 @@ get_inner_reference (tree exp, poly_int64_pod
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?