[Bug target/103274] New: Remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[Bug target/103274] New: Remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[tomas.kalibera at gmail dot com]

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

            Bug ID: 103274
           Summary: Remaining -freorder-blocks-and-partition/ glitch with
                    Windows SEH
           Product: gcc
           Version: 10.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: tomas.kalibera at gmail dot com
  Target Milestone: ---

Created attachment 51809
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51809&action=edit
When compiled with -O3, dummy ends in a call (invalid unwind information).

-freorder-blocks-and-partition sometimes causes a function to end right in a
(non-returning) call, but SEH needs at least one more instruction on x86_64.
Seen in GCC 10.3, 11.2 and git master. Maybe [1] did not cover all the cases?

The attached compile-only example compiled with -O3 reproduces the problem. It
is extracted from R, where this problem causes crashes (and where in wine, one
gets an error "virtual_unwind exception data not found" and further
instrumentation reveals it is because the address following the call
instruction is already at the function boundary). The problem can be seen
directly from the assembly:

gcc -O3 -c -S ../main/a.c -o -

produces

dummy:
 [...]
.L5:
        call    myerrorcall
        .seh_endproc
 [...]
dummy.cold:

For reference, R is affected by this and has been disabling SEH as a
workaround, but it has been reported that the workaround causes in turns
crashes with CFG.

This report is with substantial help from Martin Storsjo.

=======

[1]
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=15278fb2877184c75a6ee3a6def09efbb191968b;hp=9d3b9a3e70e634c7c48bb12bb35ec8219024f98b

[2]
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/i386/i386.c;h=1bca5a7eea6ab9accbbf6953f79e8a4da61859e2;hb=4212a6a3e44f870412d9025eeb323fd4f50a61da#l29076

[3]
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86AvoidTrailingCall.cpp

[4] https://bugs.r-project.org/show_bug.cgi?id=18180

[Bug target/103274] Remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-11-16
             Status|UNCONFIRMED                 |WAITING

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> -freorder-blocks-and-partition sometimes causes a function to end right in a
> (non-returning) call, but SEH needs at least one more instruction on x86_64.
> Seen in GCC 10.3, 11.2 and git master. Maybe [1] did not cover all the cases?

SEH means "Structured Exception Handling" but there is no exception handling in
this chunk of program since it's written in C and compiled without
-fexceptions, so I'm not quite sure what you're expecting here.

[Bug target/103274] Remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[tomas.kalibera at gmail dot com]

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

--- Comment #2 from Tomas Kalibera <tomas.kalibera at gmail dot com> ---
(In reply to Eric Botcazou from comment #1)
> > -freorder-blocks-and-partition sometimes causes a function to end right in a
> > (non-returning) call, but SEH needs at least one more instruction on x86_64.
> > Seen in GCC 10.3, 11.2 and git master. Maybe [1] did not cover all the cases?
> 
> SEH means "Structured Exception Handling" but there is no exception handling
> in this chunk of program since it's written in C and compiled without
> -fexceptions, so I'm not quite sure what you're expecting here.

This also causes crashes with setjmp/longjmp only (no C++ exceptions, no
explicit use of C-exceptions in the C code).

R triggers unwinding via long jumps (explicitly calling longjmp in C, to
implement error handling in R), of the frames between setjmp and longjmp. The
unwinding sometimes crashes due to the problem reported, because it does not
find the unwinding information for some frames. Windows looks for a function
matching a given instruction pointer, which happens to be right after the call
causing the long jump. However, when the function end is marked right after
such a call (such as in the example), the instruction pointer is regarded past
the function end, and hence it is not matched to the function it should be.

We were using workarounds to disable SEH during unwinding, via redefining
setjmp to __intrinsic_setjmpex((x), NULL), so using the NULL frame argument
instead of the default __builtin_frame_address(0).

[Bug target/103274] Remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[martin at martin dot st]

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

Martin Storsjö <martin at martin dot st> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |martin at martin dot st

--- Comment #3 from Martin Storsjö <martin at martin dot st> ---
(In reply to Eric Botcazou from comment #1)
> > -freorder-blocks-and-partition sometimes causes a function to end right in a
> > (non-returning) call, but SEH needs at least one more instruction on x86_64.
> > Seen in GCC 10.3, 11.2 and git master. Maybe [1] did not cover all the cases?
> 
> SEH means "Structured Exception Handling" but there is no exception handling
> in this chunk of program since it's written in C and compiled without
> -fexceptions, so I'm not quite sure what you're expecting here.

Even if it doesn't have explicit exception handling, there's still unwind
information generated, and the needed "nop" instruction between the trailing
"call" instruction and ".seh_endproc" is missing.

[Bug target/103274] Remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[martin at martin dot st]

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

--- Comment #4 from Martin Storsjö <martin at martin dot st> ---
Also for additional context; with GCC 9.x, this testcase had the needed nop
instruction between "call" and ".seh_endproc".

In GCC 10.x (regressed in
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=095f78c62157124ad479a3f98b6995ced090b807),
-freorder-blocks-and-partition applies on this function and it gets split into
a hot and cold part, and the needed "nop" instruction disappeared.

If building with -fno-reorder-blocks-and-partition, the issue disappears and
you get roughly the same output as in GCC 9.x

This seems like the same issue as was fixed in
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=15278fb2877184c75a6ee3a6def09efbb191968b;hp=9d3b9a3e70e634c7c48bb12bb35ec8219024f98b
- but that fix doesn't seem to help here, as builds of GCC that include that
commit still are missing the nop between "call" and ".seh_endproc".

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.4
             Status|WAITING                     |NEW
            Summary|Remaining                   |[10/11/12 regression]
                   |-freorder-blocks-and-partit |remaining
                   |ion/ glitch with Windows    |-freorder-blocks-and-partit
                   |SEH                         |ion/ glitch with Windows
                   |                            |SEH

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
OK, if it's a regression, then let's so something indeed.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |ebotcazou at gcc dot gnu.org

--- Comment #6 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Investigating.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:bc8d6c60137f8bbf173b86ddf31b15d7ba2a33dd

commit r12-5598-gbc8d6c60137f8bbf173b86ddf31b15d7ba2a33dd
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Tue Nov 30 10:17:09 2021 +0100

    Fix -freorder-blocks-and-partition glitch with Windows SEH (continued)

    This fixes a thinko in the fix for the -freorder-blocks-and-partition
    glitch with SEH on 64-bit Windows:
      https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565208.html

    Even if no exceptions are active, e.g. in C, we need to consider calls.

    gcc/
            PR target/103274
            * config/i386/i386.c (ix86_output_call_insn): Beef up comment about
            nops emitted with SEH.
            * config/i386/winnt.c (i386_pe_seh_unwind_emit): When switching to
            the cold section, emit a nop before the directive if the previous
            active instruction is a call.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:aef7d09baf91341540e7d468419918d58dd33601

commit r11-9343-gaef7d09baf91341540e7d468419918d58dd33601
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Tue Nov 30 10:17:09 2021 +0100

    Fix -freorder-blocks-and-partition glitch with Windows SEH (continued)

    This fixes a thinko in the fix for the -freorder-blocks-and-partition
    glitch with SEH on 64-bit Windows:
      https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565208.html

    Even if no exceptions are active, e.g. in C, we need to consider calls.

    gcc/
            PR target/103274
            * config/i386/i386.c (ix86_output_call_insn): Beef up comment about
            nops emitted with SEH.
            * config/i386/winnt.c (i386_pe_seh_unwind_emit): When switching to
            the cold section, emit a nop before the directive if the previous
            active instruction is a call.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:ecc34ba16920957de21f89fc8b4c9efef7786f72

commit r10-10316-gecc34ba16920957de21f89fc8b4c9efef7786f72
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Tue Nov 30 10:17:09 2021 +0100

    Fix -freorder-blocks-and-partition glitch with Windows SEH (continued)

    This fixes a thinko in the fix for the -freorder-blocks-and-partition
    glitch with SEH on 64-bit Windows:
      https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565208.html

    Even if no exceptions are active, e.g. in C, we need to consider calls.

    gcc/
            PR target/103274
            * config/i386/i386.c (ix86_output_call_insn): Beef up comment about
            nops emitted with SEH.
            * config/i386/winnt.c (i386_pe_seh_unwind_emit): When switching to
            the cold section, emit a nop before the directive if the previous
            active instruction is a call.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

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

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Thanks for reporting the problem.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[martin at martin dot st]

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

--- Comment #11 from Martin Storsjö <martin at martin dot st> ---
(In reply to Eric Botcazou from comment #10)
> Thanks for reporting the problem.

Thanks for the fix! I can confirm that the version of the patch backported on
the gcc-10 branch fixes the testcase at least. Let's hope that it manages to
fix all the instances of the issue in real-world use too.

[Bug target/103274] [10/11/12 regression] remaining -freorder-blocks-and-partition/ glitch with Windows SEH

[tomas.kalibera at gmail dot com]

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

--- Comment #12 from Tomas Kalibera <tomas.kalibera at gmail dot com> ---
I've tested with GCC 10.3 with R. R can be built and passes its tests (without
the patch, it crashes). Also, I've checked the generated assembly with an awk
script looking for a call instruction immediately followed by .seh_endproc. I
didn't find any (while there were many without the patch, based on one the
attached example was created). Thanks for the fix.