[Bug middle-end/113109] New: [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[Bug middle-end/113109] New: [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

            Bug ID: 113109
           Summary: [14 Regression] g++ EH tests fail at execution time
                    for cris-elf after r14-6674-g4759383245ac97
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hp at gcc dot gnu.org
                CC: guojiufu at gcc dot gnu.org
  Target Milestone: ---
              Host: x86_64-pc-linux-gnu
            Target: cris-elf

After r14-6674-g4759383245ac97, (at least) all tests that "throw", fail for
cris-elf at execution time: g++ tests as well as libstdc++ tests.  I don't see
any other clues from g++.log than execution failing for those tests.

Complete before/after example reports at
https://gcc.gnu.org/pipermail/gcc-testresults/2023-December/803815.html and
https://gcc.gnu.org/pipermail/gcc-testresults/2023-December/803816.html (for
r14-6672-g605d21f8ef1f and r14-6750-gf9be3d8faa47; same failures as 6674).

An example of a small hopefully-minimal test that fail is
gcc/testsuite/g++.old-deja/g++.mike/eh6.C.  Like for seemingly all others,
execution the test fails, and there's no output from the printf.  That
printf-statement is likely not reached, but the output *could* possibly still
be in an output-buffer (I don't remember how that works in newlib; that could
happen for glibc when execution is aborted).

I'm initially setting component to "middle-end" because that's what the commit
touched and also, I'm biased, but visiting the gcc-testresults archives I don't
see other targets fail in the same manner, so it could still be that "target"
fits better.  Further analysis will show; I'll dig a little deeper.  (Commit
author CC:ed.)

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |14.0

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I wonder if this is similar to what I saw years earlier, see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c11 . Jeff was worried about
this similar thing when he was reviewing the patch too.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #2 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #0)
> That
> printf-statement is likely not reached,

Now confirmed.  The assembly output for eh6.s is identical (before/after), but
apparently support-libraries (likely the unwind machinery) is miscompiled.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #3 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
It's __builtin_eh_return( that's miscompiled, such that the "handler" isn't
installed and the calling function will return to its caller instead of the
handler.

For the example below:

void f(__UINTPTR_TYPE__ p1, void *p2)
{
  __builtin_eh_return(p1, p2);
}

...there's a tell-tale diff between 6673 and 6674 in generated assembly code at
-O2:

@@ -23,7 +23,6 @@ _f:
        move.d $r13,[$sp]
 .LCFI5:
        move.d $r10,$r9
-       move.d $r11,[$sp+16]
        move.d [$sp+],$r13
        move.d [$sp+],$r12
        move.d [$sp+],$r11


cris.h defines EH_RETURN_HANDLER_RTX (as a call to cris_return_addr_rtx
yielding) gen_rtx_MEM (Pmode, plus_constant (Pmode, virtual_incoming_args_rtx,
-4)).

I'm "guessing" that the problem with the patch, is that anything any port
stores through a pointer based on virtual_incoming_args_rtx before returning,
is now eliminated.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Hmm, see PR 32398 and PR 32769. PR 32769 is interesting because it was caused
by the merge of the df branch where the store was being removed just like here
on cris. 

Oh and reading
https://inbox.sourceware.org/gcc-patches/200707151749.l6FHnXrt010084@hiauly1.hia.nrc.ca/
even mentions this exact issue it seems where dse.cc is removing the store and
such.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #5 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #4)
> Hmm, see PR 32398 and PR 32769. PR 32769 is interesting because it was
> caused by the merge of the df branch where the store was being removed just
> like here on cris. 
> 
> Oh and reading
> https://inbox.sourceware.org/gcc-patches/200707151749.l6FHnXrt010084@hiauly1.
> hia.nrc.ca/

(the patch submission for those two PR's)

> even mentions this exact issue it seems where dse.cc is removing
> the store and such.
Thanks for the bug-archive-digging!  I decided on trying making the mem
volatile and I see PR32769 supports that; exactly the same thing!

I just wonder why it was seen with pa *then* and CRIS only *now*.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So I did a quick audit of the EH_RETURN_HANDLER_RTX and most are registers
rather than a memory location  . And the ones which were memory locations used
either frame or stack pointer directly which seemed to not to be removed. I had
originally was going to record my findings but then I saw the volatile for pa
risc and deleted what I had wrote up.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[guojiufu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #7 from Jiu Fu Guo <guojiufu at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #3)
> 
> I'm "guessing" that the problem with the patch, is that anything any port
> stores through a pointer based on virtual_incoming_args_rtx before
> returning, is now eliminated.

Oh, yes, this is a possible place where that patch does not handle well.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[guojiufu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #8 from Jiu Fu Guo <guojiufu at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #6)
> So I did a quick audit of the EH_RETURN_HANDLER_RTX and most are registers
> rather than a memory location  . And the ones which were memory locations
> used either frame or stack pointer directly which seemed to not to be
> removed. I had originally was going to record my findings but then I saw the
> volatile for pa risc and deleted what I had wrote up.

I'm wondering if we need to revert r14-6674 to avoid this functionality issue.
And revisit/enhance the patch later.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #9 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:

https://gcc.gnu.org/g:3d03630b123411340e52d05124cb0cacfa1fc8b0

commit r14-6817-g3d03630b123411340e52d05124cb0cacfa1fc8b0
Author: Hans-Peter Nilsson <hp@axis.com>
Date:   Sun Dec 24 00:10:32 2023 +0100

    CRIS: Fix PR middle-end/113109; "throw" failing

    TL;DR: the "dse1" pass removed the eh-return-address store.  The
    PA also marks its EH_RETURN_HANDLER_RTX as volatile, for the same
    reason, as does visum.  See PR32769 - it's the same thing on PA.

    Conceptually, it's logical that stores to incoming args are
    optimized out on the return path or if no loads are seen -
    at least before epilogue expansion, when the subsequent load
    isn't seen in the RTL, as is the case for the "dse1" pass.

    I haven't looked into why this problem, that appeared for the PA
    already in 2007, was seen for CRIS only recently (with
    r14-6674-g4759383245ac97).

            PR middle-end/113109
            * config/cris/cris.cc (cris_eh_return_handler_rtx): New function.
            * config/cris/cris-protos.h (cris_eh_return_handler_rtx):
Prototype.
            * config/cris/cris.h (EH_RETURN_HANDLER_RTX): Redefine to call
            cris_eh_return_handler_rtx.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #10 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #6)
> So I did a quick audit of the EH_RETURN_HANDLER_RTX

Yeah, me too.

> and most are registers
> rather than a memory location  . And the ones which were memory locations
> used either frame or stack pointer directly which seemed to not to be
> removed.

And those that with an address relative to hard_frame_pointer_rtx, marks the
mem as volatile.

> I had originally was going to record my findings but then I saw the
> volatile for pa risc and deleted what I had wrote up.

Ouch, "never delete what you can't undo".  Sometimes you turn back another 180
degrees from your 180 turn in the middle of the analysis or bug-hunt.  Was it
more than a list of targets and their EH macros and patterns?

The fun thing is that the dse1 pass (the culprit) works before pro/epilogue
expansion, so it sees stores without the matching loads.  That is, for targets
that just define EH_RETURN_HANDLER_RTX (no eh_return pattern or any fancy
extra) and handles EH at epilogue expansion time.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #11 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Jiu Fu Guo from comment #8)
> I'm wondering if we need to revert r14-6674 to avoid this functionality
> issue. And revisit/enhance the patch later.

No need, not anymore; not because of this PR anyway.  I'm closing the bug, but
please don't back-port that commit (or at least, please back-port this commit
as well if you do).

Happy holidays.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #12 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
Writing and doing are different things...

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[guojiufu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #13 from Jiu Fu Guo <guojiufu at gcc dot gnu.org> ---
(In reply to GCC Commits from comment #9)
> The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:3d03630b123411340e52d05124cb0cacfa1fc8b0
> 
> commit r14-6817-g3d03630b123411340e52d05124cb0cacfa1fc8b0
> Author: Hans-Peter Nilsson <hp@axis.com>
> Date:   Sun Dec 24 00:10:32 2023 +0100
> 
>     CRIS: Fix PR middle-end/113109; "throw" failing
>     
>     TL;DR: the "dse1" pass removed the eh-return-address store.  The
>     PA also marks its EH_RETURN_HANDLER_RTX as volatile, for the same
>     reason, as does visum.  See PR32769 - it's the same thing on PA.
>     
>     Conceptually, it's logical that stores to incoming args are
>     optimized out on the return path or if no loads are seen -
>     at least before epilogue expansion, when the subsequent load
>     isn't seen in the RTL, as is the case for the "dse1" pass.

The stores to the argp/frame can be eliminated only if they are not used.
While for this case, the store may be used by EH handler, it should not be
optimized out. 

Thanks for catching and handling this quickly.

Happy holidays.

[Bug middle-end/113109] [14 Regression] g++ EH tests fail at execution time for cris-elf after r14-6674-g4759383245ac97

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113109

--- Comment #14 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to GCC Commits from comment #9)
> The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:3d03630b123411340e52d05124cb0cacfa1fc8b0
> 
> commit r14-6817-g3d03630b123411340e52d05124cb0cacfa1fc8b0
> Author: Hans-Peter Nilsson <hp@axis.com>
> Date:   Sun Dec 24 00:10:32 2023 +0100
>    
>     I haven't looked into why this problem, that appeared for the PA
>     already in 2007, was seen for CRIS only recently (with
>     r14-6674-g4759383245ac97).

I should've removed that paragraph before committing.  It's obvious from
looking at r14-6674-g4759383245ac97 and the history of the expression in that
context, and knowing that pa uses hard_frame_pointer_rtx...

Still, why not all those other targets?

(In reply to Jiu Fu Guo from comment #13)
> (In reply to GCC Commits from comment #9)
> >     Conceptually, it's logical that stores to incoming args are
> >     optimized out on the return path or if no loads are seen -
> >     at least before epilogue expansion, when the subsequent load
> >     isn't seen in the RTL, as is the case for the "dse1" pass.
> 
> The stores to the argp/frame can be eliminated only if they are not used.
> While for this case, the store may be used by EH handler, it should not be
> optimized out. 

That use is not seen before the pro/epilogue expansion pass.  Maybe the pass
should be restricted in what it does prior to that (not do in dse1, do in 
dse2).

> Thanks for catching and handling this quickly.
> 
> Happy holidays.

No worries, same to you!