[PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[apinski]

From: Andrew Pinski <apinski@marvell.com>

The Linux kernel started to fail compile when the jump threader was improved
(r12-2591-g2e96b5f14e4025691). This failure was due to the IPA splitting code
decided now to split off the basic block which contained two functions,
one of those functions included the error attribute on them.  This patch fixes
the problem by disallowing basic blocks from being split which contain functions
that have either the error or warning attribute on them.

The two new testcases are to make sure we still split the function for other
places if we reject the one case.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR tree-optimization/101941

gcc/ChangeLog:

	* ipa-split.c (visit_bb): Disallow function calls where
	the function has either error or warning attribute.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/compile/pr101941-1.c: New test.
	* gcc.dg/tree-ssa/pr101941-1.c: New test.
---
 gcc/ipa-split.c                               | 12 ++++-
 .../gcc.c-torture/compile/pr101941-1.c        | 44 +++++++++++++++++
 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c    | 48 +++++++++++++++++++
 3 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c

diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index c68577d04a9..070e894ef31 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
       gimple *stmt = gsi_stmt (bsi);
       tree op;
       ssa_op_iter iter;
-      tree decl;
+      tree decl = NULL_TREE;
 
       if (is_gimple_debug (stmt))
 	continue;
@@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
 	    break;
 	  }
 
+      /* If a function call and that function has either the
+	 warning or error attribute on it, don't split.  */
+      if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
+		   || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    fprintf (dump_file, "Cannot split: warning or error attribute.\n");
+	  can_split = false;
+	}
+
       FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
 	bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
       FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
new file mode 100644
index 00000000000..ab3bbea8ed7
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
@@ -0,0 +1,44 @@
+/* { dg-additional-options "-fconserve-stack" } */
+struct crypto_aes_ctx {
+  char key_dec[128];
+};
+
+int rfc4106_set_hash_subkey_hash_subkey;
+
+void __write_overflow(void)__attribute__((__error__("")));
+void __write_overflow1(void);
+void aes_encrypt(void*);
+
+void fortify_panic(const char*) __attribute__((__noreturn__)) ;
+
+char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
+  void *a = &ctx->key_dec[0];
+  unsigned p_size =  __builtin_object_size(a, 0);
+#ifdef __OPTIMIZE__
+  if (p_size < 16) {
+    __write_overflow1();
+    fortify_panic(__func__);
+  }
+  if (p_size < 32) {
+    __write_overflow();
+    fortify_panic(__func__);
+  }
+#endif
+  aes_encrypt(ctx);
+  return ctx->key_dec;
+}
+
+char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
+
+void a(void)
+{
+  struct crypto_aes_ctx ctx;
+  rfc4106_set_hash_subkey(&ctx);
+}
+void b(void)
+{
+  struct crypto_aes_ctx ctx;
+  ctx.key_dec[0] = 0;
+  rfc4106_set_hash_subkey(&ctx);
+}
+
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
new file mode 100644
index 00000000000..21c1d1ec466
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
+struct crypto_aes_ctx {
+  char key_dec[128];
+};
+
+int rfc4106_set_hash_subkey_hash_subkey;
+
+void __write_overflow(void)__attribute__((__error__("")));
+void __write_overflow1(void);
+void aes_encrypt(void*);
+
+void fortify_panic(const char*) __attribute__((__noreturn__)) ;
+
+char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
+  void *a = &ctx->key_dec[0];
+  unsigned p_size =  __builtin_object_size(a, 0);
+#ifdef __OPTIMIZE__
+  if (p_size < 16) {
+    __write_overflow1();
+    fortify_panic(__func__);
+  }
+  if (p_size < 32) {
+    __write_overflow();
+    fortify_panic(__func__);
+  }
+#endif
+  aes_encrypt(ctx);
+  return ctx->key_dec;
+}
+
+char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
+
+void a(void)
+{
+  struct crypto_aes_ctx ctx;
+  rfc4106_set_hash_subkey(&ctx);
+}
+void b(void)
+{
+  struct crypto_aes_ctx ctx;
+  ctx.key_dec[0] = 0;
+  rfc4106_set_hash_subkey(&ctx);
+}
+
+/* This testcase should still split out one of the above basic blocks dealing
+   with __write_overflow. */
+/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
-- 
2.17.1

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Richard Biener]

On Wed, Nov 17, 2021 at 7:25 AM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> The Linux kernel started to fail compile when the jump threader was improved
> (r12-2591-g2e96b5f14e4025691). This failure was due to the IPA splitting code
> decided now to split off the basic block which contained two functions,
> one of those functions included the error attribute on them.  This patch fixes
> the problem by disallowing basic blocks from being split which contain functions
> that have either the error or warning attribute on them.
>
> The two new testcases are to make sure we still split the function for other
> places if we reject the one case.

Hmm, it's only problematic if the immediate(?) controlling condition of the
error/warning annotated call is not in the split part, no?  Interestingly
we for example don't avoid splitting away __builtin_constant_p either.

Richard.

> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
>         PR tree-optimization/101941
>
> gcc/ChangeLog:
>
>         * ipa-split.c (visit_bb): Disallow function calls where
>         the function has either error or warning attribute.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.c-torture/compile/pr101941-1.c: New test.
>         * gcc.dg/tree-ssa/pr101941-1.c: New test.
> ---
>  gcc/ipa-split.c                               | 12 ++++-
>  .../gcc.c-torture/compile/pr101941-1.c        | 44 +++++++++++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c    | 48 +++++++++++++++++++
>  3 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
>
> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> index c68577d04a9..070e894ef31 100644
> --- a/gcc/ipa-split.c
> +++ b/gcc/ipa-split.c
> @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
>        gimple *stmt = gsi_stmt (bsi);
>        tree op;
>        ssa_op_iter iter;
> -      tree decl;
> +      tree decl = NULL_TREE;
>
>        if (is_gimple_debug (stmt))
>         continue;
> @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
>             break;
>           }
>
> +      /* If a function call and that function has either the
> +        warning or error attribute on it, don't split.  */
> +      if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
> +                  || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
> +       {
> +         if (dump_file && (dump_flags & TDF_DETAILS))
> +           fprintf (dump_file, "Cannot split: warning or error attribute.\n");
> +         can_split = false;
> +       }
> +
>        FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
>         bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
>        FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> new file mode 100644
> index 00000000000..ab3bbea8ed7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> @@ -0,0 +1,44 @@
> +/* { dg-additional-options "-fconserve-stack" } */
> +struct crypto_aes_ctx {
> +  char key_dec[128];
> +};
> +
> +int rfc4106_set_hash_subkey_hash_subkey;
> +
> +void __write_overflow(void)__attribute__((__error__("")));
> +void __write_overflow1(void);
> +void aes_encrypt(void*);
> +
> +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> +
> +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> +  void *a = &ctx->key_dec[0];
> +  unsigned p_size =  __builtin_object_size(a, 0);
> +#ifdef __OPTIMIZE__
> +  if (p_size < 16) {
> +    __write_overflow1();
> +    fortify_panic(__func__);
> +  }
> +  if (p_size < 32) {
> +    __write_overflow();
> +    fortify_panic(__func__);
> +  }
> +#endif
> +  aes_encrypt(ctx);
> +  return ctx->key_dec;
> +}
> +
> +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> +
> +void a(void)
> +{
> +  struct crypto_aes_ctx ctx;
> +  rfc4106_set_hash_subkey(&ctx);
> +}
> +void b(void)
> +{
> +  struct crypto_aes_ctx ctx;
> +  ctx.key_dec[0] = 0;
> +  rfc4106_set_hash_subkey(&ctx);
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> new file mode 100644
> index 00000000000..21c1d1ec466
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> @@ -0,0 +1,48 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
> +struct crypto_aes_ctx {
> +  char key_dec[128];
> +};
> +
> +int rfc4106_set_hash_subkey_hash_subkey;
> +
> +void __write_overflow(void)__attribute__((__error__("")));
> +void __write_overflow1(void);
> +void aes_encrypt(void*);
> +
> +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> +
> +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> +  void *a = &ctx->key_dec[0];
> +  unsigned p_size =  __builtin_object_size(a, 0);
> +#ifdef __OPTIMIZE__
> +  if (p_size < 16) {
> +    __write_overflow1();
> +    fortify_panic(__func__);
> +  }
> +  if (p_size < 32) {
> +    __write_overflow();
> +    fortify_panic(__func__);
> +  }
> +#endif
> +  aes_encrypt(ctx);
> +  return ctx->key_dec;
> +}
> +
> +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> +
> +void a(void)
> +{
> +  struct crypto_aes_ctx ctx;
> +  rfc4106_set_hash_subkey(&ctx);
> +}
> +void b(void)
> +{
> +  struct crypto_aes_ctx ctx;
> +  ctx.key_dec[0] = 0;
> +  rfc4106_set_hash_subkey(&ctx);
> +}
> +
> +/* This testcase should still split out one of the above basic blocks dealing
> +   with __write_overflow. */
> +/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
> --
> 2.17.1
>

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Andrew Pinski]

On Fri, Nov 19, 2021 at 2:16 AM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Wed, Nov 17, 2021 at 7:25 AM apinski--- via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > The Linux kernel started to fail compile when the jump threader was improved
> > (r12-2591-g2e96b5f14e4025691). This failure was due to the IPA splitting code
> > decided now to split off the basic block which contained two functions,
> > one of those functions included the error attribute on them.  This patch fixes
> > the problem by disallowing basic blocks from being split which contain functions
> > that have either the error or warning attribute on them.
> >
> > The two new testcases are to make sure we still split the function for other
> > places if we reject the one case.
>
> Hmm, it's only problematic if the immediate(?) controlling condition of the
> error/warning annotated call is not in the split part, no?
No, if we have something like:
  if (p_size < 32) {
    if (ctx != 0) {
      __write_overflow();
   }
    fortify_panic(__func__);
 }
We would still run into the problem if we just disable the splitting
for the innermost bb and split off the 3 other bb's
I have a testcase where the above would fail if we decide only to make
sure the split point is not after ctx !=0 check.

> Interestingly we for example don't avoid splitting away __builtin_constant_p either.

__builtin_constant_p is handled a different way already; in
check_forbidden_calls we set forbidden_dominators to include the bb
where the builtin_constant_p would have been true.
And then when we consider the split, we reject if the entry point
would have been dominated by one of those bbs.
This was PR49642.


Thanks,
Andrew Pinski


>
> Richard.
>
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> >         PR tree-optimization/101941
> >
> > gcc/ChangeLog:
> >
> >         * ipa-split.c (visit_bb): Disallow function calls where
> >         the function has either error or warning attribute.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.c-torture/compile/pr101941-1.c: New test.
> >         * gcc.dg/tree-ssa/pr101941-1.c: New test.
> > ---
> >  gcc/ipa-split.c                               | 12 ++++-
> >  .../gcc.c-torture/compile/pr101941-1.c        | 44 +++++++++++++++++
> >  gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c    | 48 +++++++++++++++++++
> >  3 files changed, 103 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> >
> > diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> > index c68577d04a9..070e894ef31 100644
> > --- a/gcc/ipa-split.c
> > +++ b/gcc/ipa-split.c
> > @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
> >        gimple *stmt = gsi_stmt (bsi);
> >        tree op;
> >        ssa_op_iter iter;
> > -      tree decl;
> > +      tree decl = NULL_TREE;
> >
> >        if (is_gimple_debug (stmt))
> >         continue;
> > @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
> >             break;
> >           }
> >
> > +      /* If a function call and that function has either the
> > +        warning or error attribute on it, don't split.  */
> > +      if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
> > +                  || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
> > +       {
> > +         if (dump_file && (dump_flags & TDF_DETAILS))
> > +           fprintf (dump_file, "Cannot split: warning or error attribute.\n");
> > +         can_split = false;
> > +       }
> > +
> >        FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
> >         bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
> >        FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > new file mode 100644
> > index 00000000000..ab3bbea8ed7
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > @@ -0,0 +1,44 @@
> > +/* { dg-additional-options "-fconserve-stack" } */
> > +struct crypto_aes_ctx {
> > +  char key_dec[128];
> > +};
> > +
> > +int rfc4106_set_hash_subkey_hash_subkey;
> > +
> > +void __write_overflow(void)__attribute__((__error__("")));
> > +void __write_overflow1(void);
> > +void aes_encrypt(void*);
> > +
> > +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> > +
> > +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> > +  void *a = &ctx->key_dec[0];
> > +  unsigned p_size =  __builtin_object_size(a, 0);
> > +#ifdef __OPTIMIZE__
> > +  if (p_size < 16) {
> > +    __write_overflow1();
> > +    fortify_panic(__func__);
> > +  }
> > +  if (p_size < 32) {
> > +    __write_overflow();
> > +    fortify_panic(__func__);
> > +  }
> > +#endif
> > +  aes_encrypt(ctx);
> > +  return ctx->key_dec;
> > +}
> > +
> > +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> > +
> > +void a(void)
> > +{
> > +  struct crypto_aes_ctx ctx;
> > +  rfc4106_set_hash_subkey(&ctx);
> > +}
> > +void b(void)
> > +{
> > +  struct crypto_aes_ctx ctx;
> > +  ctx.key_dec[0] = 0;
> > +  rfc4106_set_hash_subkey(&ctx);
> > +}
> > +
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > new file mode 100644
> > index 00000000000..21c1d1ec466
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > @@ -0,0 +1,48 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
> > +struct crypto_aes_ctx {
> > +  char key_dec[128];
> > +};
> > +
> > +int rfc4106_set_hash_subkey_hash_subkey;
> > +
> > +void __write_overflow(void)__attribute__((__error__("")));
> > +void __write_overflow1(void);
> > +void aes_encrypt(void*);
> > +
> > +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> > +
> > +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> > +  void *a = &ctx->key_dec[0];
> > +  unsigned p_size =  __builtin_object_size(a, 0);
> > +#ifdef __OPTIMIZE__
> > +  if (p_size < 16) {
> > +    __write_overflow1();
> > +    fortify_panic(__func__);
> > +  }
> > +  if (p_size < 32) {
> > +    __write_overflow();
> > +    fortify_panic(__func__);
> > +  }
> > +#endif
> > +  aes_encrypt(ctx);
> > +  return ctx->key_dec;
> > +}
> > +
> > +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> > +
> > +void a(void)
> > +{
> > +  struct crypto_aes_ctx ctx;
> > +  rfc4106_set_hash_subkey(&ctx);
> > +}
> > +void b(void)
> > +{
> > +  struct crypto_aes_ctx ctx;
> > +  ctx.key_dec[0] = 0;
> > +  rfc4106_set_hash_subkey(&ctx);
> > +}
> > +
> > +/* This testcase should still split out one of the above basic blocks dealing
> > +   with __write_overflow. */
> > +/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
> > --
> > 2.17.1
> >

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Richard Biener]

On Fri, Nov 19, 2021 at 12:50 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Fri, Nov 19, 2021 at 2:16 AM Richard Biener via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Wed, Nov 17, 2021 at 7:25 AM apinski--- via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > From: Andrew Pinski <apinski@marvell.com>
> > >
> > > The Linux kernel started to fail compile when the jump threader was improved
> > > (r12-2591-g2e96b5f14e4025691). This failure was due to the IPA splitting code
> > > decided now to split off the basic block which contained two functions,
> > > one of those functions included the error attribute on them.  This patch fixes
> > > the problem by disallowing basic blocks from being split which contain functions
> > > that have either the error or warning attribute on them.
> > >
> > > The two new testcases are to make sure we still split the function for other
> > > places if we reject the one case.
> >
> > Hmm, it's only problematic if the immediate(?) controlling condition of the
> > error/warning annotated call is not in the split part, no?
> No, if we have something like:
>   if (p_size < 32) {
>     if (ctx != 0) {
>       __write_overflow();
>    }
>     fortify_panic(__func__);
>  }
> We would still run into the problem if we just disable the splitting
> for the innermost bb and split off the 3 other bb's
> I have a testcase where the above would fail if we decide only to make
> sure the split point is not after ctx !=0 check.
>
> > Interestingly we for example don't avoid splitting away __builtin_constant_p either.
>
> __builtin_constant_p is handled a different way already; in
> check_forbidden_calls we set forbidden_dominators to include the bb
> where the builtin_constant_p would have been true.
> And then when we consider the split, we reject if the entry point
> would have been dominated by one of those bbs.
> This was PR49642.

I see.  So how's error/warning different - don't we want to forbit split points
that dominate those calls itself?

>
>
> Thanks,
> Andrew Pinski
>
>
> >
> > Richard.
> >
> > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > >
> > >         PR tree-optimization/101941
> > >
> > > gcc/ChangeLog:
> > >
> > >         * ipa-split.c (visit_bb): Disallow function calls where
> > >         the function has either error or warning attribute.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.c-torture/compile/pr101941-1.c: New test.
> > >         * gcc.dg/tree-ssa/pr101941-1.c: New test.
> > > ---
> > >  gcc/ipa-split.c                               | 12 ++++-
> > >  .../gcc.c-torture/compile/pr101941-1.c        | 44 +++++++++++++++++
> > >  gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c    | 48 +++++++++++++++++++
> > >  3 files changed, 103 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > >
> > > diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> > > index c68577d04a9..070e894ef31 100644
> > > --- a/gcc/ipa-split.c
> > > +++ b/gcc/ipa-split.c
> > > @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
> > >        gimple *stmt = gsi_stmt (bsi);
> > >        tree op;
> > >        ssa_op_iter iter;
> > > -      tree decl;
> > > +      tree decl = NULL_TREE;
> > >
> > >        if (is_gimple_debug (stmt))
> > >         continue;
> > > @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
> > >             break;
> > >           }
> > >
> > > +      /* If a function call and that function has either the
> > > +        warning or error attribute on it, don't split.  */
> > > +      if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
> > > +                  || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
> > > +       {
> > > +         if (dump_file && (dump_flags & TDF_DETAILS))
> > > +           fprintf (dump_file, "Cannot split: warning or error attribute.\n");
> > > +         can_split = false;
> > > +       }
> > > +
> > >        FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
> > >         bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
> > >        FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> > > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > > new file mode 100644
> > > index 00000000000..ab3bbea8ed7
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > > @@ -0,0 +1,44 @@
> > > +/* { dg-additional-options "-fconserve-stack" } */
> > > +struct crypto_aes_ctx {
> > > +  char key_dec[128];
> > > +};
> > > +
> > > +int rfc4106_set_hash_subkey_hash_subkey;
> > > +
> > > +void __write_overflow(void)__attribute__((__error__("")));
> > > +void __write_overflow1(void);
> > > +void aes_encrypt(void*);
> > > +
> > > +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> > > +
> > > +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> > > +  void *a = &ctx->key_dec[0];
> > > +  unsigned p_size =  __builtin_object_size(a, 0);
> > > +#ifdef __OPTIMIZE__
> > > +  if (p_size < 16) {
> > > +    __write_overflow1();
> > > +    fortify_panic(__func__);
> > > +  }
> > > +  if (p_size < 32) {
> > > +    __write_overflow();
> > > +    fortify_panic(__func__);
> > > +  }
> > > +#endif
> > > +  aes_encrypt(ctx);
> > > +  return ctx->key_dec;
> > > +}
> > > +
> > > +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> > > +
> > > +void a(void)
> > > +{
> > > +  struct crypto_aes_ctx ctx;
> > > +  rfc4106_set_hash_subkey(&ctx);
> > > +}
> > > +void b(void)
> > > +{
> > > +  struct crypto_aes_ctx ctx;
> > > +  ctx.key_dec[0] = 0;
> > > +  rfc4106_set_hash_subkey(&ctx);
> > > +}
> > > +
> > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > > new file mode 100644
> > > index 00000000000..21c1d1ec466
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > > @@ -0,0 +1,48 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
> > > +struct crypto_aes_ctx {
> > > +  char key_dec[128];
> > > +};
> > > +
> > > +int rfc4106_set_hash_subkey_hash_subkey;
> > > +
> > > +void __write_overflow(void)__attribute__((__error__("")));
> > > +void __write_overflow1(void);
> > > +void aes_encrypt(void*);
> > > +
> > > +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> > > +
> > > +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> > > +  void *a = &ctx->key_dec[0];
> > > +  unsigned p_size =  __builtin_object_size(a, 0);
> > > +#ifdef __OPTIMIZE__
> > > +  if (p_size < 16) {
> > > +    __write_overflow1();
> > > +    fortify_panic(__func__);
> > > +  }
> > > +  if (p_size < 32) {
> > > +    __write_overflow();
> > > +    fortify_panic(__func__);
> > > +  }
> > > +#endif
> > > +  aes_encrypt(ctx);
> > > +  return ctx->key_dec;
> > > +}
> > > +
> > > +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> > > +
> > > +void a(void)
> > > +{
> > > +  struct crypto_aes_ctx ctx;
> > > +  rfc4106_set_hash_subkey(&ctx);
> > > +}
> > > +void b(void)
> > > +{
> > > +  struct crypto_aes_ctx ctx;
> > > +  ctx.key_dec[0] = 0;
> > > +  rfc4106_set_hash_subkey(&ctx);
> > > +}
> > > +
> > > +/* This testcase should still split out one of the above basic blocks dealing
> > > +   with __write_overflow. */
> > > +/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
> > > --
> > > 2.17.1
> > >

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Martin Liška]

PING^1

May I please ping this so that we can can Linux kernel as soon as possible?
We would benefit from that for GCC 12.1.0 release.

Thanks,
Martin

On 11/19/21 14:07, Richard Biener via Gcc-patches wrote:
> On Fri, Nov 19, 2021 at 12:50 PM Andrew Pinski <pinskia@gmail.com> wrote:
>>
>> On Fri, Nov 19, 2021 at 2:16 AM Richard Biener via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> On Wed, Nov 17, 2021 at 7:25 AM apinski--- via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> From: Andrew Pinski <apinski@marvell.com>
>>>>
>>>> The Linux kernel started to fail compile when the jump threader was improved
>>>> (r12-2591-g2e96b5f14e4025691). This failure was due to the IPA splitting code
>>>> decided now to split off the basic block which contained two functions,
>>>> one of those functions included the error attribute on them.  This patch fixes
>>>> the problem by disallowing basic blocks from being split which contain functions
>>>> that have either the error or warning attribute on them.
>>>>
>>>> The two new testcases are to make sure we still split the function for other
>>>> places if we reject the one case.
>>>
>>> Hmm, it's only problematic if the immediate(?) controlling condition of the
>>> error/warning annotated call is not in the split part, no?
>> No, if we have something like:
>>    if (p_size < 32) {
>>      if (ctx != 0) {
>>        __write_overflow();
>>     }
>>      fortify_panic(__func__);
>>   }
>> We would still run into the problem if we just disable the splitting
>> for the innermost bb and split off the 3 other bb's
>> I have a testcase where the above would fail if we decide only to make
>> sure the split point is not after ctx !=0 check.
>>
>>> Interestingly we for example don't avoid splitting away __builtin_constant_p either.
>>
>> __builtin_constant_p is handled a different way already; in
>> check_forbidden_calls we set forbidden_dominators to include the bb
>> where the builtin_constant_p would have been true.
>> And then when we consider the split, we reject if the entry point
>> would have been dominated by one of those bbs.
>> This was PR49642.
> 
> I see.  So how's error/warning different - don't we want to forbit split points
> that dominate those calls itself?
> 
>>
>>
>> Thanks,
>> Andrew Pinski
>>
>>
>>>
>>> Richard.
>>>
>>>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>>>
>>>>          PR tree-optimization/101941
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>          * ipa-split.c (visit_bb): Disallow function calls where
>>>>          the function has either error or warning attribute.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>          * gcc.c-torture/compile/pr101941-1.c: New test.
>>>>          * gcc.dg/tree-ssa/pr101941-1.c: New test.
>>>> ---
>>>>   gcc/ipa-split.c                               | 12 ++++-
>>>>   .../gcc.c-torture/compile/pr101941-1.c        | 44 +++++++++++++++++
>>>>   gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c    | 48 +++++++++++++++++++
>>>>   3 files changed, 103 insertions(+), 1 deletion(-)
>>>>   create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
>>>>   create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
>>>>
>>>> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
>>>> index c68577d04a9..070e894ef31 100644
>>>> --- a/gcc/ipa-split.c
>>>> +++ b/gcc/ipa-split.c
>>>> @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
>>>>         gimple *stmt = gsi_stmt (bsi);
>>>>         tree op;
>>>>         ssa_op_iter iter;
>>>> -      tree decl;
>>>> +      tree decl = NULL_TREE;
>>>>
>>>>         if (is_gimple_debug (stmt))
>>>>          continue;
>>>> @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
>>>>              break;
>>>>            }
>>>>
>>>> +      /* If a function call and that function has either the
>>>> +        warning or error attribute on it, don't split.  */
>>>> +      if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
>>>> +                  || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
>>>> +       {
>>>> +         if (dump_file && (dump_flags & TDF_DETAILS))
>>>> +           fprintf (dump_file, "Cannot split: warning or error attribute.\n");
>>>> +         can_split = false;
>>>> +       }
>>>> +
>>>>         FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
>>>>          bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
>>>>         FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
>>>> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
>>>> new file mode 100644
>>>> index 00000000000..ab3bbea8ed7
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
>>>> @@ -0,0 +1,44 @@
>>>> +/* { dg-additional-options "-fconserve-stack" } */
>>>> +struct crypto_aes_ctx {
>>>> +  char key_dec[128];
>>>> +};
>>>> +
>>>> +int rfc4106_set_hash_subkey_hash_subkey;
>>>> +
>>>> +void __write_overflow(void)__attribute__((__error__("")));
>>>> +void __write_overflow1(void);
>>>> +void aes_encrypt(void*);
>>>> +
>>>> +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
>>>> +
>>>> +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
>>>> +  void *a = &ctx->key_dec[0];
>>>> +  unsigned p_size =  __builtin_object_size(a, 0);
>>>> +#ifdef __OPTIMIZE__
>>>> +  if (p_size < 16) {
>>>> +    __write_overflow1();
>>>> +    fortify_panic(__func__);
>>>> +  }
>>>> +  if (p_size < 32) {
>>>> +    __write_overflow();
>>>> +    fortify_panic(__func__);
>>>> +  }
>>>> +#endif
>>>> +  aes_encrypt(ctx);
>>>> +  return ctx->key_dec;
>>>> +}
>>>> +
>>>> +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
>>>> +
>>>> +void a(void)
>>>> +{
>>>> +  struct crypto_aes_ctx ctx;
>>>> +  rfc4106_set_hash_subkey(&ctx);
>>>> +}
>>>> +void b(void)
>>>> +{
>>>> +  struct crypto_aes_ctx ctx;
>>>> +  ctx.key_dec[0] = 0;
>>>> +  rfc4106_set_hash_subkey(&ctx);
>>>> +}
>>>> +
>>>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
>>>> new file mode 100644
>>>> index 00000000000..21c1d1ec466
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
>>>> @@ -0,0 +1,48 @@
>>>> +/* { dg-do compile } */
>>>> +/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
>>>> +struct crypto_aes_ctx {
>>>> +  char key_dec[128];
>>>> +};
>>>> +
>>>> +int rfc4106_set_hash_subkey_hash_subkey;
>>>> +
>>>> +void __write_overflow(void)__attribute__((__error__("")));
>>>> +void __write_overflow1(void);
>>>> +void aes_encrypt(void*);
>>>> +
>>>> +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
>>>> +
>>>> +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
>>>> +  void *a = &ctx->key_dec[0];
>>>> +  unsigned p_size =  __builtin_object_size(a, 0);
>>>> +#ifdef __OPTIMIZE__
>>>> +  if (p_size < 16) {
>>>> +    __write_overflow1();
>>>> +    fortify_panic(__func__);
>>>> +  }
>>>> +  if (p_size < 32) {
>>>> +    __write_overflow();
>>>> +    fortify_panic(__func__);
>>>> +  }
>>>> +#endif
>>>> +  aes_encrypt(ctx);
>>>> +  return ctx->key_dec;
>>>> +}
>>>> +
>>>> +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
>>>> +
>>>> +void a(void)
>>>> +{
>>>> +  struct crypto_aes_ctx ctx;
>>>> +  rfc4106_set_hash_subkey(&ctx);
>>>> +}
>>>> +void b(void)
>>>> +{
>>>> +  struct crypto_aes_ctx ctx;
>>>> +  ctx.key_dec[0] = 0;
>>>> +  rfc4106_set_hash_subkey(&ctx);
>>>> +}
>>>> +
>>>> +/* This testcase should still split out one of the above basic blocks dealing
>>>> +   with __write_overflow. */
>>>> +/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
>>>> --
>>>> 2.17.1
>>>>

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Jan Hubicka]

> > > > > --- a/gcc/ipa-split.c
> > > > > +++ b/gcc/ipa-split.c
> > > > > @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
> > > > >         gimple *stmt = gsi_stmt (bsi);
> > > > >         tree op;
> > > > >         ssa_op_iter iter;
> > > > > -      tree decl;
> > > > > +      tree decl = NULL_TREE;
> > > > > 
> > > > >         if (is_gimple_debug (stmt))
> > > > >          continue;
> > > > > @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
Decl is initialized in
      if (gimple_code (stmt) == GIMPLE_CALL
          && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
          && fndecl_built_in_p (decl, BUILT_IN_NORMAL))

I think this is confusing. I would change it to
      if (gimple_code (stmt) == GIMPLE_CALL
          && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
	{
          if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
	    ... existing code ...
	  if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
		       || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
	    ... your code ...
	}
OK with that change.
Honza
> > > > >              break;
> > > > >            }
> > > > > 
> > > > > +      /* If a function call and that function has either the
> > > > > +        warning or error attribute on it, don't split.  */
> > > > > +      if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
> > > > > +                  || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
> > > > > +       {
> > > > > +         if (dump_file && (dump_flags & TDF_DETAILS))
> > > > > +           fprintf (dump_file, "Cannot split: warning or error attribute.\n");
> > > > > +         can_split = false;
> > > > > +       }
> > > > > +
> > > > >         FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
> > > > >          bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
> > > > >         FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> > > > > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > > > > new file mode 100644
> > > > > index 00000000000..ab3bbea8ed7
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
> > > > > @@ -0,0 +1,44 @@
> > > > > +/* { dg-additional-options "-fconserve-stack" } */
> > > > > +struct crypto_aes_ctx {
> > > > > +  char key_dec[128];
> > > > > +};
> > > > > +
> > > > > +int rfc4106_set_hash_subkey_hash_subkey;
> > > > > +
> > > > > +void __write_overflow(void)__attribute__((__error__("")));
> > > > > +void __write_overflow1(void);
> > > > > +void aes_encrypt(void*);
> > > > > +
> > > > > +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> > > > > +
> > > > > +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> > > > > +  void *a = &ctx->key_dec[0];
> > > > > +  unsigned p_size =  __builtin_object_size(a, 0);
> > > > > +#ifdef __OPTIMIZE__
> > > > > +  if (p_size < 16) {
> > > > > +    __write_overflow1();
> > > > > +    fortify_panic(__func__);
> > > > > +  }
> > > > > +  if (p_size < 32) {
> > > > > +    __write_overflow();
> > > > > +    fortify_panic(__func__);
> > > > > +  }
> > > > > +#endif
> > > > > +  aes_encrypt(ctx);
> > > > > +  return ctx->key_dec;
> > > > > +}
> > > > > +
> > > > > +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> > > > > +
> > > > > +void a(void)
> > > > > +{
> > > > > +  struct crypto_aes_ctx ctx;
> > > > > +  rfc4106_set_hash_subkey(&ctx);
> > > > > +}
> > > > > +void b(void)
> > > > > +{
> > > > > +  struct crypto_aes_ctx ctx;
> > > > > +  ctx.key_dec[0] = 0;
> > > > > +  rfc4106_set_hash_subkey(&ctx);
> > > > > +}
> > > > > +
> > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > > > > new file mode 100644
> > > > > index 00000000000..21c1d1ec466
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
> > > > > @@ -0,0 +1,48 @@
> > > > > +/* { dg-do compile } */
> > > > > +/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
> > > > > +struct crypto_aes_ctx {
> > > > > +  char key_dec[128];
> > > > > +};
> > > > > +
> > > > > +int rfc4106_set_hash_subkey_hash_subkey;
> > > > > +
> > > > > +void __write_overflow(void)__attribute__((__error__("")));
> > > > > +void __write_overflow1(void);
> > > > > +void aes_encrypt(void*);
> > > > > +
> > > > > +void fortify_panic(const char*) __attribute__((__noreturn__)) ;
> > > > > +
> > > > > +char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
> > > > > +  void *a = &ctx->key_dec[0];
> > > > > +  unsigned p_size =  __builtin_object_size(a, 0);
> > > > > +#ifdef __OPTIMIZE__
> > > > > +  if (p_size < 16) {
> > > > > +    __write_overflow1();
> > > > > +    fortify_panic(__func__);
> > > > > +  }
> > > > > +  if (p_size < 32) {
> > > > > +    __write_overflow();
> > > > > +    fortify_panic(__func__);
> > > > > +  }
> > > > > +#endif
> > > > > +  aes_encrypt(ctx);
> > > > > +  return ctx->key_dec;
> > > > > +}
> > > > > +
> > > > > +char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
> > > > > +
> > > > > +void a(void)
> > > > > +{
> > > > > +  struct crypto_aes_ctx ctx;
> > > > > +  rfc4106_set_hash_subkey(&ctx);
> > > > > +}
> > > > > +void b(void)
> > > > > +{
> > > > > +  struct crypto_aes_ctx ctx;
> > > > > +  ctx.key_dec[0] = 0;
> > > > > +  rfc4106_set_hash_subkey(&ctx);
> > > > > +}
> > > > > +
> > > > > +/* This testcase should still split out one of the above basic blocks dealing
> > > > > +   with __write_overflow. */
> > > > > +/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
> > > > > --
> > > > > 2.17.1
> > > > > 
> 

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Jakub Jelinek]

On Fri, Jan 14, 2022 at 09:41:35AM +0100, Jan Hubicka via Gcc-patches wrote:
> > > > > > --- a/gcc/ipa-split.c
> > > > > > +++ b/gcc/ipa-split.c
> > > > > > @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
> > > > > >         gimple *stmt = gsi_stmt (bsi);
> > > > > >         tree op;
> > > > > >         ssa_op_iter iter;
> > > > > > -      tree decl;
> > > > > > +      tree decl = NULL_TREE;
> > > > > > 
> > > > > >         if (is_gimple_debug (stmt))
> > > > > >          continue;
> > > > > > @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
> Decl is initialized in
>       if (gimple_code (stmt) == GIMPLE_CALL
>           && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
>           && fndecl_built_in_p (decl, BUILT_IN_NORMAL))
> 
> I think this is confusing. I would change it to
>       if (gimple_code (stmt) == GIMPLE_CALL
>           && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
> 	{
>           if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
> 	    ... existing code ...
> 	  if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
> 		       || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
> 	    ... your code ...
> 	}
> OK with that change.

Perhaps even
      /* Check builtins that prevent splitting.  */
      if (gimple_code (stmt) == GIMPLE_CALL)
	if (tree decl = gimple_call_fndecl (stmt))
	  {
	    if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
	      ...
	    if (lookup_attribute || lookup_attribute)
	      ...
	  }
?          

	Jakub

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Andrew Pinski]

On Mon, Jan 17, 2022 at 10:36 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Fri, Jan 14, 2022 at 09:41:35AM +0100, Jan Hubicka via Gcc-patches wrote:
> > > > > > > --- a/gcc/ipa-split.c
> > > > > > > +++ b/gcc/ipa-split.c
> > > > > > > @@ -873,7 +873,7 @@ visit_bb (basic_block bb, basic_block return_bb,
> > > > > > >         gimple *stmt = gsi_stmt (bsi);
> > > > > > >         tree op;
> > > > > > >         ssa_op_iter iter;
> > > > > > > -      tree decl;
> > > > > > > +      tree decl = NULL_TREE;
> > > > > > >
> > > > > > >         if (is_gimple_debug (stmt))
> > > > > > >          continue;
> > > > > > > @@ -927,6 +927,16 @@ visit_bb (basic_block bb, basic_block return_bb,
> > Decl is initialized in
> >       if (gimple_code (stmt) == GIMPLE_CALL
> >           && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
> >           && fndecl_built_in_p (decl, BUILT_IN_NORMAL))
> >
> > I think this is confusing. I would change it to
> >       if (gimple_code (stmt) == GIMPLE_CALL
> >           && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
> >       {
> >           if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
> >           ... existing code ...
> >         if (decl && (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
> >                      || lookup_attribute ("error", DECL_ATTRIBUTES (decl))))
> >           ... your code ...
> >       }
> > OK with that change.
>
> Perhaps even
>       /* Check builtins that prevent splitting.  */
>       if (gimple_code (stmt) == GIMPLE_CALL)
>         if (tree decl = gimple_call_fndecl (stmt))
>           {
>             if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
>               ...
>             if (lookup_attribute || lookup_attribute)
>               ...
>           }
> ?

Yes that looks good, I am just finished up the patch and going to run
a full bootstrap/test to make sure I didn't mess up.
I had originally trying not to reindent the code to make it easier to
see what was being added but I think re-indentation is correct after
all.

Thanks
Andrew


>
>         Jakub
>

Re: [PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[Jakub Jelinek]

On Mon, Jan 17, 2022 at 09:08:22PM -0800, Andrew Pinski wrote:
> > Perhaps even
> >       /* Check builtins that prevent splitting.  */
> >       if (gimple_code (stmt) == GIMPLE_CALL)
> >         if (tree decl = gimple_call_fndecl (stmt))
> >           {
> >             if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
> >               ...
> >             if (lookup_attribute || lookup_attribute)
> >               ...
> >           }
> > ?
> 
> Yes that looks good, I am just finished up the patch and going to run
> a full bootstrap/test to make sure I didn't mess up.
> I had originally trying not to reindent the code to make it easier to
> see what was being added but I think re-indentation is correct after
> all.

Here is what I've bootstrapped/regtested successfully on x86_64-linux
and i686-linux.  Note, besides the reformatting I've changed the wording
of the added comment.

2022-01-18  Andrew Pinski  <apinski@marvell.com>

	PR tree-optimization/101941
	* ipa-split.c (visit_bb): Disallow function calls where
	the function has either error or warning attribute.

	* gcc.c-torture/compile/pr101941.c: New test.
	* gcc.dg/tree-ssa/pr101941.c: New test.

--- gcc/ipa-split.c.jj	2022-01-11 23:11:22.664286304 +0100
+++ gcc/ipa-split.c	2022-01-17 19:40:23.837234404 +0100
@@ -873,7 +873,6 @@ visit_bb (basic_block bb, basic_block re
       gimple *stmt = gsi_stmt (bsi);
       tree op;
       ssa_op_iter iter;
-      tree decl;
 
       if (is_gimple_debug (stmt))
 	continue;
@@ -900,31 +899,45 @@ visit_bb (basic_block bb, basic_block re
 	}
 
       /* Check builtins that prevent splitting.  */
-      if (gimple_code (stmt) == GIMPLE_CALL
-	  && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
-	  && fndecl_built_in_p (decl, BUILT_IN_NORMAL))
-	switch (DECL_FUNCTION_CODE (decl))
+      if (gimple_code (stmt) == GIMPLE_CALL)
+	if (tree decl = gimple_call_fndecl (stmt))
 	  {
-	  /* FIXME: once we will allow passing non-parm values to split part,
-	     we need to be sure to handle correct builtin_stack_save and
-	     builtin_stack_restore.  At the moment we are safe; there is no
-	     way to store builtin_stack_save result in non-SSA variable
-	     since all calls to those are compiler generated.  */
-	  case BUILT_IN_APPLY:
-	  case BUILT_IN_APPLY_ARGS:
-	  case BUILT_IN_VA_START:
-	    if (dump_file && (dump_flags & TDF_DETAILS))
-	      fprintf (dump_file,
-		       "Cannot split: builtin_apply and va_start.\n");
-	    can_split = false;
-	    break;
-	  case BUILT_IN_EH_POINTER:
-	    if (dump_file && (dump_flags & TDF_DETAILS))
-	      fprintf (dump_file, "Cannot split: builtin_eh_pointer.\n");
-	    can_split = false;
-	    break;
-	  default:
-	    break;
+	    if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
+	      switch (DECL_FUNCTION_CODE (decl))
+		{
+		  /* FIXME: once we will allow passing non-parm values to
+		     split part, we need to be sure to handle correct
+		     builtin_stack_save and builtin_stack_restore.  At the
+		     moment we are safe; there is no way to store
+		     builtin_stack_save result in non-SSA variable
+		     since all calls to those are compiler generated.  */
+		case BUILT_IN_APPLY:
+		case BUILT_IN_APPLY_ARGS:
+		case BUILT_IN_VA_START:
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    fprintf (dump_file,
+			     "Cannot split: builtin_apply and va_start.\n");
+		  can_split = false;
+		  break;
+		case BUILT_IN_EH_POINTER:
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    fprintf (dump_file, "Cannot split: builtin_eh_pointer.\n");
+		  can_split = false;
+		  break;
+		default:
+		  break;
+		}
+
+	    /* If there is a function call and that function has either the
+	       warning or error attribute on it, don't split.  */
+	    if (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
+		|| lookup_attribute ("error", DECL_ATTRIBUTES (decl)))
+	      {
+		if (dump_file && (dump_flags & TDF_DETAILS))
+		  fprintf (dump_file, "Cannot split: warning or error "
+				      "attribute.\n");
+		can_split = false;
+	      }
 	  }
 
       FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
--- gcc/testsuite/gcc.dg/tree-ssa/pr101941.c.jj	2022-01-17 19:37:01.609066831 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/pr101941.c	2022-01-17 19:37:01.609066831 +0100
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
+struct crypto_aes_ctx {
+  char key_dec[128];
+};
+
+int rfc4106_set_hash_subkey_hash_subkey;
+
+void __write_overflow(void)__attribute__((__error__("")));
+void __write_overflow1(void);
+void aes_encrypt(void*);
+
+void fortify_panic(const char*) __attribute__((__noreturn__)) ;
+
+char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
+  void *a = &ctx->key_dec[0];
+  unsigned p_size =  __builtin_object_size(a, 0);
+#ifdef __OPTIMIZE__
+  if (p_size < 16) {
+    __write_overflow1();
+    fortify_panic(__func__);
+  }
+  if (p_size < 32) {
+    __write_overflow();
+    fortify_panic(__func__);
+  }
+#endif
+  aes_encrypt(ctx);
+  return ctx->key_dec;
+}
+
+char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
+
+void a(void)
+{
+  struct crypto_aes_ctx ctx;
+  rfc4106_set_hash_subkey(&ctx);
+}
+void b(void)
+{
+  struct crypto_aes_ctx ctx;
+  ctx.key_dec[0] = 0;
+  rfc4106_set_hash_subkey(&ctx);
+}
+
+/* This testcase should still split out one of the above basic blocks dealing
+   with __write_overflow. */
+/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
--- gcc/testsuite/gcc.c-torture/compile/pr101941.c.jj	2022-01-17 19:37:01.609066831 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr101941.c	2022-01-17 19:37:01.609066831 +0100
@@ -0,0 +1,43 @@
+/* { dg-additional-options "-fconserve-stack" } */
+struct crypto_aes_ctx {
+  char key_dec[128];
+};
+
+int rfc4106_set_hash_subkey_hash_subkey;
+
+void __write_overflow(void)__attribute__((__error__("")));
+void __write_overflow1(void);
+void aes_encrypt(void*);
+
+void fortify_panic(const char*) __attribute__((__noreturn__)) ;
+
+char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
+  void *a = &ctx->key_dec[0];
+  unsigned p_size =  __builtin_object_size(a, 0);
+#ifdef __OPTIMIZE__
+  if (p_size < 16) {
+    __write_overflow1();
+    fortify_panic(__func__);
+  }
+  if (p_size < 32) {
+    __write_overflow();
+    fortify_panic(__func__);
+  }
+#endif
+  aes_encrypt(ctx);
+  return ctx->key_dec;
+}
+
+char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
+
+void a(void)
+{
+  struct crypto_aes_ctx ctx;
+  rfc4106_set_hash_subkey(&ctx);
+}
+void b(void)
+{
+  struct crypto_aes_ctx ctx;
+  ctx.key_dec[0] = 0;
+  rfc4106_set_hash_subkey(&ctx);
+}


	Jakub

[PATCH] Fix tree-optimization/101941: IPA splitting out function with error attribute

[apinski]

From: Andrew Pinski <apinski@marvell.com>

The Linux kernel started to fail compile when the jump threader was improved
(r12-2591-g2e96b5f14e4025691). This failure was due to the IPA splitting code
decided now to split off the basic block which contained two functions,
one of those functions included the error attribute on them.  This patch fixes
the problem by disallowing basic blocks from being split which contain functions
that have either the error or warning attribute on them.

The two new testcases are to make sure we still split the function for other
places if we reject the one case.

Committed as approved after Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR tree-optimization/101941

gcc/ChangeLog:

	* ipa-split.cc (visit_bb): Disallow function calls where
	the function has either error or warning attribute.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/compile/pr101941-1.c: New test.
	* gcc.dg/tree-ssa/pr101941-1.c: New test.
---
 gcc/ipa-split.cc                              | 74 ++++++++++++-------
 .../gcc.c-torture/compile/pr101941-1.c        | 50 +++++++++++++
 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c    | 53 +++++++++++++
 3 files changed, 149 insertions(+), 28 deletions(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c

diff --git a/gcc/ipa-split.cc b/gcc/ipa-split.cc
index fc6be8eadef..6ca45f3b745 100644
--- a/gcc/ipa-split.cc
+++ b/gcc/ipa-split.cc
@@ -873,7 +873,6 @@ visit_bb (basic_block bb, basic_block return_bb,
       gimple *stmt = gsi_stmt (bsi);
       tree op;
       ssa_op_iter iter;
-      tree decl;
 
       if (is_gimple_debug (stmt))
 	continue;
@@ -899,33 +898,52 @@ visit_bb (basic_block bb, basic_block return_bb,
 	  can_split = false;
 	}
 
-      /* Check builtins that prevent splitting.  */
-      if (gimple_code (stmt) == GIMPLE_CALL
-	  && (decl = gimple_call_fndecl (stmt)) != NULL_TREE
-	  && fndecl_built_in_p (decl, BUILT_IN_NORMAL))
-	switch (DECL_FUNCTION_CODE (decl))
-	  {
-	  /* FIXME: once we will allow passing non-parm values to split part,
-	     we need to be sure to handle correct builtin_stack_save and
-	     builtin_stack_restore.  At the moment we are safe; there is no
-	     way to store builtin_stack_save result in non-SSA variable
-	     since all calls to those are compiler generated.  */
-	  case BUILT_IN_APPLY:
-	  case BUILT_IN_APPLY_ARGS:
-	  case BUILT_IN_VA_START:
-	    if (dump_file && (dump_flags & TDF_DETAILS))
-	      fprintf (dump_file,
-		       "Cannot split: builtin_apply and va_start.\n");
-	    can_split = false;
-	    break;
-	  case BUILT_IN_EH_POINTER:
-	    if (dump_file && (dump_flags & TDF_DETAILS))
-	      fprintf (dump_file, "Cannot split: builtin_eh_pointer.\n");
-	    can_split = false;
-	    break;
-	  default:
-	    break;
-	  }
+      /* Check calls that would prevent splitting.  */
+      if (gimple_code (stmt) == GIMPLE_CALL)
+	{
+	  if (tree decl = gimple_call_fndecl (stmt))
+	    {
+	      /* Check builtins that would prevent splitting.  */
+	      if (fndecl_built_in_p (decl, BUILT_IN_NORMAL))
+		switch (DECL_FUNCTION_CODE (decl))
+		  {
+		  /* FIXME: once we will allow passing non-parm values to
+		     split part, we need to be sure to handle correct
+		     builtin_stack_save and builtin_stack_restore.  At the
+		     moment we are safe; there is no way to store
+		     builtin_stack_save result in non-SSA variable since all
+		     calls to those are compiler generated.  */
+		  case BUILT_IN_APPLY:
+		  case BUILT_IN_APPLY_ARGS:
+		  case BUILT_IN_VA_START:
+		    if (dump_file && (dump_flags & TDF_DETAILS))
+		      fprintf (dump_file,
+			       "Cannot split: builtin_apply and va_start.\n");
+		    can_split = false;
+		    break;
+		  case BUILT_IN_EH_POINTER:
+		    if (dump_file && (dump_flags & TDF_DETAILS))
+		      fprintf (dump_file,
+			       "Cannot split: builtin_eh_pointer.\n");
+		    can_split = false;
+		    break;
+		  default:
+		    break;
+		  }
+
+	      /* Calls that function has either the warning or error
+	         attribute on it should not be split off into another
+		 function.  */
+	      if (lookup_attribute ("warning", DECL_ATTRIBUTES (decl))
+		   || lookup_attribute ("error", DECL_ATTRIBUTES (decl)))
+		{
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    fprintf (dump_file,
+			     "Cannot split: warning or error attribute.\n");
+		  can_split = false;
+		}
+	    }
+	}
 
       FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_DEF)
 	bitmap_set_bit (set_ssa_names, SSA_NAME_VERSION (op));
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
new file mode 100644
index 00000000000..0481a3835a6
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr101941-1.c
@@ -0,0 +1,50 @@
+/* { dg-additional-options "-fconserve-stack" } */
+
+/* Check to make sure that if
+   rfc4106_set_hash_subkey gets split,
+   the error function does not gets split away
+   from the if statement conditionalizing it. */
+
+struct crypto_aes_ctx {
+  char key_dec[128];
+};
+
+int rfc4106_set_hash_subkey_hash_subkey;
+
+void __write_overflow(void)__attribute__((__error__("")));
+void __write_overflow1(void);
+void aes_encrypt(void*);
+
+void fortify_panic(const char*) __attribute__((__noreturn__)) ;
+
+char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
+  void *a = &ctx->key_dec[0];
+  unsigned p_size =  __builtin_object_size(a, 0);
+#ifdef __OPTIMIZE__
+  if (p_size < 16) {
+    __write_overflow1();
+    fortify_panic(__func__);
+  }
+  if (p_size < 32) {
+    __write_overflow();
+    fortify_panic(__func__);
+  }
+#endif
+  aes_encrypt(ctx);
+  return ctx->key_dec;
+}
+
+char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
+
+void a(void)
+{
+  struct crypto_aes_ctx ctx;
+  rfc4106_set_hash_subkey(&ctx);
+}
+void b(void)
+{
+  struct crypto_aes_ctx ctx;
+  ctx.key_dec[0] = 0;
+  rfc4106_set_hash_subkey(&ctx);
+}
+
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
new file mode 100644
index 00000000000..a1a342a253a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr101941-1.c
@@ -0,0 +1,53 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fconserve-stack -fdump-tree-optimized" } */
+/* Check to make sure that if
+   rfc4106_set_hash_subkey gets split,
+   the error function does not gets split away
+   from the if statement conditionalizing it.
+   Checking this via the scan tree also. */
+struct crypto_aes_ctx {
+  char key_dec[128];
+};
+
+int rfc4106_set_hash_subkey_hash_subkey;
+
+void __write_overflow(void)__attribute__((__error__("")));
+void __write_overflow1(void);
+void aes_encrypt(void*);
+
+void fortify_panic(const char*) __attribute__((__noreturn__)) ;
+
+char *rfc4106_set_hash_subkey(struct crypto_aes_ctx *ctx) {
+  void *a = &ctx->key_dec[0];
+  unsigned p_size =  __builtin_object_size(a, 0);
+#ifdef __OPTIMIZE__
+  if (p_size < 16) {
+    __write_overflow1();
+    fortify_panic(__func__);
+  }
+  if (p_size < 32) {
+    __write_overflow();
+    fortify_panic(__func__);
+  }
+#endif
+  aes_encrypt(ctx);
+  return ctx->key_dec;
+}
+
+char *(*gg)(struct crypto_aes_ctx *) = rfc4106_set_hash_subkey;
+
+void a(void)
+{
+  struct crypto_aes_ctx ctx;
+  rfc4106_set_hash_subkey(&ctx);
+}
+void b(void)
+{
+  struct crypto_aes_ctx ctx;
+  ctx.key_dec[0] = 0;
+  rfc4106_set_hash_subkey(&ctx);
+}
+
+/* This testcase should still split out one of the above basic blocks dealing
+   with __write_overflow. */
+/* { dg-final { scan-tree-dump-times "Function rfc4106_set_hash_subkey.part" 1 "optimized" } } */
-- 
2.27.0