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 <mik...@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
>
>
