| Message ID | alpine.DEB.2.21.2406112240070.9248@angie.orcam.me.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [PR115565] cse: Don't use a valid regno for non-register in comparison_qty | expand |
"Maciej W. Rozycki" <macro@orcam.me.uk> writes: > Use INT_MIN rather than -1 in `comparison_qty' where a comparison is not > with a register, because the value of -1 is actually a valid reference > to register 0 in the case where it has not been assigned a quantity. > > Using -1 makes `REG_QTY (REGNO (folded_arg1)) == ent->comparison_qty' > comparison in `fold_rtx' to incorrectly trigger in rare circumstances > and return true for a memory reference making CSE consider a comparison > operation to evaluate to a constant expression and consequently make the > resulting code incorrectly execute or fail to execute conditional > blocks. > > This has caused a miscompilation of rwlock.c from LinuxThreads for the > `alpha-linux-gnu' target, where `rwlock->__rw_writer != thread_self ()' > expression (where `thread_self' returns the thread pointer via a PALcode > call) has been decided to be always true (with `ent->comparison_qty' > using -1 for a reference to to `rwlock->__rw_writer', while register 0 > holding the thread pointer retrieved by `thread_self') and code for the > false case has been optimized away where it mustn't have, causing > program lockups. > > The issue has been observed as a regression from commit 08a692679fb8 > ("Undefined cse.c behaviour causes 3.4 regression on HPUX"), > <https://gcc.gnu.org/ml/gcc-patches/2004-10/msg02027.html>, and up to > commit 932ad4d9b550 ("Make CSE path following use the CFG"), > <https://gcc.gnu.org/ml/gcc-patches/2006-12/msg00431.html>, where CSE > has been restructured sufficiently for the issue not to trigger with the > original reproducer anymore. However the original bug remains and can > trigger, because `comparison_qty' will still be assigned -1 for a memory > reference and the `reg_qty' member of a `cse_reg_info_table' entry will > still be assigned -1 for register 0 where the entry has not been > assigned a quantity, e.g. at initialization. > > Use INT_MIN then as noted above, so that the value remains negative, for > consistency with the REGNO_QTY_VALID_P macro (even though not used on > `comparison_qty'), and then so that it should not ever match a valid > negated register number, fixing the regression with commit 08a692679fb8. > > gcc/ > PR rtl-optimization/115565 > * cse.cc (record_jump_cond): Use INT_MIN rather than -1 for > `comparison_qty' if !REG_P. > --- > Hi, > > Oh boy, this was hard to chase and debug! See the PR referred for > details. Sadly I have no reproducer for GCC 15, this bug seems too > elusive to make one easily. > > This has passed verification in native `powerpc64le-linux-gnu' and > `x86_64-linux-gnu' regstraps, as well as with the `alpha-linux-gnu' > target. OK to apply and backport to the release branches? Huh! Nice detective work. The patch is OK for trunk, thanks. I agree that it's a regression from 08a692679fb8. Since it's fixing such a hard-to-diagnose wrong code bug, and since it seems very safe, I think it's worth backporting to all active branches, after a grace period. Thanks, Richard > > Maciej > --- > gcc/cse.cc | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > gcc-cse-comparison-qty.diff > Index: gcc/gcc/cse.cc > =================================================================== > --- gcc.orig/gcc/cse.cc > +++ gcc/gcc/cse.cc > @@ -239,7 +239,7 @@ static int next_qty; > the constant being compared against, or zero if the comparison > is not against a constant. `comparison_qty' holds the quantity > being compared against when the result is known. If the comparison > - is not with a register, `comparison_qty' is -1. */ > + is not with a register, `comparison_qty' is INT_MIN. */ > > struct qty_table_elem > { > @@ -4058,7 +4058,7 @@ record_jump_cond (enum rtx_code code, ma > else > { > ent->comparison_const = op1; > - ent->comparison_qty = -1; > + ent->comparison_qty = INT_MIN; > } > > return;
On Fri, 21 Jun 2024, Richard Sandiford wrote: > > This has passed verification in native `powerpc64le-linux-gnu' and > > `x86_64-linux-gnu' regstraps, as well as with the `alpha-linux-gnu' > > target. OK to apply and backport to the release branches? > > Huh! Nice detective work. Thank you. It did help that `call_pal 158' or the lack of is so easy to spot in disassembly or compiler output. Inspired by Roger Sayle's observation that the use of $0 hard register pre-reload is uncommon I tried to come up with an RTL test case that I *might* be able to tweak enough, knowing the nature of the bug, to still trigger with trunk. For that I backported `print_rtx_function' to 4.1.2. The output it produced was sufficiently different though from the syntax now accepted for input it wasn't suitable at all. So I thought I could perhaps tweak it by hand using output from GCC 15 as a reference. But that did not work either, GCC 15 cannot accept its own output it would seem, complaining about integer suffixes across a couple of places: rwlock-test.i:112:100: error: invalid suffix "B" on integer constant 112 | 32)) [1 MEM[(struct _pthread_descr_struct * *)rwlock_8(D) + 32B]+0 S8 A64])) "rwlock-test.i":84:5 discrim 1) | ^~~ and most importantly, the asm statement: rwlock-test.i:53:62: error: expected character `)', found `:' rwlock-test.i:53:65: note: following context is `37)' where input is: (cinsn 18 (set (reg/v:DI $0 [ __self ]) (asm_operands:DI ("call_pal %1") ("=r") 0 [ (const_int 158) ] [ (asm_input:SI ("i") rwlock-test.i:37) ] [] rwlock-test.i:37)) "rwlock-test.i":37:58) and the complaint about the `asm_input' expression, and it's not clear to me what is expected here. So I have given up. I guess the RTL parser could be improved, or maybe output produced from `print_rtx_function' isn't right, I don't know. > The patch is OK for trunk, thanks. I agree that it's a regression > from 08a692679fb8. Since it's fixing such a hard-to-diagnose wrong > code bug, and since it seems very safe, I think it's worth backporting > to all active branches, after a grace period. Agreed as to the grace period. I have since additionally run the patch through `riscv64-linux-gnu' verification and John David Anglin was kind enough to do so with `hppa-unknown-linux-gnu'. I have pushed the change to trunk now, thank you for your review. Maciej
On Sun, 30 Jun 2024, Maciej W. Rozycki wrote: > > The patch is OK for trunk, thanks. I agree that it's a regression > > from 08a692679fb8. Since it's fixing such a hard-to-diagnose wrong > > code bug, and since it seems very safe, I think it's worth backporting > > to all active branches, after a grace period. > > Agreed as to the grace period. I have since additionally run the patch > through `riscv64-linux-gnu' verification and John David Anglin was kind > enough to do so with `hppa-unknown-linux-gnu'. Is it OK to backport now or shall we skip GCC 11 altogether since it's going to be closed after the upcoming 11.5 release later this week? Maciej
On Mon, 15 Jul 2024, Maciej W. Rozycki wrote: > On Sun, 30 Jun 2024, Maciej W. Rozycki wrote: > > > > The patch is OK for trunk, thanks. I agree that it's a regression > > > from 08a692679fb8. Since it's fixing such a hard-to-diagnose wrong > > > code bug, and since it seems very safe, I think it's worth backporting > > > to all active branches, after a grace period. > > > > Agreed as to the grace period. I have since additionally run the patch > > through `riscv64-linux-gnu' verification and John David Anglin was kind > > enough to do so with `hppa-unknown-linux-gnu'. > > Is it OK to backport now or shall we skip GCC 11 altogether since it's > going to be closed after the upcoming 11.5 release later this week? Yes, please leave 11.5 alone, it's OK for the rest of branches. Richard.
On Mon, 15 Jul 2024, Richard Biener wrote: > > > > The patch is OK for trunk, thanks. I agree that it's a regression > > > > from 08a692679fb8. Since it's fixing such a hard-to-diagnose wrong > > > > code bug, and since it seems very safe, I think it's worth backporting > > > > to all active branches, after a grace period. > > > > > > Agreed as to the grace period. I have since additionally run the patch > > > through `riscv64-linux-gnu' verification and John David Anglin was kind > > > enough to do so with `hppa-unknown-linux-gnu'. > > > > Is it OK to backport now or shall we skip GCC 11 altogether since it's > > going to be closed after the upcoming 11.5 release later this week? > > Yes, please leave 11.5 alone, it's OK for the rest of branches. Ack. I'll wait some more yet, just in case something pops out. Nothing should be done in haste but gripping a flea. Thank you for your input. Maciej
On Mon, 15 Jul 2024, Maciej W. Rozycki wrote: > > > Is it OK to backport now or shall we skip GCC 11 altogether since it's > > > going to be closed after the upcoming 11.5 release later this week? > > > > Yes, please leave 11.5 alone, it's OK for the rest of branches. > > Ack. I'll wait some more yet, just in case something pops out. Nothing > should be done in haste but gripping a flea. Thank you for your input. And I have backported this change now through GCC 12. Thank you. Maciej
Index: gcc/gcc/cse.cc =================================================================== --- gcc.orig/gcc/cse.cc +++ gcc/gcc/cse.cc @@ -239,7 +239,7 @@ static int next_qty; the constant being compared against, or zero if the comparison is not against a constant. `comparison_qty' holds the quantity being compared against when the result is known. If the comparison - is not with a register, `comparison_qty' is -1. */ + is not with a register, `comparison_qty' is INT_MIN. */ struct qty_table_elem { @@ -4058,7 +4058,7 @@ record_jump_cond (enum rtx_code code, ma else { ent->comparison_const = op1; - ent->comparison_qty = -1; + ent->comparison_qty = INT_MIN; } return;
Use INT_MIN rather than -1 in `comparison_qty' where a comparison is not with a register, because the value of -1 is actually a valid reference to register 0 in the case where it has not been assigned a quantity. Using -1 makes `REG_QTY (REGNO (folded_arg1)) == ent->comparison_qty' comparison in `fold_rtx' to incorrectly trigger in rare circumstances and return true for a memory reference making CSE consider a comparison operation to evaluate to a constant expression and consequently make the resulting code incorrectly execute or fail to execute conditional blocks. This has caused a miscompilation of rwlock.c from LinuxThreads for the `alpha-linux-gnu' target, where `rwlock->__rw_writer != thread_self ()' expression (where `thread_self' returns the thread pointer via a PALcode call) has been decided to be always true (with `ent->comparison_qty' using -1 for a reference to to `rwlock->__rw_writer', while register 0 holding the thread pointer retrieved by `thread_self') and code for the false case has been optimized away where it mustn't have, causing program lockups. The issue has been observed as a regression from commit 08a692679fb8 ("Undefined cse.c behaviour causes 3.4 regression on HPUX"), <https://gcc.gnu.org/ml/gcc-patches/2004-10/msg02027.html>, and up to commit 932ad4d9b550 ("Make CSE path following use the CFG"), <https://gcc.gnu.org/ml/gcc-patches/2006-12/msg00431.html>, where CSE has been restructured sufficiently for the issue not to trigger with the original reproducer anymore. However the original bug remains and can trigger, because `comparison_qty' will still be assigned -1 for a memory reference and the `reg_qty' member of a `cse_reg_info_table' entry will still be assigned -1 for register 0 where the entry has not been assigned a quantity, e.g. at initialization. Use INT_MIN then as noted above, so that the value remains negative, for consistency with the REGNO_QTY_VALID_P macro (even though not used on `comparison_qty'), and then so that it should not ever match a valid negated register number, fixing the regression with commit 08a692679fb8. gcc/ PR rtl-optimization/115565 * cse.cc (record_jump_cond): Use INT_MIN rather than -1 for `comparison_qty' if !REG_P. --- Hi, Oh boy, this was hard to chase and debug! See the PR referred for details. Sadly I have no reproducer for GCC 15, this bug seems too elusive to make one easily. This has passed verification in native `powerpc64le-linux-gnu' and `x86_64-linux-gnu' regstraps, as well as with the `alpha-linux-gnu' target. OK to apply and backport to the release branches? Maciej --- gcc/cse.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) gcc-cse-comparison-qty.diff