"Maciej W. Rozycki" <ma...@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;
