| Message ID | AM5PR0701MB2657456335DF1CF6267D1BAFE4330@AM5PR0701MB2657.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid negative bitpos in expand_expr_real_1 (PR 86984) | expand |
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? Richard.
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. Bernd. > Richard. >
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. > > >
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
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); Bernd.
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
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 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 > Okay, committed as r263665: Index: gcc/ChangeLog =================================================================== --- gcc/ChangeLog (revision 263664) +++ gcc/ChangeLog (revision 263665) @@ -1,5 +1,9 @@ 2018-08-20 Bernd Edlinger <bernd.edlinger@hotmail.de> + * expr.c (store_field): Change gcc_assert to gcc_checking_assert. + +2018-08-20 Bernd Edlinger <bernd.edlinger@hotmail.de> + PR target/86984 * expr.c (expand_assignment): Assert that bitpos is positive. (store_field): Likewise Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 263664) +++ gcc/expr.c (revision 263665) @@ -7047,7 +7047,7 @@ } /* Store the value in the bitfield. */ - gcc_assert (known_ge (bitpos, 0)); + gcc_checking_assert (known_ge (bitpos, 0)); store_bit_field (target, bitsize, bitpos, bitregion_start, bitregion_end, mode, temp, reverse); Bernd.
2018-08-19 Bernd Edlinger <bernd.edlinger@hotmail.de> 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 <bernd.edlinger@hotmail.de> 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) < 0x10000) + && (UINTVAL (X) + 0x8000) < 0x10000) /* 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 =================================================================== --- gcc/testsuite/gcc.target/alpha/pr86984.c (revision 0) +++ gcc/testsuite/gcc.target/alpha/pr86984.c (working copy) @@ -0,0 +1,96 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wall -Wwrite-strings -Werror -fmerge-all-constants -fno-stack-protector -mieee -fdump-rtl-expand" } */ + +struct expression { + unsigned long int num; +}; +union YYSTYPE { + unsigned long int num; + struct expression *exp; +}; + +typedef union YYSTYPE YYSTYPE; + +struct expression * new_exp_0 (int); + +union yyalloc { + short yyss_alloc; +}; + +static const signed char yypact[] = { + -9, -9, -10, -10, -9, 8, 36, -10, 13, -10, -9, -9, -9, -9, -9, -9, -9, -10, 26, 41, 45, 18, -2, 14, -10, -9, 36 }; +static const unsigned char yydefact[] = { + 0, 0, 12, 11, 0, 0, 2, 10, 0, 1, 0, 0, 0, 0, 0, 0, 0, 13, 0, 4, 5, 6, 7, 8, 9, 0, 3 }; + +static const signed char yypgoto[3] = "\366\366\377"; +static const signed char yydefgoto[3] = "\377\005\006"; + +static const unsigned char yytable[] = { + 7, 1, 2, 8, 3, 4, 15, 16, 9, 18, 19, 20, 21, 22, 23, 24, 10, 11, 12, 13, 14, 15, 16, 16, 26, 14, 15, 16, 17, 10, 11, 12, 13, 14, 15, 16, 0, 0, 25, 10, 11, 12, 13, 14, 15, 16, 12, 13, 14, 15, 16, 13, 14, 15, 16 }; + +static const signed char yycheck[] = { + 1, 10, 11, 4, 13, 14, 8, 9, 0, 10, 11, 12, 13, 14, 15, 16, 3, 4, 5, 6, 7, 8, 9, 9, 25, 7, 8, 9, 15, 3, 4, 5, 6, 7, 8, 9, -1, -1, 12, 3, 4, 5, 6, 7, 8, 9, 5, 6, 7, 8, 9, 6, 7, 8, 9 }; + +static const unsigned char yyr1[] = { + 0, 16, 17, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18 }; + +static const unsigned char yyr2[] = { + 0, 2, 1, 5, 3, 3, 3, 3, 3, 3, 2, 1, 1, 3 }; + +int __gettextparse (void) +{ + int yystate = 0; + short yyssa[200]; + short *yyss = yyss; + short *yyssp = yyssa; + YYSTYPE yyvsa[200]; + YYSTYPE *yyvsp = yyvsa; + enum { yystacksize = 200 }; + int yylen = 0; + goto yysetstate; + yynewstate: yyssp++; + yysetstate: *yyssp = yystate; + + if (yyss + yystacksize - 1 <= yyssp) + { + long unsigned int yysize = yyssp - yyss + 1; + { + short *yyss1 = yyss; + union yyalloc *yyptr = (union yyalloc *) __builtin_malloc ((yystacksize * (sizeof (short) + sizeof (YYSTYPE)) + (sizeof (union yyalloc) - 1))); + if (!yyptr) return 0; + __builtin_memcpy (&yyptr->yyss_alloc, yyss, yysize * sizeof *(yyss)); + yyss = &yyptr->yyss_alloc; + if (yyss1 != yyssa) __builtin_free (yyss1); + } + if (yyss + yystacksize - 1 <= yyssp) + return 0; + } + + int yyn = yypact[yystate]; + if (yyn == -10) + goto yydefault; + + yyn = yytable[yyn]; + if (yyn <= 0) + goto yyreduce; + + yydefault: yyn = yydefact[yystate]; + yyreduce: yylen = yyr2[yyn]; + + YYSTYPE yyval; + if (yyn == 12 && (yyval.exp = new_exp_0 (0)) != 0) + (yyval.exp)->num = (yyvsp[0].num); + + (yyvsp -= yylen, yyssp -= yylen); + yyn = yyr1[yyn]; + yystate = yypgoto[yyn - 16] + *yyssp; + if (0 <= yystate && yystate <= 54 && yycheck[yystate] == *yyssp) + yystate = yytable[yystate]; + else + yystate = yydefgoto[yyn - 16]; + + goto yynewstate; +} + +/* { dg-final { scan-rtl-dump-not "const_int 230584300921" "expand" } } */ +/* { dg-final { scan-assembler-not "yypgoto\\+230584300921" } } */