Re: [PATCH] reg-stack: Fix a -fcompare-debug bug in reg-stack [PR107183]

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/19/22 02:15, Jakub Jelinek wrote:
> Hi!
>
> As the following testcase shows, the swap_rtx_condition function
> in reg-stack can result in different code generation between -g and -g0.
> The function is doing the changes as it goes, so does analysis and
> changes together, which makes it harder to deal with DEBUG_INSNs,
> where normally analysis phase ignores them and the later phase
> doesn't.
> swap_rtx_condition walks instructions two different ways, one is
> using next_flags_user function which stops on non-call instructions
> that mention the flags register, and the other is a loop on fnstsw
> where it stops on instructions mentioning it and tries to find
> sahf instruction that uses it (in both cases calls stop it and so
> does end of basic block).
> Now both of these currently stop on DEBUG_INSNs that mention
> the flags register resp. the fnstsw result register.
> On success the function recurses on next flags user instruction
> if still live and if the recursion failed, reverts the changes
> it did too and fails.
> If it were just for the next_flags_user case, the fix could be
> just not doing
>        INSN_CODE (insn) = -1;
>        if (recog_memoized (insn) == -1)
> 	fail = 1;
> on DEBUG_INSNs (assuming all changes to those are fine),
> swap_rtx_condition_1 just changes one comparison to a different
> one.  But due to the possibility of fnstsw result being used
> in theory before sahf in some DEBUG_INSNs, this patch takes
> a different approach.  swap_rtx_condition has now a new argument
> and two modes.  The first mode is when debug_seen is >= 0, in this
> case both next_flags_user and the loop for fnstsw -> sahf will
> ignore but note DEBUG_INSNs (that mention flags register or fnstsw
> result).  If no such DEBUG_INSN is found during the whole call
> including recursive invocations (so e.g. for -g0 but probably most
> often for -g as well), it behaves as before, if it returns true
> all the changes are done and nothing further needs to be done later.
> If any DEBUG_INSNs are seen along the way, even when returning success
> all the changes are reverted, so it just reports that the function
> would be successful if DEBUG_INSNs were ignored.
> In this case, compare_for_stack_reg needs to call it again in
> debug_seen = -1 mode, which tells the function to update everything
> including DEBUG_INSNs.  For the fnstsw -> sahf case which I hope
> will be very rare I just reset the DEBUG_INSNs, I don't really
> know how to express it easily otherwise.  For the rest
> swap_rtx_condition_1 is done even on the DEBUG_INSNs.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> And after some time for release branches too?
>
> 2022-11-19  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/107183
> 	* reg-stack.cc (next_flags_user): Add DEBUG_SEEN argument.
> 	If >= 0 and a DEBUG_INSN would be otherwise returned, set
> 	DEBUG_SEEN to 1 and ignore it.
> 	(swap_rtx_condition): Add DEBUG_SEEN argument.  In >= 0
> 	mode only set DEBUG_SEEN to 1 if problematic DEBUG_ISNSs
> 	were seen and revert all changes on success in that case.
> 	Don't try to recog_memoized DEBUG_INSNs.
> 	(compare_for_stack_reg): Adjust swap_rtx_condition caller.
> 	If it returns true and debug_seen is 1, call swap_rtx_condition
> 	again with debug_seen -1.
>
> 	* gcc.dg/ubsan/pr107183.c: New test.

OK

jeff