On Fri, Nov 19, 2021 at 12:50 PM Andrew Pinski <pins...@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 <apin...@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
> > >
