harden-conditionals: detach values before compares

harden-conditionals: detach values before compares

[Alexandre Oliva]

The optimization barriers inserted after compares enable GCC to derive
information about the values from e.g. the taken paths, or the absence
of exceptions.  Move them before the original compares, so that the
reversed compares test copies of the original operands, without
further optimizations.

Regstrapped on x86_64-linux-gnu, and also bootstrapped with both passes
enabled.  Further tested on multiple other targets with gcc-12.  Ok to
install?


for  gcc/ChangeLog

	* gimple-harden-conditionals.cc (insert_edge_check_and_trap):
	Move detach value calls...
	(pass_harden_conditional_branches::execute): ... here.
	(pass_harden_compares::execute): Detach values before
	compares.

for  gcc/testsuite/ChangeLog

	* c-c++-common/torture/harden-cond-comp.c: New.
---
 gcc/gimple-harden-conditionals.cc                  |   25 ++++++++++++--------
 .../c-c++-common/torture/harden-cond-comp.c        |   24 +++++++++++++++++++
 2 files changed, 39 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/torture/harden-cond-comp.c

diff --git a/gcc/gimple-harden-conditionals.cc b/gcc/gimple-harden-conditionals.cc
index 78b8d5692d76f..2e5a42e9e71b1 100644
--- a/gcc/gimple-harden-conditionals.cc
+++ b/gcc/gimple-harden-conditionals.cc
@@ -276,8 +276,8 @@ insert_check_and_trap (location_t loc, gimple_stmt_iterator *gsip,
 }
 
 /* Split edge E, and insert_check_and_trap (see above) in the
-   newly-created block, using detached copies of LHS's and RHS's
-   values (see detach_value above) for the COP compare.  */
+   newly-created block, using already-detached copies of LHS's and
+   RHS's values (see detach_value above) for the COP compare.  */
 
 static inline void
 insert_edge_check_and_trap (location_t loc, edge e,
@@ -301,10 +301,6 @@ insert_edge_check_and_trap (location_t loc, edge e,
 
   gimple_stmt_iterator gsik = gsi_after_labels (chk);
 
-  bool same_p = (lhs == rhs);
-  lhs = detach_value (loc, &gsik, lhs);
-  rhs = same_p ? lhs : detach_value (loc, &gsik, rhs);
-
   insert_check_and_trap (loc, &gsik, flags, cop, lhs, rhs);
 }
 
@@ -366,6 +362,12 @@ pass_harden_conditional_branches::execute (function *fun)
 	/* ??? Can we do better?  */
 	continue;
 
+      /* Detach the values before the compares.  If we do so later,
+	 the compiler may use values inferred from the compares.  */
+      bool same_p = (lhs == rhs);
+      lhs = detach_value (loc, &gsi, lhs);
+      rhs = same_p ? lhs : detach_value (loc, &gsi, rhs);
+
       insert_edge_check_and_trap (loc, EDGE_SUCC (bb, 0), cop, lhs, rhs);
       insert_edge_check_and_trap (loc, EDGE_SUCC (bb, 1), cop, lhs, rhs);
     }
@@ -508,6 +510,13 @@ pass_harden_compares::execute (function *fun)
 
 	  tree rhs = copy_ssa_name (lhs);
 
+	  /* Detach the values before the compares, so that the
+	     compiler infers nothing from them, not even from a
+	     throwing compare that didn't throw.  */
+	  bool same_p = (op1 == op2);
+	  op1 = detach_value (loc, &gsi, op1);
+	  op2 = same_p ? op1 : detach_value (loc, &gsi, op2);
+
 	  gimple_stmt_iterator gsi_split = gsi;
 	  /* Don't separate the original assignment from debug stmts
 	     that might be associated with it, and arrange to split the
@@ -529,10 +538,6 @@ pass_harden_compares::execute (function *fun)
 			 gimple_bb (asgn)->index, nbb->index);
 	    }
 
-	  bool same_p = (op1 == op2);
-	  op1 = detach_value (loc, &gsi_split, op1);
-	  op2 = same_p ? op1 : detach_value (loc, &gsi_split, op2);
-
 	  gassign *asgnck = gimple_build_assign (rhs, cop, op1, op2);
 	  gimple_set_location (asgnck, loc);
 	  gsi_insert_before (&gsi_split, asgnck, GSI_SAME_STMT);
diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
new file mode 100644
index 0000000000000..5aad890a1d3b6
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
+
+int f(int i, int j) {
+  if (i == 0)
+    return j != 0;
+  else
+    return i * j != 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
+/* { dg-final { scan-tree-dump-times "Adding reversed compare" 2 "hardcbr" } } */
+/* { dg-final { scan-tree-dump-times "__builtin_trap" 2 "hardcbr" } } */
+
+/* { dg-final { scan-tree-dump-times "Splitting block" 2 "hardcmp" } } */
+/* { dg-final { scan-tree-dump-times "Adding reversed compare" 2 "hardcmp" } } */
+/* { dg-final { scan-tree-dump-times "__builtin_trap" 4 "hardcmp" } } */
+
+/* Check that the optimization barrier is placed before the original compare.  */
+/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0" i_[0-9]*[(]D[)][)][;][\n][ ]*if [(]i_[0-9]*[(]D[)] == 0[)]} 1 "hardcbr" } } */
+/* { dg-final { scan-tree-dump-times {if [(]_[0-9]* != 0[)]} 2 "hardcbr" } } */
+
+/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0" j_[0-9]*[(]D[)][)][;][\n][ ]*_[0-9]* = j_[0-9]*[(]D[)] != 0;[\n] *_[0-9]* = _[0-9]* == 0} 1 "hardcmp" } } */
+/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0" _[0-9]*[)][;][\n][ ]*_[0-9]* = _[0-9]* != 0;[\n] *_[0-9]* = _[0-9]* == 0} 1 "hardcmp" } } */


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: harden-conditionals: detach values before compares

[Richard Biener]

On Thu, Apr 27, 2023 at 1:48 PM Alexandre Oliva via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> The optimization barriers inserted after compares enable GCC to derive
> information about the values from e.g. the taken paths, or the absence
> of exceptions.  Move them before the original compares, so that the
> reversed compares test copies of the original operands, without
> further optimizations.
>
> Regstrapped on x86_64-linux-gnu, and also bootstrapped with both passes
> enabled.  Further tested on multiple other targets with gcc-12.  Ok to
> install?

OK.

>
> for  gcc/ChangeLog
>
>         * gimple-harden-conditionals.cc (insert_edge_check_and_trap):
>         Move detach value calls...
>         (pass_harden_conditional_branches::execute): ... here.
>         (pass_harden_compares::execute): Detach values before
>         compares.
>
> for  gcc/testsuite/ChangeLog
>
>         * c-c++-common/torture/harden-cond-comp.c: New.
> ---
>  gcc/gimple-harden-conditionals.cc                  |   25 ++++++++++++--------
>  .../c-c++-common/torture/harden-cond-comp.c        |   24 +++++++++++++++++++
>  2 files changed, 39 insertions(+), 10 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
>
> diff --git a/gcc/gimple-harden-conditionals.cc b/gcc/gimple-harden-conditionals.cc
> index 78b8d5692d76f..2e5a42e9e71b1 100644
> --- a/gcc/gimple-harden-conditionals.cc
> +++ b/gcc/gimple-harden-conditionals.cc
> @@ -276,8 +276,8 @@ insert_check_and_trap (location_t loc, gimple_stmt_iterator *gsip,
>  }
>
>  /* Split edge E, and insert_check_and_trap (see above) in the
> -   newly-created block, using detached copies of LHS's and RHS's
> -   values (see detach_value above) for the COP compare.  */
> +   newly-created block, using already-detached copies of LHS's and
> +   RHS's values (see detach_value above) for the COP compare.  */
>
>  static inline void
>  insert_edge_check_and_trap (location_t loc, edge e,
> @@ -301,10 +301,6 @@ insert_edge_check_and_trap (location_t loc, edge e,
>
>    gimple_stmt_iterator gsik = gsi_after_labels (chk);
>
> -  bool same_p = (lhs == rhs);
> -  lhs = detach_value (loc, &gsik, lhs);
> -  rhs = same_p ? lhs : detach_value (loc, &gsik, rhs);
> -
>    insert_check_and_trap (loc, &gsik, flags, cop, lhs, rhs);
>  }
>
> @@ -366,6 +362,12 @@ pass_harden_conditional_branches::execute (function *fun)
>         /* ??? Can we do better?  */
>         continue;
>
> +      /* Detach the values before the compares.  If we do so later,
> +        the compiler may use values inferred from the compares.  */
> +      bool same_p = (lhs == rhs);
> +      lhs = detach_value (loc, &gsi, lhs);
> +      rhs = same_p ? lhs : detach_value (loc, &gsi, rhs);
> +
>        insert_edge_check_and_trap (loc, EDGE_SUCC (bb, 0), cop, lhs, rhs);
>        insert_edge_check_and_trap (loc, EDGE_SUCC (bb, 1), cop, lhs, rhs);
>      }
> @@ -508,6 +510,13 @@ pass_harden_compares::execute (function *fun)
>
>           tree rhs = copy_ssa_name (lhs);
>
> +         /* Detach the values before the compares, so that the
> +            compiler infers nothing from them, not even from a
> +            throwing compare that didn't throw.  */
> +         bool same_p = (op1 == op2);
> +         op1 = detach_value (loc, &gsi, op1);
> +         op2 = same_p ? op1 : detach_value (loc, &gsi, op2);
> +
>           gimple_stmt_iterator gsi_split = gsi;
>           /* Don't separate the original assignment from debug stmts
>              that might be associated with it, and arrange to split the
> @@ -529,10 +538,6 @@ pass_harden_compares::execute (function *fun)
>                          gimple_bb (asgn)->index, nbb->index);
>             }
>
> -         bool same_p = (op1 == op2);
> -         op1 = detach_value (loc, &gsi_split, op1);
> -         op2 = same_p ? op1 : detach_value (loc, &gsi_split, op2);
> -
>           gassign *asgnck = gimple_build_assign (rhs, cop, op1, op2);
>           gimple_set_location (asgnck, loc);
>           gsi_insert_before (&gsi_split, asgnck, GSI_SAME_STMT);
> diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> new file mode 100644
> index 0000000000000..5aad890a1d3b6
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fharden-conditional-branches -fharden-compares -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
> +
> +int f(int i, int j) {
> +  if (i == 0)
> +    return j != 0;
> +  else
> +    return i * j != 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
> +/* { dg-final { scan-tree-dump-times "Adding reversed compare" 2 "hardcbr" } } */
> +/* { dg-final { scan-tree-dump-times "__builtin_trap" 2 "hardcbr" } } */
> +
> +/* { dg-final { scan-tree-dump-times "Splitting block" 2 "hardcmp" } } */
> +/* { dg-final { scan-tree-dump-times "Adding reversed compare" 2 "hardcmp" } } */
> +/* { dg-final { scan-tree-dump-times "__builtin_trap" 4 "hardcmp" } } */
> +
> +/* Check that the optimization barrier is placed before the original compare.  */
> +/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0" i_[0-9]*[(]D[)][)][;][\n][ ]*if [(]i_[0-9]*[(]D[)] == 0[)]} 1 "hardcbr" } } */
> +/* { dg-final { scan-tree-dump-times {if [(]_[0-9]* != 0[)]} 2 "hardcbr" } } */
> +
> +/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0" j_[0-9]*[(]D[)][)][;][\n][ ]*_[0-9]* = j_[0-9]*[(]D[)] != 0;[\n] *_[0-9]* = _[0-9]* == 0} 1 "hardcmp" } } */
> +/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0" _[0-9]*[)][;][\n][ ]*_[0-9]* = _[0-9]* != 0;[\n] *_[0-9]* = _[0-9]* == 0} 1 "hardcmp" } } */
>
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>