[PR102988] harden cond: detach without decls

[PR102988] harden cond: detach without decls

[Alexandre Oliva]

When we create copies of SSA_NAMEs to hold "detached" copies of the
values for the hardening tests, we end up with assignments to
SSA_NAMEs that refer to the same decls.  That would be generally
desirable, since it enables the variable to be recognized in dumps,
and makes coalescing more likely if the original variable dies at that
point.  When the decl is a DECL_BY_REFERENCE, the SSA_NAME holds the
address of a parm or result, and it's read-only, so we shouldn't
create assignments to it.  Gimple checkers flag at least the case of
results.

This patch arranges for us to avoid referencing the same decls, which
cures the problem, but retaining the visible association between the
SSA_NAMEs, by using the same identifier for the copy.

Regstrapped on x86_64-linux-gnu, also bootstrapped with both
-fharden-compares and -fharden-conditional-branches enabled by default.
Ok to install?


for  gcc/ChangeLog

	PR tree-optimization/102988
	* gimple-harden-conditionals.cc (detach_value): Copy SSA_NAME
	without decl sharing.

for  gcc/testsuite/ChangeLog

	PR tree-optimization/102988
	* g++.dg/pr102988.C: New.
---
 gcc/gimple-harden-conditionals.cc |    9 ++++++++-
 gcc/testsuite/g++.dg/pr102988.C   |   17 +++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/pr102988.C

diff --git a/gcc/gimple-harden-conditionals.cc b/gcc/gimple-harden-conditionals.cc
index 8916420d7dfe9..cfa2361d65be0 100644
--- a/gcc/gimple-harden-conditionals.cc
+++ b/gcc/gimple-harden-conditionals.cc
@@ -123,7 +123,14 @@ detach_value (location_t loc, gimple_stmt_iterator *gsip, tree val)
       return val;
     }
 
-  tree ret = copy_ssa_name (val);
+  /* Create a SSA "copy" of VAL.  This could be an anonymous
+     temporary, but it's nice to have it named after the corresponding
+     variable.  Alas, when VAL is a DECL_BY_REFERENCE RESULT_DECL,
+     setting (a copy of) it would be flagged by checking, so we don't
+     use copy_ssa_name: we create an anonymous SSA name, and then give
+     it the same identifier (rather than decl) as VAL.  */
+  tree ret = make_ssa_name (TREE_TYPE (val));
+  SET_SSA_NAME_VAR_OR_IDENTIFIER (ret, SSA_NAME_IDENTIFIER (val));
 
   /* Output asm ("" : "=g" (ret) : "0" (val));  */
   vec<tree, va_gc> *inputs = NULL;
diff --git a/gcc/testsuite/g++.dg/pr102988.C b/gcc/testsuite/g++.dg/pr102988.C
new file mode 100644
index 0000000000000..05a1a8f67ed9b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr102988.C
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fharden-conditional-branches -fchecking=1" } */
+
+/* DECL_BY_REFERENCE RESULT_DECL is read-only, we can't create a copy
+   of its (address) default def and set it.  */
+
+void ll();
+struct k {
+  ~k();
+};
+k ice(k *a)
+{
+  k v;
+  if (&v!= a)
+    ll();
+  return v;
+}


-- 
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: [PR102988] harden cond: detach without decls

[Richard Biener]

On Fri, Nov 19, 2021 at 2:38 PM Alexandre Oliva via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> When we create copies of SSA_NAMEs to hold "detached" copies of the
> values for the hardening tests, we end up with assignments to
> SSA_NAMEs that refer to the same decls.  That would be generally
> desirable, since it enables the variable to be recognized in dumps,
> and makes coalescing more likely if the original variable dies at that
> point.  When the decl is a DECL_BY_REFERENCE, the SSA_NAME holds the
> address of a parm or result, and it's read-only, so we shouldn't
> create assignments to it.  Gimple checkers flag at least the case of
> results.
>
> This patch arranges for us to avoid referencing the same decls, which
> cures the problem, but retaining the visible association between the
> SSA_NAMEs, by using the same identifier for the copy.
>
> Regstrapped on x86_64-linux-gnu, also bootstrapped with both
> -fharden-compares and -fharden-conditional-branches enabled by default.
> Ok to install?

OK.

>
> for  gcc/ChangeLog
>
>         PR tree-optimization/102988
>         * gimple-harden-conditionals.cc (detach_value): Copy SSA_NAME
>         without decl sharing.
>
> for  gcc/testsuite/ChangeLog
>
>         PR tree-optimization/102988
>         * g++.dg/pr102988.C: New.
> ---
>  gcc/gimple-harden-conditionals.cc |    9 ++++++++-
>  gcc/testsuite/g++.dg/pr102988.C   |   17 +++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/pr102988.C
>
> diff --git a/gcc/gimple-harden-conditionals.cc b/gcc/gimple-harden-conditionals.cc
> index 8916420d7dfe9..cfa2361d65be0 100644
> --- a/gcc/gimple-harden-conditionals.cc
> +++ b/gcc/gimple-harden-conditionals.cc
> @@ -123,7 +123,14 @@ detach_value (location_t loc, gimple_stmt_iterator *gsip, tree val)
>        return val;
>      }
>
> -  tree ret = copy_ssa_name (val);
> +  /* Create a SSA "copy" of VAL.  This could be an anonymous
> +     temporary, but it's nice to have it named after the corresponding
> +     variable.  Alas, when VAL is a DECL_BY_REFERENCE RESULT_DECL,
> +     setting (a copy of) it would be flagged by checking, so we don't
> +     use copy_ssa_name: we create an anonymous SSA name, and then give
> +     it the same identifier (rather than decl) as VAL.  */
> +  tree ret = make_ssa_name (TREE_TYPE (val));
> +  SET_SSA_NAME_VAR_OR_IDENTIFIER (ret, SSA_NAME_IDENTIFIER (val));
>
>    /* Output asm ("" : "=g" (ret) : "0" (val));  */
>    vec<tree, va_gc> *inputs = NULL;
> diff --git a/gcc/testsuite/g++.dg/pr102988.C b/gcc/testsuite/g++.dg/pr102988.C
> new file mode 100644
> index 0000000000000..05a1a8f67ed9b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr102988.C
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fharden-conditional-branches -fchecking=1" } */
> +
> +/* DECL_BY_REFERENCE RESULT_DECL is read-only, we can't create a copy
> +   of its (address) default def and set it.  */
> +
> +void ll();
> +struct k {
> +  ~k();
> +};
> +k ice(k *a)
> +{
> +  k v;
> +  if (&v!= a)
> +    ll();
> +  return v;
> +}
>
>
> --
> 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>