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

["Martin Liška" <mliska@suse.cz>]

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