[Bug middle-end/104971] New: Optimisation for __builtin_ia32_readeflags corrupts the stack

[Bug middle-end/104971] New: Optimisation for __builtin_ia32_readeflags corrupts the stack

[andrew.cooper3 at citrix dot com]

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

            Bug ID: 104971
           Summary: Optimisation for __builtin_ia32_readeflags corrupts
                    the stack
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: andrew.cooper3 at citrix dot com
  Target Milestone: ---

Full example: https://godbolt.org/z/xGq3c4Mnc

Given:

int broken(void)
{
    int fl = __builtin_ia32_readeflags_u64();
}

gcc -O2 generates:

broken:
        pushfq
        ret

Which is going explode very quickly.

Code generation appears to be safe without optimisation, but even -O alone is
enough to create problems.

At a guess, the optimiser has concluded that the result is unused, drops the
`pop %reg`, but fails to also drop the `pushf` too.

Looking through history on Godbolt, it appears that GCC 4.9 (which introduced
this builtin) has correct optimised code generation, and it regressed between
4.9 and 5.1.

[Bug middle-end/104971] [9/10/11/12 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |9.5
            Summary|Optimisation for            |[9/10/11/12 Regression]
                   |__builtin_ia32_readeflags   |Optimisation for
                   |corrupts the stack          |__builtin_ia32_readeflags
                   |                            |corrupts the stack
                 CC|                            |jakub at gcc dot gnu.org
           Priority|P3                          |P2

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The builtin has been implemented in
r0-127024-g9bbd48d120d203e8eade09e0bb830370b6d69801
and the pop is optimized away with pushf kept since
r5-4951-g4ab74a01477d4089a3474d52479ed372c9b5ae29
so this is a regression from GCC 4.9.

[Bug middle-end/104971] [9/10/11/12 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vmakarov at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Might be useful to drop the builtin on the floor during expansion if it doesn't
have lhs as an optimization (I think it is intentionally not const or pure
because it depends on any code that sets the flags; though it seems rather
poorly defined because we don't have any dependencies on the surrounding code,
but after all, user can't know what arithmetic instruction that modifies flags
we decide to use last before the builtin).
That said, LRA shouldn't optimize away instructions like:
(insn 6 5 0 2 (set (reg:DI 82)
        (mem:DI (post_inc:DI (reg/f:DI 7 sp)) [0  S8 A8])) "pr104971.c":3:14 62
{*popdi1}
     (expr_list:REG_UNUSED (reg:DI 82)
        (nil)))
just because they are unused, because they have side-effects.

[Bug middle-end/104971] [9/10/11/12 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[andrew.cooper3 at citrix dot com]

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

--- Comment #3 from Andrew Cooper <andrew.cooper3 at citrix dot com> ---
So yes - my experimentation did start from investigating the memory ordering
behaviour of these builtins, based on a thread on LKML.

The pushf in readflags and popf in writeflags have wildly different ordering
requirements, depending on which flags are wanted/modified.  AC for example
(and IF for kernels) need to not be reordered with respect to any memory
access.

As you observe, readflags in particular needs to not be reordered with any
instruction that modifies the arithmetic flags (which is most of them).

IMO, it would be safe to omit the pushf from readflags if the result is not not
used, because there are no unexpected side effects for pushf.

The same is not true of popf in writeflags, which has side effects even when
written twice with the same value.

[Bug middle-end/104971] [9/10/11/12 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-03-17
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Confirmed then.

[Bug middle-end/104971] [9/10/11/12 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 52650
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52650&action=edit
gcc12-pr104971.patch

Here is untested patch with the optimization not to expand the pushf/pop at all
if we don't need the result.  Though, the LRA bug will remain latent with it
and should be fixed anyway.

[Bug middle-end/104971] [9/10/11/12 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

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

commit r12-7721-gb60bc913cca7439d29a7ec9e9a7f448d8841b43c
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Mar 19 13:53:12 2022 +0100

    i386: Don't emit pushf;pop for __builtin_ia32_readeflags_u* with unused lhs
[PR104971]

    __builtin_ia32_readeflags_u* aren't marked const or pure I think
    intentionally, so that they aren't CSEd from different regions of a
function
    etc. because we don't and can't easily track all dependencies between
    it and surrounding code (if somebody looks at the condition flags, it is
    dependent on the vast majority of instructions).
    But the builtin itself doesn't have any side-effects, so if we ignore the
    result of the builtin, there is no point to emit anything.

    There is a LRA bug that miscompiles the testcase which this patch makes
    latent, which is certainly worth fixing too, but IMHO this change
    (and maybe ix86_gimple_fold_builtin too which would fold it even earlier
    when it looses lhs) is worth it as well.

    2022-03-19  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/104971
            * config/i386/i386-expand.cc
            (ix86_expand_builtin) <case IX86_BUILTIN_READ_FLAGS>: If ignore,
            don't push/pop anything and just return const0_rtx.

            * gcc.target/i386/pr104971.c: New test.

[Bug middle-end/104971] [9/10/11 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:25725506b85f478076770942d76799c54310c696

commit r12-7817-g25725506b85f478076770942d76799c54310c696
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Fri Mar 25 12:22:08 2022 -0400

    [PR104971] LRA: check live hard regs to remove a dead insn

    LRA removes insn modifying sp for given PR test set.  We should also have
    checked living hard regs to prevent this.  The patch fixes this.

    gcc/ChangeLog:

            PR middle-end/104971
            * lra-lives.cc (process_bb_lives): Check hard_regs_live for hard
            regs to clear remove_p flag.

[Bug middle-end/104971] [9/10/11 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r11-9733-gbe64c349e7312a4e89c569d42c4e2284610d4908
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Mar 19 13:53:12 2022 +0100

    i386: Don't emit pushf;pop for __builtin_ia32_readeflags_u* with unused lhs
[PR104971]

    __builtin_ia32_readeflags_u* aren't marked const or pure I think
    intentionally, so that they aren't CSEd from different regions of a
function
    etc. because we don't and can't easily track all dependencies between
    it and surrounding code (if somebody looks at the condition flags, it is
    dependent on the vast majority of instructions).
    But the builtin itself doesn't have any side-effects, so if we ignore the
    result of the builtin, there is no point to emit anything.

    There is a LRA bug that miscompiles the testcase which this patch makes
    latent, which is certainly worth fixing too, but IMHO this change
    (and maybe ix86_gimple_fold_builtin too which would fold it even earlier
    when it looses lhs) is worth it as well.

    2022-03-19  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/104971
            * config/i386/i386-expand.c
            (ix86_expand_builtin) <case IX86_BUILTIN_READ_FLAGS>: If ignore,
            don't push/pop anything and just return const0_rtx.

            * gcc.target/i386/pr104971.c: New test.

    (cherry picked from commit b60bc913cca7439d29a7ec9e9a7f448d8841b43c)

[Bug middle-end/104971] [9/10 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
            Summary|[9/10/11 Regression]        |[9/10 Regression]
                   |Optimisation for            |Optimisation for
                   |__builtin_ia32_readeflags   |__builtin_ia32_readeflags
                   |corrupts the stack          |corrupts the stack

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 11.3 too.

[Bug middle-end/104971] [9/10 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:07ab38979126c39563c2c75b8e40cb7ce9b479b3

commit r10-10697-g07ab38979126c39563c2c75b8e40cb7ce9b479b3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Mar 19 13:53:12 2022 +0100

    i386: Don't emit pushf;pop for __builtin_ia32_readeflags_u* with unused lhs
[PR104971]

    __builtin_ia32_readeflags_u* aren't marked const or pure I think
    intentionally, so that they aren't CSEd from different regions of a
function
    etc. because we don't and can't easily track all dependencies between
    it and surrounding code (if somebody looks at the condition flags, it is
    dependent on the vast majority of instructions).
    But the builtin itself doesn't have any side-effects, so if we ignore the
    result of the builtin, there is no point to emit anything.

    There is a LRA bug that miscompiles the testcase which this patch makes
    latent, which is certainly worth fixing too, but IMHO this change
    (and maybe ix86_gimple_fold_builtin too which would fold it even earlier
    when it looses lhs) is worth it as well.

    2022-03-19  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/104971
            * config/i386/i386-expand.c
            (ix86_expand_builtin) <case IX86_BUILTIN_READ_FLAGS>: If ignore,
            don't push/pop anything and just return const0_rtx.

            * gcc.target/i386/pr104971.c: New test.

    (cherry picked from commit b60bc913cca7439d29a7ec9e9a7f448d8841b43c)

[Bug middle-end/104971] [9/10 Regression] Optimisation for __builtin_ia32_readeflags corrupts the stack

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

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

commit r9-10140-gc1a8261b7054da31420e5c715e682c1b42e473b5
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Mar 19 13:53:12 2022 +0100

    i386: Don't emit pushf;pop for __builtin_ia32_readeflags_u* with unused lhs
[PR104971]

    __builtin_ia32_readeflags_u* aren't marked const or pure I think
    intentionally, so that they aren't CSEd from different regions of a
function
    etc. because we don't and can't easily track all dependencies between
    it and surrounding code (if somebody looks at the condition flags, it is
    dependent on the vast majority of instructions).
    But the builtin itself doesn't have any side-effects, so if we ignore the
    result of the builtin, there is no point to emit anything.

    There is a LRA bug that miscompiles the testcase which this patch makes
    latent, which is certainly worth fixing too, but IMHO this change
    (and maybe ix86_gimple_fold_builtin too which would fold it even earlier
    when it looses lhs) is worth it as well.

    2022-03-19  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/104971
            * config/i386/i386.c
            (ix86_expand_builtin) <case IX86_BUILTIN_READ_FLAGS>: If ignore,
            don't push/pop anything and just return const0_rtx.

            * gcc.target/i386/pr104971.c: New test.

    (cherry picked from commit b60bc913cca7439d29a7ec9e9a7f448d8841b43c)