[Bug fortran/116388] New: [13/14/15 regression] Finalizer called on uninitialized components of intent(out) argument

[Bug fortran/116388] New: [13/14/15 regression] Finalizer called on uninitialized components of intent(out) argument

[trnka at scm dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116388

            Bug ID: 116388
           Summary: [13/14/15 regression] Finalizer called on
                    uninitialized components of intent(out) argument
           Product: gcc
           Version: 13.3.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: trnka at scm dot com
  Target Milestone: ---

Created attachment 58935
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58935&action=edit
testcase illustrating this issue

This feels potentially related to PR115070 but it is probably not an exact
duplicate; the proposed fix in PR115070 comment 9 does not fix this issue on
current master branch.

The attached testcase contains a type like this:

   type, public :: AType
      type(C_ptr) :: cptr = C_null_ptr
      logical     :: cptr_invalid = .true.
      integer, allocatable :: x(:)
   contains
      final              :: FinalizerA
   end type
…
   impure elemental subroutine FinalizerA(self)
      type(AType), intent(inout) :: self

      if (.not. self%cptr_invalid) write(*,*) "A:", self%cptr
   end subroutine

Nothing in the program ever sets "cptr_invalid" to .false., so "A:" should
never be printed. However, compiling the testcase with gfortran 13.x and up
produces a program which often prints "A: somerandomvalue" once. Looking at the
tree dump, this is because the __final for CType creates a temporary
"comp_byte_stride", which is only partially initialized (so the components of
AType apart from x aren't initialized) but then calls FinalizerA on the whole a
component.

integer(kind=4) __final_finalizertestmodule_Ctype (struct array15_ctype &
restrict array, integer(kind=8) byte_stride, logical(kind=1) fini_coarray)
{
  struct btype comp_byte_stride;
…
  try
    {
      comp_byte_stride.a.x.data = 0B;
      // no other components of comp_byte_stride are initialized here
…
  finally
    {
      __builtin_free ((void *) strides);
      __builtin_free ((void *) sizes);
      desc.6.dtype = {.elem_len=80, .version=0, .rank=0, .type=5};
      desc.6.data = (void * restrict) &comp_byte_stride.a;
      desc.6.span = (integer(kind=8)) desc.6.dtype.elem_len;
      __final_finalizertestmodule_Atype (&desc.6, 80, 1);

Accessing uninitialized data at runtime is also confirmed using valgrind
--tool=memcheck.

"comp_byte_stride" was introduced by the following commit which went into 13.x.
GCC 12 and older seems to be unaffected by this issue.

commit d7caf313525a46f200d7f5db1ba893f853774aee
Author: Paul Thomas <pault@gcc.gnu.org>
Date:   Sat Mar 18 07:56:23 2023 +0000

    Fortran: Fix bugs and missing features in finalization [PR37336]

    2023-03-18  Paul Thomas  <pault@gcc.gnu.org>

(BType in the testcase seems to be superfluous but removing it and putting
AType straight into CType masks the issue somewhat, because the uninitialized
data then happens to have the right value so that A: is never printed. However,
the bug still seems to be there in the tree dump even without BType.)

[Bug fortran/116388] [13/14/15 regression] Finalizer called on uninitialized components of intent(out) argument

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116388

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4
   Target Milestone|---                         |13.4

[Bug fortran/116388] [13/14/15 regression] Finalizer called on uninitialized components of intent(out) argument

[trnka at scm dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116388

--- Comment #1 from Tomáš Trnka <trnka at scm dot com> ---
Diagnosing this further, the "comp_byte_stride" temporary created by
finalize_component() is never initialized because it is marked .artificial=1,
and resolve_symbol() bails out early for artificial symbols instead of calling
apply_default_init() normally.

The temporary is never actually used in generated code, because it's only an
argument to storage_size(), which is then replaced by an integer constant.

However, the temporary is then finalized as if it was a normal variable,
because gfc_trans_deferred_array() skips finalization (via
gfc_deallocate_alloc_comp()) only if !(sym->attr.artificial && sym->name[0] ==
'_'). Being artificial is thus not sufficient; as long as the symbol doesn't
start with an underscore, it will be finalized. This discrepancy causes the bug
by calling finalization on uninitialized data.

One hacky "fix" is to give the temporary a leading underscore, thus avoiding
finalization altogether:
--- a/gcc/fortran/class.cc
+++ b/gcc/fortran/class.cc
@@ -1152,7 +1152,7 @@ finalize_component (gfc_expr *expr, gfc_symbol *derived,
gfc_component *comp,
       gcc_assert (c);

       /* Set scalar argument for storage_size.  */
-      gfc_get_symbol ("comp_byte_stride", sub_ns, &byte_stride);
+      gfc_get_symbol ("_comp_byte_stride", sub_ns, &byte_stride);
       byte_stride->ts = e->ts;
       byte_stride->attr.flavor = FL_VARIABLE;
       byte_stride->attr.value = 1;

This regtests cleanly on current trunk and seems to fix the issue. However,
since I'm only taking first baby steps in gfortran development, I have no clue
what the significance of the leading underscore is and what sort of other side
effects this change could have.

Could someone (Paul?) comment on this?

I have also tested the alternative of making comp_byte_stride not artificial:

--- a/gcc/fortran/class.cc
+++ b/gcc/fortran/class.cc
@@ -1088,8 +1088,8 @@ finalize_component (gfc_expr *expr, gfc_symbol *derived,
gfc_component *comp,
       gfc_get_symbol ("comp_byte_stride", sub_ns, &byte_stride);
       byte_stride->ts = e->ts;
       byte_stride->attr.flavor = FL_VARIABLE;
-      byte_stride->attr.value = 1;
-      byte_stride->attr.artificial = 1;
+      byte_stride->attr.value = 0;
+      byte_stride->attr.artificial = 0;
       gfc_set_sym_referenced (byte_stride);
       gfc_commit_symbol (byte_stride);
       scalar = gfc_lval_expr_from_sym (byte_stride);

Although this seems to fix the issue in this PR (according to the tree dump,
the variable is then fully initialized and finalized afterwards), it causes
crashes in several tests (which I have not investigated further):

FAIL: gfortran.dg/finalize_25.f90
FAIL: gfortran.dg/finalize_37.f90
FAIL: gfortran.dg/finalize_44.f90
FAIL: gfortran.dg/finalize_46.f90
(snipped all the different optimization levels)