Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
On 08/20/18 16:54, Jeff Law wrote: > 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 >>> 0x1ff0. >>> >>> To avoid that I propose to use Jakub's r20 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 0x2000, (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 >>> 0x1ff0. >>> >>> 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 r20 change would be effectively become
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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 >> 0x1ff0. >> >> To avoid that I propose to use Jakub's r20 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 0x2000, (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 >> 0x1ff0. >> >> 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 r20 change would be effectively become superfluous: @@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem tem = get_inner_reference (to, , , , ,
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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
> 0x1ff0.
>
> To avoid that I propose to use Jakub's r20 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 0x2000, (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
> 0x1ff0.
>
> 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 r20 change would be effectively become superfluous:
>>>
>>> @@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem
>>> tem = get_inner_reference (to, , , , ,
>>>, , true);
>>>
>>> + /* Make sure bitpos is not negative, it can wreak havoc later. */
>>> + if (bitpos < 0)
>>> + {
>>> +
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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 0x1ff0.
To avoid that I propose to use Jakub's r20 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 0x2000, (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
0x1ff0.
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 r20 change would be effectively become superfluous:
>>
>> @@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem
>> tem = get_inner_reference (to, , , , ,
>> , , 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
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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 0x1ff0.
> >>
> >> To avoid that I propose to use Jakub's r20 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 0x2000, (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
> >> 0x1ff0.
> >>
> >> 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 r20 change would be effectively become superfluous:
>
> @@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem
> tem = get_inner_reference (to, , , , ,
> , , 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;
> + }
> +
>
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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 0x1ff0.
>>
>> To avoid that I propose to use Jakub's r20 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 0x2000, (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
>> 0x1ff0.
>>
>> 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 r20 change would be effectively become superfluous:
@@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem
tem = get_inner_reference (to, , , , ,
, , 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 (_start, _end, to, , );
and should be reverted. I did not really like it then, but I'd respect Jakub's
advice.
Bernd.
> Richard.
>
Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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 0x1ff0. > > To avoid that I propose to use Jakub's r20 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 0x2000, (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 > 0x1ff0. > > 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? Richard.
[PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)
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 0x1ff0.
To avoid that I propose to use Jakub's r20 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 0x2000, (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
0x1ff0.
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?
Thanks
Bernd.
2018-08-19 Bernd Edlinger
PR target/86984
* expr.c (expand_assignment): Assert that bitpos is positive.
(store_field): Likewise
(expand_expr_real_1): Make sure that bitpos is positive.
* config/alpha/alpha.h (CONSTANT_ADDRESS_P): Avoid signed
integer overflow.
testsuite:
2018-08-19 Bernd Edlinger
PR target/86984
* gcc.target/alpha/pr86984.c: New test.
Index: gcc/config/alpha/alpha.h
===
--- gcc/config/alpha/alpha.h (revision 263644)
+++ gcc/config/alpha/alpha.h (working copy)
@@ -678,7 +678,7 @@ enum reg_class {
#define CONSTANT_ADDRESS_P(X) \
(CONST_INT_P (X) \
- && (unsigned HOST_WIDE_INT) (INTVAL (X) + 0x8000) < 0x1)
+ && (UINTVAL (X) + 0x8000) < 0x1)
/* The macros REG_OK_FOR..._P assume that the arg is a REG rtx
and check its validity for a certain class.
Index: gcc/expr.c
===
--- gcc/expr.c (revision 263644)
+++ gcc/expr.c (working copy)
@@ -5270,6 +5270,7 @@ expand_assignment (tree to, tree from, bool nontem
MEM_VOLATILE_P (to_rtx) = 1;
}
+ gcc_assert (known_ge (bitpos, 0));
if (optimize_bitfield_assignment_op (bitsize, bitpos,
bitregion_start, bitregion_end,
mode1, to_rtx, to, from,
@@ -7046,6 +7047,7 @@ store_field (rtx target, poly_int64 bitsize, poly_
}
/* 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);
@@ -10545,6 +10547,14 @@ expand_expr_real_1 (tree exp, rtx target, machine_
mode2
= CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
+ /* Make sure bitpos is not negative, it can wreak havoc later. */
+ if (maybe_lt (bitpos, 0))
+ {
+ gcc_assert (offset == NULL_TREE);
+ offset = size_int (bits_to_bytes_round_down (bitpos));
+ bitpos = num_trailing_bits (bitpos);
+ }
+
/* If we have either an offset, a BLKmode result, or a reference
outside the underlying object, we must force it to memory.
Such a case can occur in Ada if we have unchecked conversion
@@ -10795,6 +10805,7 @@ expand_expr_real_1 (tree exp, rtx target, machine_
&& GET_MODE_CLASS (ext_mode) == MODE_INT)
reversep = TYPE_REVERSE_STORAGE_ORDER (type);
+ gcc_assert (known_ge (bitpos, 0));
op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp,
(modifier == EXPAND_STACK_PARM
? NULL_RTX : target),
Index: gcc/testsuite/gcc.target/alpha/pr86984.c
===
---
