fortran: Assume there is no cyclic reference with submodule symbols [PR99798]

Hello,

Here is my final patch to fix the ICE of PR99798.
It's maybe overly verbose with comments, but the memory management is
hopefully clarified.
I tested this with a full fortran regression test on x86_64-linux and a
manual check with valgrind on the testcase.
OK for master?

-- 8< --

This prevents a premature release of memory with procedure symbols from
submodules, causing random compiler crashes.

The problem is a fragile detection of cyclic references, which can match
with procedures host-associated from a module in submodules, in cases where it
shouldn't.  The formal namespace is released, and with it the dummy arguments
symbols of the procedure.  But there is no cyclic reference, so the procedure
symbol itself is not released and remains, with pointers to its dummy arguments
now dangling.

The fix adds a condition to avoid the case, and refactors to a new predicate
by the way.  Part of the original condition is also removed, for lack of a
reason to keep it.

	PR fortran/99798

gcc/fortran/ChangeLog:

	* symbol.cc (gfc_release_symbol): Move the condition guarding
	the handling cyclic references...
	(cyclic_reference_break_needed): ... here as a new predicate.
	Remove superfluous parts.  Add a condition preventing any premature
	release with submodule symbols.

gcc/testsuite/ChangeLog:

	* gfortran.dg/submodule_33.f08: New test.
---
 gcc/fortran/symbol.cc                      | 54 +++++++++++++++++++++-
 gcc/testsuite/gfortran.dg/submodule_33.f08 | 20 ++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/submodule_33.f08

Hi Mikael,

That is an ingenious solution. Given the complexity, I think that the
comments are well warranted.

OK for master and, I would suggest, 14-branch after a few weeks.

Thanks!

Paul

On Sun, 12 May 2024 at 14:16, Mikael Morin <mikael@gcc.gnu.org> wrote:

> Hello,
>
> Here is my final patch to fix the ICE of PR99798.
> It's maybe overly verbose with comments, but the memory management is
> hopefully clarified.
> I tested this with a full fortran regression test on x86_64-linux and a
> manual check with valgrind on the testcase.
> OK for master?
>
> -- 8< --
>
> This prevents a premature release of memory with procedure symbols from
> submodules, causing random compiler crashes.
>
> The problem is a fragile detection of cyclic references, which can match
> with procedures host-associated from a module in submodules, in cases
> where it
> shouldn't.  The formal namespace is released, and with it the dummy
> arguments
> symbols of the procedure.  But there is no cyclic reference, so the
> procedure
> symbol itself is not released and remains, with pointers to its dummy
> arguments
> now dangling.
>
> The fix adds a condition to avoid the case, and refactors to a new
> predicate
> by the way.  Part of the original condition is also removed, for lack of a
> reason to keep it.
>
>         PR fortran/99798
>
> gcc/fortran/ChangeLog:
>
>         * symbol.cc (gfc_release_symbol): Move the condition guarding
>         the handling cyclic references...
>         (cyclic_reference_break_needed): ... here as a new predicate.
>         Remove superfluous parts.  Add a condition preventing any premature
>         release with submodule symbols.
>
> gcc/testsuite/ChangeLog:
>
>         * gfortran.dg/submodule_33.f08: New test.
> ---
>  gcc/fortran/symbol.cc                      | 54 +++++++++++++++++++++-
>  gcc/testsuite/gfortran.dg/submodule_33.f08 | 20 ++++++++
>  2 files changed, 72 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gfortran.dg/submodule_33.f08
>
> diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc
> index 8f7deac1d1e..0a1646def67 100644
> --- a/gcc/fortran/symbol.cc
> +++ b/gcc/fortran/symbol.cc
> @@ -3179,6 +3179,57 @@ gfc_free_symbol (gfc_symbol *&sym)
>  }
>
>
> +/* Returns true if the symbol SYM has, through its FORMAL_NS field, a
> reference
> +   to itself which should be eliminated for the symbol memory to be
> released
> +   via normal reference counting.
> +
> +   The implementation is crucial as it controls the proper release of
> symbols,
> +   especially (contained) procedure symbols, which can represent a lot of
> memory
> +   through the namespace of their body.
> +
> +   We try to avoid freeing too much memory (causing dangling pointers),
> to not
> +   leak too much (wasting memory), and to avoid expensive walks of the
> symbol
> +   tree (which would be the correct way to check for a cycle).  */
> +
> +bool
> +cyclic_reference_break_needed (gfc_symbol *sym)
> +{
> +  /* Normal symbols don't reference themselves.  */
> +  if (sym->formal_ns == nullptr)
> +    return false;
> +
> +  /* Procedures at the root of the file do have a self reference, but
> they don't
> +     have a reference in a parent namespace preventing the release of the
> +     procedure namespace, so they can use the normal reference counting.
> */
> +  if (sym->formal_ns == sym->ns)
> +    return false;
> +
> +  /* If sym->refs == 1, we can use normal reference counting.  If
> sym->refs > 2,
> +     the symbol won't be freed anyway, with or without cyclic reference.
> */
> +  if (sym->refs != 2)
> +    return false;
> +
> +  /* Procedure symbols host-associated from a module in submodules are
> special,
> +     because the namespace of the procedure block in the submodule is
> different
> +     from the FORMAL_NS namespace generated by host-association.  So
> there are
> +     two different namespaces representing the same procedure namespace.
> As
> +     FORMAL_NS comes from host-association, which only imports symbols
> visible
> +     from the outside (dummy arguments basically), we can assume there is
> no
> +     self reference through FORMAL_NS in that case.  */
> +  if (sym->attr.host_assoc && sym->attr.used_in_submodule)
> +    return false;
> +
> +  /* We can assume that contained procedures have cyclic references,
> because
> +     the symbol of the procedure itself is accessible in the procedure
> body
> +     namespace.  So we assume that symbols with a formal namespace
> different
> +     from the declaration namespace and two references, one of which is
> about
> +     to be removed, are procedures with just the self reference left.  At
> this
> +     point, the symbol SYM matches that pattern, so we return true here to
> +     permit the release of SYM.  */
> +  return true;
> +}
> +
> +
>  /* Decrease the reference counter and free memory when we reach zero.
>     Returns true if the symbol has been freed, false otherwise.  */
>
> @@ -3188,8 +3239,7 @@ gfc_release_symbol (gfc_symbol *&sym)
>    if (sym == NULL)
>      return false;
>
> -  if (sym->formal_ns != NULL && sym->refs == 2 && sym->formal_ns !=
> sym->ns
> -      && (!sym->attr.entry || !sym->module))
> +  if (cyclic_reference_break_needed (sym))
>      {
>        /* As formal_ns contains a reference to sym, delete formal_ns just
>          before the deletion of sym.  */
> diff --git a/gcc/testsuite/gfortran.dg/submodule_33.f08
> b/gcc/testsuite/gfortran.dg/submodule_33.f08
> new file mode 100644
> index 00000000000..b61d750def1
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/submodule_33.f08
> @@ -0,0 +1,20 @@
> +! { dg-do compile }
> +!
> +! PR fortran/99798
> +! This example used to trigger an ICE caused by a premature release of
> the G
> +! symbol (with its argument X) following the rejection of the subroutine
> in
> +! the submodule.
> +
> +module m
> +   interface
> +      module integer function g(x)
> +         integer, intent(in) :: x
> +      end
> +   end interface
> +end
> +submodule(m) m2
> +contains
> +   subroutine g(x)      ! { dg-error "FUNCTION attribute conflicts with
> SUBROUTINE" }
> +     integer, intent(in) :: x  ! { dg-error "Unexpected data declaration"
> }
> +   end
> +end
> --
> 2.43.0
>
>