[Bug target/96536] New: -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[Bug target/96536] New: -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[rsandifo at gcc dot gnu.org]

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

            Bug ID: 96536
           Summary: -fcf-protection code in i386.md:restore_stack_nonlocal
                    uses invalid compare-and-jump rtl
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rsandifo at gcc dot gnu.org
  Target Milestone: ---
            Target: x86_64

Noticed while working on an optimisation that the
-fcf-protection code in restore_stack_nonlocal uses
a branch based on a clobber result:

(insn 14 13 15 2 (parallel [
            (set (reg:DI 85)
                (minus:DI (reg:DI 85)
                    (mem:DI (plus:DI (reg/f:DI 82)
                            (const_int 16 [0x10])) [2  S8 A8])))
            (clobber (reg:CC 17 flags))
        ]) "gcc/testsuite/gcc.target/i386/cet-sjlj-1.c":16:3 -1
     (nil))
(jump_insn 15 14 40 2 (set (pc)
        (if_then_else (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (label_ref 28)
            (pc))) "gcc/testsuite/gcc.target/i386/cet-sjlj-1.c":16:3 -1
     (int_list:REG_BR_PROB 536870916 (nil))
 -> 28)

IMO this is invalid RTL.  A clobber:

  Represents the storing or possible storing of an unpredictable,
  undescribed value …

but this usage requires it to be predictable.  I think we need
to use the parallel-SET *sub<mode>_2 representation instead.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[crazylht at gmail dot com]

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

--- Comment #1 from Hongtao.liu <crazylht at gmail dot com> ---
I'm testing patch like

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index b24a4557871..269c528c3ad 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -19132,15 +19132,15 @@

       /* Compare through substraction the saved and the current ssp to decide
         if ssp has to be adjusted.  */
-      tmp = gen_rtx_SET (reg_ssp, gen_rtx_MINUS (word_mode, reg_ssp,
-                                                ssp_slot));
-      clob = gen_rtx_CLOBBER (VOIDmode, gen_rtx_REG (CCmode, FLAGS_REG));
-      tmp = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, tmp, clob));
-      emit_insn (tmp);
+      emit_insn ((word_mode == SImode)
+                 ? gen_subsi3 (reg_ssp, reg_ssp, ssp_slot)
+                 : gen_subdi3 (reg_ssp, reg_ssp, ssp_slot));
+      tmp = gen_rtx_COMPARE (CCZmode, reg_ssp, const0_rtx);
+      flags = gen_rtx_REG (CCZmode, FLAGS_REG);
+      emit_insn (gen_rtx_SET (flags, tmp));

       /* Compare and jump over adjustment code.  */
       noadj_label = gen_label_rtx ();
-      flags = gen_rtx_REG (CCZmode, FLAGS_REG);
       tmp = gen_rtx_EQ (VOIDmode, flags, const0_rtx);
       tmp = gen_rtx_IF_THEN_ELSE (VOIDmode, tmp,
                                  gen_rtx_LABEL_REF (VOIDmode, noadj_label),
@@ -19184,14 +19184,10 @@
       emit_insn ((word_mode == SImode)
                 ? gen_incsspsi (reg_255)
                 : gen_incsspdi (reg_255));
-      tmp = gen_rtx_SET (reg_adj, gen_rtx_MINUS (ptr_mode,
-                                                reg_adj,
-                                                GEN_INT (255)));
-      clob = gen_rtx_CLOBBER (VOIDmode, gen_rtx_REG (CCmode, FLAGS_REG));
-      tmp = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, tmp, clob));
-      emit_insn (tmp);
-
-      tmp = gen_rtx_COMPARE (CCmode, reg_adj, GEN_INT (255));
+      emit_insn ((ptr_mode == SImode)
+                 ? gen_subsi3 (reg_adj, reg_adj, GEN_INT (255))
+                 : gen_subdi3 (reg_adj, reg_adj, GEN_INT (255)));
+      tmp = gen_rtx_COMPARE (CCmode, reg_adj, const0_rtx);
       flags = gen_rtx_REG (CCmode, FLAGS_REG);
       emit_insn (gen_rtx_SET (flags, tmp));

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[ubizjak at gmail dot com]

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

--- Comment #2 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Hongtao.liu from comment #1)
> I'm testing patch like

You can probably use gen_sub2_insn here.

On a related note, "@" prefix can be used for rdssp, so one can pass mode to an
expander. This would eliminate e.g. "(word_mode == SImode) ? gen_rdsspsi
(reg_ssp) : gen_rdsspdi (reg_ssp)" constructs with just one call to the
expander.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[crazylht at gmail dot com]

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

--- Comment #3 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Uroš Bizjak from comment #2)
> (In reply to Hongtao.liu from comment #1)
> > I'm testing patch like
> 
> You can probably use gen_sub2_insn here.
> 
> On a related note, "@" prefix can be used for rdssp, so one can pass mode to
> an expander. This would eliminate e.g. "(word_mode == SImode) ? gen_rdsspsi
> (reg_ssp) : gen_rdsspdi (reg_ssp)" constructs with just one call to the
> expander.

Yes, thanks for pointing out.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[ubizjak at gmail dot com]

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

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |ubizjak at gmail dot com
   Last reconfirmed|                            |2020-08-13
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

--- Comment #4 from Uroš Bizjak <ubizjak at gmail dot com> ---
Created attachment 49060
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49060&action=edit
Proposed patch

Attached patch completely rewrites restore_stack_nonlocal expander.

Can someone please test the patch on a CET enabled target?

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[crazylht at gmail dot com]

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

--- Comment #5 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Uroš Bizjak from comment #4)
> Created attachment 49060 [details]
> Proposed patch
> 
> Attached patch completely rewrites restore_stack_nonlocal expander.
> 
> Can someone please test the patch on a CET enabled target?

I can help with this, we have CET enabled tigerlake.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[ubizjak at gmail dot com]

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

--- Comment #6 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Hongtao.liu from comment #1)
> I'm testing patch like
>
>        emit_insn ((word_mode == SImode)
>                  ? gen_incsspsi (reg_255)
>                  : gen_incsspdi (reg_255));
> -      tmp = gen_rtx_SET (reg_adj, gen_rtx_MINUS (ptr_mode,
> -                                                reg_adj,
> -                                                GEN_INT (255)));
> -      clob = gen_rtx_CLOBBER (VOIDmode, gen_rtx_REG (CCmode, FLAGS_REG));
> -      tmp = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, tmp, clob));
> -      emit_insn (tmp);
> -
> -      tmp = gen_rtx_COMPARE (CCmode, reg_adj, GEN_INT (255));
> +      emit_insn ((ptr_mode == SImode)
> +                 ? gen_subsi3 (reg_adj, reg_adj, GEN_INT (255))
> +                 : gen_subdi3 (reg_adj, reg_adj, GEN_INT (255)));
> +      tmp = gen_rtx_COMPARE (CCmode, reg_adj, const0_rtx);
>        flags = gen_rtx_REG (CCmode, FLAGS_REG);
>        emit_insn (gen_rtx_SET (flags, tmp));

The above part is not correct. The original code compares result with 255, your
patch compares result with 0.

So, the minimum patch (for backport) should just introduce:

--cut here--
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 292de142e90..6c207be3512 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -18695,6 +18695,10 @@
       tmp = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, tmp, clob));
       emit_insn (tmp);

+      tmp = gen_rtx_COMPARE (CCZmode, reg_ssp, const0_rtx);
+      flags = gen_rtx_REG (CCZmode, FLAGS_REG);
+      emit_insn (gen_rtx_SET (flags, tmp));
+
       /* Compare and jump over adjustment code.  */
       noadj_label = gen_label_rtx ();
       flags = gen_rtx_REG (CCZmode, FLAGS_REG);
--cut here--

The patch creates correct form of sub insn (tested with cet-sjlj-1.c testcase):

#(insn 15 14 16 2 (parallel [
#            (set (reg:CCZ 17 flags)
#                (compare:CCZ (minus:DI (reg:DI 0 ax [85])
#                        (mem:DI (const:DI (plus:DI (symbol_ref:DI ("buf")
[flags 0x2] <var_decl 0x7f41c308cb40 buf>)
#                                    (const_int 16 [0x10]))) [2  S8 A8]))
#                    (const_int 0 [0])))
#            (set (reg:DI 0 ax [85])
#                (minus:DI (reg:DI 0 ax [85])
#                    (mem:DI (const:DI (plus:DI (symbol_ref:DI ("buf") [flags
0x2] <var_decl 0x7f41c308cb40 buf>)
#                                (const_int 16 [0x10]))) [2  S8 A8])))
#        ]) "cet-sjlj-1.c":16:3 262 {*subdi_2}
#     (nil))
        subq    buf+16(%rip), %rax      # 15    [c=8 l=7]  *subdi_2/1

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[crazylht at gmail dot com]

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

--- Comment #7 from Hongtao.liu <crazylht at gmail dot com> ---


(In reply to Hongtao.liu from comment #5)
> (In reply to Uroš Bizjak from comment #4)
> > Created attachment 49060 [details]
> > Proposed patch
> > 
> > Attached patch completely rewrites restore_stack_nonlocal expander.
> > 
> > Can someone please test the patch on a CET enabled target?
> 
> I can help with this, we have CET enabled tigerlake.

Bootstrap is ok, regression test for i386/x86-64 backend is ok.
(our cet-enabled tigerlake is pre-alpha version, test ran very slowly.)

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Uros Bizjak <uros@gcc.gnu.org>:

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

commit r11-2739-gf8104bb9dc2365d268ca93e43a24f42e8314fcc1
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Tue Aug 18 17:31:49 2020 +0200

    i386: Rewrite restore_stack_nonlocal expander [PR96536].

    -fcf-protection code in restore_stack_nonlocal uses a branch based on
    a clobber result.  The patch adds missing compare and completely
    rewrites the expander to use high-level functions in RTL construction.

    2020-08-18  Uroš Bizjak  <ubizjak@gmail.com>

    gcc/ChangeLog:

            PR target/96536
            * config/i386/i386.md (restore_stack_nonlocal): Add missing compare
            RTX.  Rewrite expander to use high-level functions in RTL
construction.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:6342cee8801f191466b71116d004e8ccb812caaa

commit r10-8638-g6342cee8801f191466b71116d004e8ccb812caaa
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Tue Aug 18 17:34:37 2020 +0200

    i386: Fix restore_stack_nonlocal expander [PR96536].

    -fcf-protection code in restore_stack_nonlocal uses a branch based on
    a clobber result.  The patch adds missing compare.

    2020-08-18  Uroš Bizjak  <ubizjak@gmail.com>

    gcc/ChangeLog:

            PR target/96536
            * config/i386/i386.md (restore_stack_nonlocal):
            Add missing compare RTX.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:65f460db575eb004172e75b88f5a76724f04e255

commit r9-8813-g65f460db575eb004172e75b88f5a76724f04e255
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Tue Aug 18 18:47:47 2020 +0200

    i386: Fix restore_stack_nonlocal expander [PR96536].

    -fcf-protection code in restore_stack_nonlocal uses a branch based on
    a clobber result.  The patch adds missing compare.

    2020-08-18  Uroš Bizjak  <ubizjak@gmail.com>

    gcc/ChangeLog:

            PR target/96536
            * config/i386/i386.md (restore_stack_nonlocal):
            Add missing compare RTX.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r8-10410-gbf7b9330982165e051de0962c5bc231e2d1242d9
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Tue Aug 18 19:48:51 2020 +0200

    i386: Fix restore_stack_nonlocal expander [PR96536].

    -fcf-protection code in restore_stack_nonlocal uses a branch based on
    a clobber result.  The patch adds missing compare.

    2020-08-18  Uroš Bizjak  <ubizjak@gmail.com>

    gcc/ChangeLog:

            PR target/96536
            * config/i386/i386.md (restore_stack_nonlocal):
            Add missing compare RTX.

[Bug target/96536] -fcf-protection code in i386.md:restore_stack_nonlocal uses invalid compare-and-jump rtl

[ubizjak at gmail dot com]

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

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |8.5
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Uroš Bizjak <ubizjak at gmail dot com> ---
Fixed everywhere.