On Fri, Nov 15, 2024 at 3:51 AM Joseph Myers <josmy...@redhat.com> wrote:
>
> Bug 117164 is an ICE on an existing test with -std=gnu23 involving a
> nested function returning a variable-size structure (and I think the
> last bug needing to be resolved before switching to -std=gnu23 as the
> default, as without fixing this would be a clear regression from a
> change in default).
>
> The problem is a GIMPLE verification failure where (after type
> remapping from inlining / cloning) the return type of the function no
> longer exactly matches the type to which it is assigned (these types
> use structural equality, which means GIMPLE verification can't use
> TYPE_CANONICAL and expects an exact match). Specifically, the nested
> function itself is *not* inlined (the -fno-inline-small-functions in
> the original test nested-func-12.c, I think, or the noinline attribute
> in some of my variant tests), but the function containing it is either
> cloned (the --param ipa-cp-eval-threshold=0 in the original test) or
> inlined. (I'm not sure what role -fno-guess-branch-probability plays
> in getting the right situation for the ICE; maybe affecting when
> inlining or cloning is considered profitable?)
>
> There is in fact existing code in tree-nested.cc to prevent inlining
> of a function containing a nested function with variably modified
> *argument* types. I think the same issue of ensuring consistency of
> types means such prevention should also apply for a variably modified
> return type. Furthermore, exactly the same problem applies for
> cloning for other reasons as it does for inlining. Thus, change the
> logic to include variably modified return types for nested functions
> alongside those for arguments of those functions as a reason not to
> inline, and also add the noclone attribute in these cases.
>
> Bootstrapped with no regressions for x86-64-pc-linux-gnu.
OK.
Thanks,
Richard.
> PR c/117164
>
> gcc/
> * tree-nested.cc: Include "attribs.h".
> (check_for_nested_with_variably_modified): Also return true for
> variably modified return type.
> (create_nesting_tree): If check_for_nested_with_variably_modified
> returns true, also add noclone attribute.
>
> gcc/testsuite/
> * gcc.dg/nested-func-13.c, gcc.dg/nested-func-14.c:
> gcc.dg/nested-func-15.c, gcc.dg/nested-func-16.c,
> gcc.dg/nested-func-17.c: New tests.
>
> diff --git a/gcc/testsuite/gcc.dg/nested-func-13.c
> b/gcc/testsuite/gcc.dg/nested-func-13.c
> new file mode 100644
> index 000000000000..697a62354109
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-func-13.c
> @@ -0,0 +1,6 @@
> +/* Test nested-func-12.c with -std=gnu17. */
> +/* { dg-do run } */
> +/* { dg-options "-Ofast --param ipa-cp-eval-threshold=0
> -fno-guess-branch-probability -fno-inline-small-functions -std=gnu17" } */
> +/* { dg-require-effective-target alloca } */
> +
> +#include "nested-func-12.c"
> diff --git a/gcc/testsuite/gcc.dg/nested-func-14.c
> b/gcc/testsuite/gcc.dg/nested-func-14.c
> new file mode 100644
> index 000000000000..05a77bdb1a5a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-func-14.c
> @@ -0,0 +1,6 @@
> +/* Test nested-func-12.c with -std=gnu23. */
> +/* { dg-do run } */
> +/* { dg-options "-Ofast --param ipa-cp-eval-threshold=0
> -fno-guess-branch-probability -fno-inline-small-functions -std=gnu23" } */
> +/* { dg-require-effective-target alloca } */
> +
> +#include "nested-func-12.c"
> diff --git a/gcc/testsuite/gcc.dg/nested-func-15.c
> b/gcc/testsuite/gcc.dg/nested-func-15.c
> new file mode 100644
> index 000000000000..490d69599868
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-func-15.c
> @@ -0,0 +1,29 @@
> +/* Bug 117164: ICE with -std=gnu23 inlining function containing call
> + to non-inlined nested function returning variable-size struct. */
> +/* { dg-do run } */
> +/* { dg-options "-O3 --param ipa-cp-eval-threshold=0
> -fno-guess-branch-probability -fno-inline-small-functions -std=gnu23" } */
> +/* { dg-require-effective-target alloca } */
> +
> +void
> +foo (int n)
> +{
> + struct S { int a[n]; };
> +
> + struct S
> + fn (void)
> + {
> + struct S s;
> + s.a[0] = 42;
> + return s;
> + }
> +
> + fn ();
> + fn ();
> + fn ();
> +}
> +
> +int
> +main (void)
> +{
> + foo (1);
> +}
> diff --git a/gcc/testsuite/gcc.dg/nested-func-16.c
> b/gcc/testsuite/gcc.dg/nested-func-16.c
> new file mode 100644
> index 000000000000..061485727bba
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-func-16.c
> @@ -0,0 +1,27 @@
> +/* Bug 117164: ICE with -std=gnu23 inlining function containing call
> + to non-inlined nested function returning variable-size struct. */
> +/* { dg-do run } */
> +/* { dg-options "-O3 -fno-guess-branch-probability -std=gnu23" } */
> +/* { dg-require-effective-target alloca } */
> +
> +void
> +foo (int n)
> +{
> + struct S { int a[n]; };
> +
> + __attribute__ ((noinline)) struct S
> + fn (void)
> + {
> + struct S s;
> + s.a[0] = 42;
> + return s;
> + }
> +
> + fn ();
> +}
> +
> +int
> +main (void)
> +{
> + foo (1);
> +}
> diff --git a/gcc/testsuite/gcc.dg/nested-func-17.c
> b/gcc/testsuite/gcc.dg/nested-func-17.c
> new file mode 100644
> index 000000000000..3441d79ac0dd
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/nested-func-17.c
> @@ -0,0 +1,7 @@
> +/* Bug 117164: ICE with -std=gnu23 inlining function containing call
> + to non-inlined nested function returning variable-size struct. */
> +/* { dg-do run } */
> +/* { dg-options "-O3 --param ipa-cp-eval-threshold=0
> -fno-guess-branch-probability -std=gnu23" } */
> +/* { dg-require-effective-target alloca } */
> +
> +#include "nested-func-16.c"
> diff --git a/gcc/tree-nested.cc b/gcc/tree-nested.cc
> index a54e72c32370..4f5763634d3f 100644
> --- a/gcc/tree-nested.cc
> +++ b/gcc/tree-nested.cc
> @@ -47,6 +47,7 @@
> #include "tree-nested.h"
> #include "symbol-summary.h"
> #include "symtab-thunks.h"
> +#include "attribs.h"
>
> /* Summary of nested functions. */
> static function_summary <nested_function_info *>
> @@ -908,7 +909,11 @@ walk_all_functions (walk_stmt_fn callback_stmt,
> walk_tree_fn callback_op,
> add a VIEW_CONVERT_EXPR to each such operand for each call to the nested
> function. The former is not practical. The latter would still require
> detecting this case to know when to add the conversions. So, for now at
> - least, we don't inline such an enclosing function.
> + least, we don't inline such an enclosing function. A similar issue
> + applies if the nested function has a variably modified return type, and
> + is not inlined, but the enclosing function is inlined and so the type of
> + the return slot as used in the enclosing function is remapped, so also
> + avoid inlining in that case.
>
> We have to do that check recursively, so here return indicating whether
> FNDECL has such a nested function. ORIG_FN is the function we were
> @@ -929,6 +934,9 @@ check_for_nested_with_variably_modified (tree fndecl,
> tree orig_fndecl)
> for (cgn = first_nested_function (cgn); cgn;
> cgn = next_nested_function (cgn))
> {
> + if (variably_modified_type_p (TREE_TYPE (TREE_TYPE (cgn->decl)),
> + orig_fndecl))
> + return true;
> for (arg = DECL_ARGUMENTS (cgn->decl); arg; arg = DECL_CHAIN (arg))
> if (variably_modified_type_p (TREE_TYPE (arg), orig_fndecl))
> return true;
> @@ -967,7 +975,13 @@ create_nesting_tree (struct cgraph_node *cgn)
> /* See discussion at check_for_nested_with_variably_modified for a
> discussion of why this has to be here. */
> if (check_for_nested_with_variably_modified (info->context, info->context))
> - DECL_UNINLINABLE (info->context) = true;
> + {
> + DECL_UNINLINABLE (info->context) = true;
> + tree attrs = DECL_ATTRIBUTES (info->context);
> + if (lookup_attribute ("noclone", attrs) == NULL)
> + DECL_ATTRIBUTES (info->context)
> + = tree_cons (get_identifier ("noclone"), NULL, attrs);
> + }
>
> return info;
> }
>
> --
> Joseph S. Myers
> josmy...@redhat.com
>
