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

Richard Biener richard.guenther@gmail.com
Fri Nov 19 13:07:33 GMT 2021


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
> > >


More information about the Gcc-patches mailing list