| Message ID | DUB122-W209385567C3551814F6331E4020@phx.gbl |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 22, 2013 at 3:50 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi, > > On Tue, 17 Sep 2013 01:09:45, Martin Jambor wrote: >>> @@ -4773,6 +4738,8 @@ expand_assignment (tree to, tree from, b >>> if (MEM_P (to_rtx) >>> && GET_MODE (to_rtx) == BLKmode >>> && GET_MODE (XEXP (to_rtx, 0)) != VOIDmode >>> + && bitregion_start == 0 >>> + && bitregion_end == 0 >>> && bitsize> 0 >>> && (bitpos % bitsize) == 0 >>> && (bitsize % GET_MODE_ALIGNMENT (mode1)) == 0 >>> >> ... >> >> I'm not sure to what extent the hunk adding tests for bitregion_start >> and bitregion_end being zero is connected to this issue. I do not see >> any of the testcases exercising that path. If it is indeed another >> problem, I think it should be submitted (and potentially committed) as >> a separate patch, preferably with a testcase. >> > > Meanwhile I am able to give an example where that code is executed > with bitpos = 64, bitsize=32, bitregion_start = 32, bitregion_end = 95. > > Afterwards bitpos=0, bitsize=32, which is completely outside > bitregion_start=32, bitregion_end=95. > > However this can only be seen in the debugger, as the store_field > goes thru a code path that does not look at bitregion_start/end. > > Well that is at least extremely ugly, and I would not be sure, that > I cannot come up with a sample that crashes or creates wrong code. > > Currently I think that maybe the best way to fix that would be this: > > --- gcc/expr.c 2013-10-21 08:27:09.546035668 +0200 > +++ gcc/expr.c 2013-10-22 15:19:56.749476525 +0200 > @@ -4762,6 +4762,9 @@ expand_assignment (tree to, tree from, b > && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) > { > to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT); > + bitregion_start = 0; > + if (bitregion_end>= (unsigned HOST_WIDE_INT) bitpos) > + bitregion_end -= bitpos; > bitpos = 0; > } > > Any suggestions? if bitregion_start/end are used after the adjust_address call then they have to be adjusted (or bitpos left in place). In fact why we apply byte-parts of bitpos here only if offset != 0 is weird. OTOH this code is _very_ old... what happens if you remove the whole case? Richard. > > > Regards > Bernd.
On, Wed, 23 Oct 2013 17:36:41Richard Biener wrote: > > if bitregion_start/end are used after the adjust_address call then they have > to be adjusted (or bitpos left in place). In fact why we apply byte-parts of > bitpos here only if offset != 0 is weird. OTOH this code is _very_ old... > what happens if you remove the whole case? > > Richard. > If I remove that code completely... Than changes nothing ARM at -O2. and on x86 at -O2 this changes: .cfi_startproc movl k, %eax leal (%eax,%eax,2), %eax - leal ss+8(,%eax,4), %eax - movl $1, 16(%eax) + leal ss+16(,%eax,4), %eax + movl $1, 8(%eax) movl k, %eax leal (%eax,%eax,2), %eax - leal ss+8(,%eax,4), %eax - movl 16(%eax), %eax + leal ss+16(,%eax,4), %eax + movl 8(%eax), %eax cmpl $1, %eax jne .L5 xorl %eax, %eax that does not really make any difference. So removing that code looks like a nice alternative. What do you think? Bernd.
> if bitregion_start/end are used after the adjust_address call then they have > to be adjusted (or bitpos left in place). In fact why we apply byte-parts > of bitpos here only if offset != 0 is weird. Presumably to be able to do arithmetic on symbols when the offset is variable, which can save one addition in the final code.
On Thu, Oct 24, 2013 at 10:37 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> if bitregion_start/end are used after the adjust_address call then they have >> to be adjusted (or bitpos left in place). In fact why we apply byte-parts >> of bitpos here only if offset != 0 is weird. > > Presumably to be able to do arithmetic on symbols when the offset is variable, > which can save one addition in the final code. Doesn't that also apply to arithmetic on symbols when the offset is NULL? But yes, the codegen example posted shows this kind of difference (though it doesn't seem to save anything for that case). I'd have expected a more explicit guarding of this case, like with MEM_P (to_rtx) && SYMBOL_REF_P (XEXP (to_rtx, 0)) though, eventually even looking whether the resulting address is legitimate. But ... doesn't forwprop fix this up later anyway? Either way, bitregion_start/end is clearly wrong after we hit this case. I'm always fast approving removal of "odd" code (less-code-is-better doctrine...), but in this case maybe the code can be improved instead. The comment before it is also odd and likely only applies to later added restrictions. Just to quote it again: if (offset != 0) { ... /* A constant address in TO_RTX can have VOIDmode, we must not try to call force_reg for that case. Avoid that case. */ if (MEM_P (to_rtx) && GET_MODE (to_rtx) == BLKmode && GET_MODE (XEXP (to_rtx, 0)) != VOIDmode && bitsize > 0 && (bitpos % bitsize) == 0 && (bitsize % GET_MODE_ALIGNMENT (mode1)) == 0 && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) { to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT); bitpos = 0; } to_rtx = offset_address (to_rtx, offset_rtx, highest_pow2_factor_for_target (to, offset)); } Thanks, Richard. > -- > Eric Botcazou
Hi, On Thu, 24 Oct 2013 11:56:52, Richard Biener wrote: > > On Thu, Oct 24, 2013 at 10:37 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> if bitregion_start/end are used after the adjust_address call then they have >>> to be adjusted (or bitpos left in place). In fact why we apply byte-parts >>> of bitpos here only if offset != 0 is weird. >> >> Presumably to be able to do arithmetic on symbols when the offset is variable, >> which can save one addition in the final code. > > Doesn't that also apply to arithmetic on symbols when the offset is NULL? > But yes, the codegen example posted shows this kind of difference > (though it doesn't seem to save anything for that case). I'd have expected > a more explicit guarding of this case, like with MEM_P (to_rtx) > && SYMBOL_REF_P (XEXP (to_rtx, 0)) though, eventually even looking > whether the resulting address is legitimate. But ... doesn't forwprop > fix this up later anyway? > > Either way, bitregion_start/end is clearly wrong after we hit this case. > I'm always fast approving removal of "odd" code (less-code-is-better > doctrine...), but in this case maybe the code can be improved instead. > The comment before it is also odd and likely only applies to later > added restrictions. Just to quote it again: > > if (offset != 0) > { > ... > /* A constant address in TO_RTX can have VOIDmode, we must not try > to call force_reg for that case. Avoid that case. */ > if (MEM_P (to_rtx) > && GET_MODE (to_rtx) == BLKmode > && GET_MODE (XEXP (to_rtx, 0)) != VOIDmode > && bitsize> 0 > && (bitpos % bitsize) == 0 > && (bitsize % GET_MODE_ALIGNMENT (mode1)) == 0 > && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) > { > to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT); > bitpos = 0; > } > > to_rtx = offset_address (to_rtx, offset_rtx, > highest_pow2_factor_for_target (to, > offset)); > } > In the initial commit from 1998 where this was introduced that block contained an explicit force_reg call, but not that comment. Bernd. > > Thanks, > Richard. > >> -- >> Eric Botcazou
> Doesn't that also apply to arithmetic on symbols when the offset is NULL? This can presumably be retrofitted when the offset is null or constant. > But yes, the codegen example posted shows this kind of difference > (though it doesn't seem to save anything for that case). I'd have expected > a more explicit guarding of this case, like with MEM_P (to_rtx) > && SYMBOL_REF_P (XEXP (to_rtx, 0)) though, eventually even looking > whether the resulting address is legitimate. But ... doesn't forwprop > fix this up later anyway? Not clear IMO, it would need to reassociate. > The comment before it is also odd and likely only applies to later > added restrictions. Just to quote it again: > > if (offset != 0) > { > ... > /* A constant address in TO_RTX can have VOIDmode, we must not try > to call force_reg for that case. Avoid that case. */ if (MEM_P (to_rtx) > && GET_MODE (to_rtx) == BLKmode > && GET_MODE (XEXP (to_rtx, 0)) != VOIDmode > && bitsize > 0 > && (bitpos % bitsize) == 0 > && (bitsize % GET_MODE_ALIGNMENT (mode1)) == 0 > && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) > { > to_rtx = adjust_address (to_rtx, mode1, bitpos / > BITS_PER_UNIT); bitpos = 0; > } > > to_rtx = offset_address (to_rtx, offset_rtx, > highest_pow2_factor_for_target (to, > offset)); > } Yes, the comment is out-of-sync and not very informative.
> Hi, > > On Tue, 17 Sep 2013 01:09:45, Martin Jambor wrote: >>> @@ -4773,6 +4738,8 @@ expand_assignment (tree to, tree from, b >>> if (MEM_P (to_rtx) >>> && GET_MODE (to_rtx) == BLKmode >>> && GET_MODE (XEXP (to_rtx, 0)) != VOIDmode >>> + && bitregion_start == 0 >>> + && bitregion_end == 0 >>> && bitsize> 0 >>> && (bitpos % bitsize) == 0 >>> && (bitsize % GET_MODE_ALIGNMENT (mode1)) == 0 >>> >> ... >> >> I'm not sure to what extent the hunk adding tests for bitregion_start >> and bitregion_end being zero is connected to this issue. I do not see >> any of the testcases exercising that path. If it is indeed another >> problem, I think it should be submitted (and potentially committed) as >> a separate patch, preferably with a testcase. >> > > Meanwhile I am able to give an example where that code is executed > with bitpos = 64, bitsize=32, bitregion_start = 32, bitregion_end = 95. > > Afterwards bitpos=0, bitsize=32, which is completely outside > bitregion_start=32, bitregion_end=95. > > However this can only be seen in the debugger, as the store_field > goes thru a code path that does not look at bitregion_start/end. > > Well that is at least extremely ugly, and I would not be sure, that > I cannot come up with a sample that crashes or creates wrong code. > > Currently I think that maybe the best way to fix that would be this: > > --- gcc/expr.c 2013-10-21 08:27:09.546035668 +0200 > +++ gcc/expr.c 2013-10-22 15:19:56.749476525 +0200 > @@ -4762,6 +4762,9 @@ expand_assignment (tree to, tree from, b > && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) > { > to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT); > + bitregion_start = 0; > + if (bitregion_end>= (unsigned HOST_WIDE_INT) bitpos) > + bitregion_end -= bitpos; > bitpos = 0; > } > > Any suggestions? > > > > Regards > Bernd. well, as already discussed, the following example executes the above memory block, and leaves bitregion_start/end in an undefined state: extern void abort (void); struct x{ int a; int :32; volatile int b:32; }; struct s { int a,b,c,d; struct x xx[1]; }; struct s ss; volatile int k; int main() { ss.xx[k].b = 1; // asm volatile("":::"memory"); if ( ss.xx[k].b != 1) abort (); return 0; } Although this does not cause malfunction at the time, I'd propose to play safe, and update the bitregion_start/bitregion_end. Additionally I'd propose to remove this comment in expand_assignment and expand_expr_real_1: "A constant address in TO_RTX can have VOIDmode, we must not try to call force_reg for that case. Avoid that case." This comment is completely out of sync: There is no longer any force_reg in that if-block, and a constant address in TO_RTX has SImode or DImode in GET_MODE (XEXP (to_rtx, 0)) I do not know how to make it a VOIDmode, therefore the comment does not help either. Boot-strapped and regression-tested on x86_64-linux-gnu. OK for trunk? Regards Bernd. 2013-11-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * expr.c (expand_assignment): Remove bogus comment. Update bitregion_start/bitregion_end. (expand_expr_real_1): Remove bogus comment.
Hi, I almost forgot about this (rather simple) one. It falls in the category "Mostly Harmless", but it would be nice to have the bitregion_start/end consistent in all cases, even when it does currently not cause any mal-function. this was: > Subject: RE: [PATCH, PR 57748] Check for out of bounds access, Part 3 > Date: Wed, 6 Nov 2013 15:40:23 +0100 > >> Meanwhile I am able to give an example where that code is executed >> with bitpos = 64, bitsize=32, bitregion_start = 32, bitregion_end = 95. >> >> Afterwards bitpos=0, bitsize=32, which is completely outside >> bitregion_start=32, bitregion_end=95. >> >> However this can only be seen in the debugger, as the store_field >> goes thru a code path that does not look at bitregion_start/end. >> >> Well that is at least extremely ugly, and I would not be sure, that >> I cannot come up with a sample that crashes or creates wrong code. >> >> Currently I think that maybe the best way to fix that would be this: >> >> --- gcc/expr.c 2013-10-21 08:27:09.546035668 +0200 >> +++ gcc/expr.c 2013-10-22 15:19:56.749476525 +0200 >> @@ -4762,6 +4762,9 @@ expand_assignment (tree to, tree from, b >> && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) >> { >> to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT); >> + bitregion_start = 0; >> + if (bitregion_end>= (unsigned HOST_WIDE_INT) bitpos) >> + bitregion_end -= bitpos; >> bitpos = 0; >> } >> > > well, as already discussed, the following example executes the above memory block, > and leaves bitregion_start/end in an undefined state: > extern void abort (void); struct x{ int a; int :32; volatile int b:32; }; struct s { int a,b,c,d; struct x xx[1]; }; struct s ss; volatile int k; int main() { ss.xx[k].b = 1; // asm volatile("":::"memory"); if ( ss.xx[k].b != 1) abort (); return 0; } > Although this does not cause malfunction at the time, I'd propose to play safe, > and update the bitregion_start/bitregion_end. Additionally I'd propose to remove > this comment in expand_assignment and expand_expr_real_1: > > "A constant address in TO_RTX can have VOIDmode, we must not try > to call force_reg for that case. Avoid that case." > > This comment is completely out of sync: There is no longer any force_reg in that if-block, > and a constant address in TO_RTX has SImode or DImode in GET_MODE (XEXP (to_rtx, 0)) > I do not know how to make it a VOIDmode, therefore the comment does not help either. > > Boot-strapped and regression-tested on x86_64-linux-gnu. OK for trunk? Regards Bernd. 2013-11-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * expr.c (expand_assignment): Remove bogus comment. Update bitregion_start/bitregion_end. (expand_expr_real_1): Remove bogus comment.
On 12/03/13 06:27, Bernd Edlinger wrote: >> This comment is completely out of sync: There is no longer any force_reg in that if-block, >> and a constant address in TO_RTX has SImode or DImode in GET_MODE (XEXP (to_rtx, 0)) >> I do not know how to make it a VOIDmode, therefore the comment does not help either. I thought that comment looked familiar... That's because it was mine... The way that was triggered in the past can be found in this ancient posting: http://gcc.gnu.org/ml/gcc-patches/1999-03n/msg00654.html The code went through a major revamp in 2002 which removed the force_reg calls without updating/removing the comment. Additionally, the way we represent and thus ultimately expand the memory reference has changed in ways that make it harder, if not impossible to have a constant address at these points. Even if were to hack up the expanders to force a constant address rather than a REG it's still nontrivial to get into that code with a constant address. I wouldn't be terribly surprised to find all kinds of dead code in the expanders. So the comment clearly needs to get zapped. As for the consistency of bitregion_start/bitregion_end, I'm just not familiar enough with this code or the issues for the C++ memory model to comment. jeff
On Tue, 3 Dec 2013 22:55:32, Jef Law wrote: > > On 12/03/13 06:27, Bernd Edlinger wrote: >>> This comment is completely out of sync: There is no longer any force_reg in that if-block, >>> and a constant address in TO_RTX has SImode or DImode in GET_MODE (XEXP (to_rtx, 0)) >>> I do not know how to make it a VOIDmode, therefore the comment does not help either. > I thought that comment looked familiar... That's because it was mine... > > The way that was triggered in the past can be found in this ancient posting: > > http://gcc.gnu.org/ml/gcc-patches/1999-03n/msg00654.html > > The code went through a major revamp in 2002 which removed the force_reg > calls without updating/removing the comment. > > Additionally, the way we represent and thus ultimately expand the memory > reference has changed in ways that make it harder, if not impossible to > have a constant address at these points. Even if were to hack up the > expanders to force a constant address rather than a REG it's still > nontrivial to get into that code with a constant address. > > I wouldn't be terribly surprised to find all kinds of dead code in the > expanders. > > So the comment clearly needs to get zapped. > > As for the consistency of bitregion_start/bitregion_end, I'm just not > familiar enough with this code or the issues for the C++ memory model to > comment. > > jeff Thanks for this info. Regarding the consistency of bitregion_start/end , they should either both be zero, or bitregion_start <= bitpos && bitpos+bitsize-1 <= bitregion_end of course bitregion_start/end should be on byte-boundarys. So if I set bitpos=0, I must move bitregion_start/end accordingly. Of course bitregion_start can not become negative. Therefore I set it to zero, store_bit_field would never try to write at negative offsets anyway. This stuff with the bit region was introduced recently, and this if-block has been overlooked then. Thanks Bernd.
On 12/04/13 00:49, Bernd Edlinger wrote: > > Regarding the consistency of bitregion_start/end , > > they should either both be zero, or > > bitregion_start <= bitpos && bitpos+bitsize-1 <= bitregion_end Presumably to satisfy the consecutive bitfields are a single memory location stuff from C++11. Thus the bitregion representation (C++11) must totally encompass the original COMPONENT_REF. I can certainly see how that holds immediately after the call to get_bit_range. When we adjust the address, we're just accounting for the bit position (bitpos). ie, if bitpos was 16 and TO_RTX was (plus (reg) (32)). We change TO_RTX to (plus (reg) (48)). This is ultimately meant to simplify the resulting insns we generate by folding the bitpos adjustment into the address computation. In effect, this results in accessing a subobject rather than the full object. Presumably this is kosher for C++11's memory model (I really don't know). *If* it is OK to access the subobject like this in C++11's memory model, then is is necessary for bitregion_{start,end} to encompass the original object, or should it be made consistent with the bitpos/bitsize? And that seems to me to be the fundamental question. What is really the purpose of the bitregion_{start,end} representation? If it must represent the C++ object, then ISTM we don't want to mess with it. If it doesn't, then why did we bother building this alternate representation to begin with? Jeff
Hi, On Thu, 5 Dec 2013 21:12:54, Jeff Law wrote: > > On 12/04/13 00:49, Bernd Edlinger wrote: >> >> Regarding the consistency of bitregion_start/end , >> >> they should either both be zero, or >> >> bitregion_start <= bitpos && bitpos+bitsize-1 <= bitregion_end > Presumably to satisfy the consecutive bitfields are a single memory > location stuff from C++11. Thus the bitregion representation (C++11) > must totally encompass the original COMPONENT_REF. I can certainly see > how that holds immediately after the call to get_bit_range. > > When we adjust the address, we're just accounting for the bit position > (bitpos). ie, if bitpos was 16 and TO_RTX was (plus (reg) (32)). We > change TO_RTX to (plus (reg) (48)). This is ultimately meant to > simplify the resulting insns we generate by folding the bitpos > adjustment into the address computation. In effect, this results in > accessing a subobject rather than the full object. Presumably this is > kosher for C++11's memory model (I really don't know). > > *If* it is OK to access the subobject like this in C++11's memory model, > then is is necessary for bitregion_{start,end} to encompass the original > object, or should it be made consistent with the bitpos/bitsize? And > that seems to me to be the fundamental question. What is really the > purpose of the bitregion_{start,end} representation? If it must > represent the C++ object, then ISTM we don't want to mess with it. If > it doesn't, then why did we bother building this alternate > representation to begin with? > Yes, that is correct. let's assume bitpos = 16, and bitsize = 16, then we have bitregion_start = 16 and bitregion_end = 31. The to_rtx points to the start of the object. to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT) makes to_rtx point to two bytes from the beginning. that is compensated by bitpos = 0. So now [to_rtx, bitpos, bitsize] means the same thing than before. What's wrong is that bitregion_start is still 16 and bitregion_end is still 31 in that example. The correct values would be bitregion_start = 0 and bitregion_end = 15. that's what this new statement is meant to do: bitregion_start = 0; if (bitregion_end>= (unsigned HOST_WIDE_INT) bitpos) bitregion_end -= bitpos; I need the "if" because bitregion_end could be 0, and in that case I want bitregion_end to stay 0. Thanks Bernd. > > Jeff
On Fri, Dec 6, 2013 at 9:02 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi, > > On Thu, 5 Dec 2013 21:12:54, Jeff Law wrote: >> >> On 12/04/13 00:49, Bernd Edlinger wrote: >>> >>> Regarding the consistency of bitregion_start/end , >>> >>> they should either both be zero, or >>> >>> bitregion_start <= bitpos && bitpos+bitsize-1 <= bitregion_end >> Presumably to satisfy the consecutive bitfields are a single memory >> location stuff from C++11. Thus the bitregion representation (C++11) >> must totally encompass the original COMPONENT_REF. I can certainly see >> how that holds immediately after the call to get_bit_range. >> >> When we adjust the address, we're just accounting for the bit position >> (bitpos). ie, if bitpos was 16 and TO_RTX was (plus (reg) (32)). We >> change TO_RTX to (plus (reg) (48)). This is ultimately meant to >> simplify the resulting insns we generate by folding the bitpos >> adjustment into the address computation. In effect, this results in >> accessing a subobject rather than the full object. Presumably this is >> kosher for C++11's memory model (I really don't know). >> >> *If* it is OK to access the subobject like this in C++11's memory model, >> then is is necessary for bitregion_{start,end} to encompass the original >> object, or should it be made consistent with the bitpos/bitsize? And >> that seems to me to be the fundamental question. What is really the >> purpose of the bitregion_{start,end} representation? If it must >> represent the C++ object, then ISTM we don't want to mess with it. If >> it doesn't, then why did we bother building this alternate >> representation to begin with? >> > > Yes, that is correct. let's assume bitpos = 16, and bitsize = 16, > then we have bitregion_start = 16 and bitregion_end = 31. > The to_rtx points to the start of the object. > > to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT) > > makes to_rtx point to two bytes from the beginning. > that is compensated by > > bitpos = 0. > > So now [to_rtx, bitpos, bitsize] means the same thing than before. > What's wrong is that bitregion_start is still 16 and bitregion_end is still 31 > in that example. > The correct values would be bitregion_start = 0 and bitregion_end = 15. > that's what this new statement is meant to do: > > bitregion_start = 0; > if (bitregion_end>= (unsigned HOST_WIDE_INT) bitpos) > bitregion_end -= bitpos; > > I need the "if" because bitregion_end could be 0, and in that case > I want bitregion_end to stay 0. The patch is ok. Thanks, Richard. > > Thanks > Bernd. > >> >> Jeff
--- gcc/expr.c 2013-10-21 08:27:09.546035668 +0200 +++ gcc/expr.c 2013-10-22 15:19:56.749476525 +0200 @@ -4762,6 +4762,9 @@ expand_assignment (tree to, tree from, b && MEM_ALIGN (to_rtx) == GET_MODE_ALIGNMENT (mode1)) { to_rtx = adjust_address (to_rtx, mode1, bitpos / BITS_PER_UNIT); + bitregion_start = 0; + if (bitregion_end>= (unsigned HOST_WIDE_INT) bitpos) + bitregion_end -= bitpos; bitpos = 0; }