[Bug fortran/113503] New: [14 Regression] xtb test miscompilation starting with r14-870

[Bug fortran/113503] New: [14 Regression] xtb test miscompilation starting with r14-870

[jakub at gcc dot gnu.org]

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

            Bug ID: 113503
           Summary: [14 Regression] xtb test miscompilation starting with
                    r14-870
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

Since r14-870-g6c95fe9bc0553743098eeaa739f14b885050fa42 4 xtb tests seem to be
miscompiled, they crash due to memcpy into NULL pointer.

Reduced testcase is:
module xtb_solv_input
   implicit none
   private
   public :: TSolvInput
   type :: TSolvInput
      character(len=:), allocatable :: solvent
   end type
end module
module xtb_main_setup
   use xtb_solv_input, only : TSolvInput
   private
   public :: addSolvationModel
contains
subroutine addSolvationModel(input)
   type(TSolvInput), intent(in) :: input
end subroutine
end module
program xtb
    use xtb_solv_input, only : TSolvInput
    use xtb_main_setup, only : addSolvationModel
    integer :: iMol
    character(len=*), parameter :: solvents(10) = [character(len=20) ::&
      & "h2o", "chcl3", "thf", "acetonitrile", "toluene", &
      & "ch2cl2", "ether", "methanol", "cs2", "dmso"]
    do iMol = 1, 10
      call addSolvationModel(TSolvInput(solvent=trim(solvents(iMol))))
    end do
end program

When compiled with -O2 -fno-inline -Wuninitialized starting with that revision
it emits:
xtb.f90:26:70:

   26 |       call addSolvationModel(TSolvInput(solvent=trim(solvents(iMol))))
      |                                                                      ^
Warning: ‘len.8’ is used uninitialized [-Wuninitialized]
xtb.f90:26:70:

   26 |       call addSolvationModel(TSolvInput(solvent=trim(solvents(iMol))))
      |                                                                      ^
note: ‘len.8’ declared here
warning.  This reduced test doesn't crash, but the unreduced one crashes
exactly because of the uninitialized use - when the uninitialized var happens
to contain some huge value (0x7fffffffffffe0b0 or so in my case), that value is
passed to malloc,
malloc obviously returns NULL as I don't have that much memory and the code
then tries to memcpy "h2o" into NULL.

The bug is visible in the gimple dump already:
            integer(kind=8) len.8;
            integer(kind=8) slen.9;

            try
              {
                slen.9 = len.8;
                slen.18_1 = (sizetype) slen.9;
                _2 = MAX_EXPR <slen.18_1, 1>;
                _3 = __builtin_malloc (_2);
                tsolvinput.6.solvent = _3;
                tsolvinput.6._solvent_length = slen.9;
                _4 = (integer(kind=8)) imol;
                _5 = _4 + -1;
                _6 = &solvents[_5];
                _gfortran_string_trim (&len.8, &pstr.7, 20, _6);
                len.19_7 = len.8;
                len.20_8 = (unsigned long) len.19_7;
                pstr.21_9 = pstr.7;
                _10 = tsolvinput.6.solvent;
                __builtin_memcpy (_10, pstr.21_9, len.20_8);
                len.22_11 = len.8;
In r14-868 instead it looked like
            integer(kind=8) len.8;
            integer(kind=8) D.4372;
            character(kind=1) * pstr.9;
            integer(kind=8) len.10;

            try
              {
                _1 = (integer(kind=8)) imol;
                _2 = _1 + -1;
                _3 = &solvents[_2];
                _gfortran_string_trim (&len.8, &pstr.7, 20, _3);
                len.19_4 = len.8;
                if (len.19_4 > 0) goto <D.4416>; else goto <D.4417>;
                <D.4416>:
                pstr.20_5 = pstr.7;
                __builtin_free (pstr.20_5);

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pault at gcc dot gnu.org
   Target Milestone|---                         |14.0
           Priority|P3                          |P1

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[anlauf at gcc dot gnu.org]

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

--- Comment #1 from anlauf at gcc dot gnu.org ---
When trying to further reduce the code I get either an ICE or an
uninitialized-warning for:

program xtb
  implicit none
  type :: TSolvInput
     character(len=:), allocatable :: solvent
  end type
  character(len=20) :: solvents(1) = 'h2o'

! call addSolvationModel(TSolvInput(solvent=     solvents(1)))  ! ICE
  call addSolvationModel(TSolvInput(solvent=trim(solvents(1)))) ! Warning

contains
  subroutine addSolvationModel(input)
    type(TSolvInput), intent(in) :: input
  end
end

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[anlauf at gcc dot gnu.org]

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

--- Comment #2 from anlauf at gcc dot gnu.org ---
(In reply to anlauf from comment #1)
> When trying to further reduce the code I get either an ICE or an
> uninitialized-warning for:

program xtb
  implicit none
  type :: TSolvInput
     character(len=:), allocatable :: solvent
  end type
  character(len=20) :: solvents(1) = 'h2o'
  type(TSolvInput)  :: x
! x = TSolvInput(solvent=     solvents(1))  ! ICE
  x = TSolvInput(solvent=trim(solvents(1))) ! Warning
end


So it clearly comes from the constructor.

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-01-20

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
.

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
It is the
PR fortran/82774
(alloc_scalar_allocatable_subcomponent): Shorten the function
name and replace the symbol argument with the se string length.
If a deferred length character length is either not present or
is not a variable, give the typespec a variable and assign the
string length to that. Use gfc_deferred_strlen to find the
hidden string length component.
(gfc_trans_subcomponent_assign): Convert the expression before
the call to alloc_scalar_allocatable_subcomponent so that a
good string length is provided.
(gfc_trans_structure_assign): Remove the unneeded derived type
symbol from calls to gfc_trans_subcomponent_assign.
part of the changes that cause this, reverting those hunks (had to revert one
manually as tree size; declaration has been added there later) makes the
testcase
not warn anymore or in the other case not ICE anymore.

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For the warning case, I think
--- gcc/fortran/trans-expr.cc.jj        2024-02-14 14:26:19.764810614 +0100
+++ gcc/fortran/trans-expr.cc   2024-02-16 13:22:48.693104239 +0100
@@ -9253,19 +9253,20 @@ gfc_trans_subcomponent_assign (tree dest
           || (cm->ts.type == BT_CLASS && CLASS_DATA (cm)->attr.allocatable
               && expr->ts.type != BT_CLASS)))
     {
+      tree size;
+
       gfc_init_se (&se, NULL);
       gfc_conv_expr (&se, expr);
-      tree size;

+      /* The remainder of these instructions follow the if (cm->attr.pointer)
+        if (!cm->attr.dimension) part above.  */
+      gfc_add_block_to_block (&block, &se.pre);
       /* Take care about non-array allocatable components here.  The alloc_*
         routine below is motivated by the alloc_scalar_allocatable_for_
         assignment() routine, but with the realloc portions removed and
         different input.  */
       alloc_scalar_allocatable_subcomponent (&block, dest, cm, expr,
                                             se.string_length);
-      /* The remainder of these instructions follow the if (cm->attr.pointer)
-        if (!cm->attr.dimension) part above.  */
-      gfc_add_block_to_block (&block, &se.pre);

       if (expr->symtree && expr->symtree->n.sym->attr.proc_pointer
          && expr->symtree->n.sym->attr.dummy)
should fix this.  I don't see how code in se.pre could depend on what
alloc_scalar_allocatable_subcomponent emits, and clearly from the #c0 testcase
there can be a dependency in the other direction.

As for the ICE case, this is due to
      if (!expr2->ts.u.cl->backend_decl
          || !VAR_P (expr2->ts.u.cl->backend_decl))
        expr2->ts.u.cl->backend_decl = gfc_create_var (TREE_TYPE (slen),
                                                       "slen");
      gfc_add_modify (block, expr2->ts.u.cl->backend_decl, slen);
expr2->ts.u.cl->backend_decl here is INTEGER_CST and slen is the same
INTEGER_CST (20 in both cases), so changing expr2->ts.u.cl->backend_decl to a
temporary variable pushed into current block is definitely undesirable, it
means references to that slen var can be emitted even before or after the
containing block into which the temporary is pushed using pushdecl.

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 57436
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57436&action=edit
gcc14-pr113503.patch

So far only lightly tested patch (make -j32 check-gfortran).

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:296284a9dbb7df4485cc5f1d3e975fdb4b8a10b8

commit r14-9049-g296284a9dbb7df4485cc5f1d3e975fdb4b8a10b8
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Feb 17 16:54:08 2024 +0100

    fortran: gfc_trans_subcomponent_assign fixes [PR113503]

    The r14-870 changes broke xtb package tests (reduced testcase is the first
    one below) and caused ICEs on a test derived from that (the second one).
    For the
      x = T(u = trim (us(1)))
    statement, before that change gfortran used to emit weird code with
    2 trim calls:
          _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
          if (len.2 > 0)
            {
              __builtin_free ((void *) pstr.1);
            }
          D.4275 = len.2;
          t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR
<(sizetype) D.4275, 1>);
          t.0._u_length = D.4275;
          _gfortran_string_trim (&len.4, (void * *) &pstr.3, 20, &us[0]);
          (void) __builtin_memcpy ((void *) t.0.u, (void *) pstr.3, (unsigned
long) NON_LVALUE_EXPR <len.4>);
          if (len.4 > 0)
            {
              __builtin_free ((void *) pstr.3);
            }
    That worked at runtime, though it is wasteful.
    That commit changed it to:
          slen.3 = len.2;
          t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR
<(sizetype) slen.3, 1>);
          t.0._u_length = slen.3;
          _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
          (void) __builtin_memcpy ((void *) t.0.u, (void *) pstr.1, (unsigned
long) NON_LVALUE_EXPR <len.2>);
          if (len.2 > 0)
            {
              __builtin_free ((void *) pstr.1);
            }
    which results in -Wuninitialized warning later on and if one is unlucky and
    the uninitialized len.2 variable is smaller than the trimmed length, it
    results in heap overflow and often crashes later on.
    The bug above is clear, len.2 is only initialized in the
    _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
    call, but used before that.  Now, the
          slen.3 = len.2;
          t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR
<(sizetype) slen.3, 1>);
          t.0._u_length = slen.3;
    statements come from the alloc_scalar_allocatable_subcomponent call,
    while
          _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
    from the gfc_conv_expr (&se, expr); call which is done before the
    alloc_scalar_allocatable_subcomponent call, but is only appended later on
    with gfc_add_block_to_block (&block, &se.pre);
    Now, obviously the alloc_scalar_allocatable_subcomponent emitted statements
    can depend on the se.pre sequence statements which can compute variables
    used by alloc_scalar_allocatable_subcomponent like the length.
    On the other side, I think the se.pre sequence really shouldn't depend
    on the changes done by alloc_scalar_allocatable_subcomponent, that is
    initializing the FIELD_DECLs of the destination allocatable subcomponent
    only, the gfc_conv_expr statements are already created, so all they could
    in theory depend above is on t.0.u or t.0._u_length, but I believe if the
    rhs dependened on the lhs content (which is allocated by those statements
    but really uninitialized), it would need to be discovered by the dependency
    analysis and forced into a temporary.
    So, in order to fix the first testcase, the second hunk of the patch just
    emits the se.pre block before the alloc_scalar_allocatable_subcomponent
    changes rather than after it.

    The second problem is an ICE on the second testcase.  expr in the caller
    (expr2 inside of alloc_scalar_allocatable_subcomponent) has
    expr2->ts.u.cl->backend_decl already set, INTEGER_CST 20, but
    alloc_scalar_allocatable_subcomponent overwrites it to a new VAR_DECL
    which it assigns a value to before the malloc.  That can work if the only
    places the expr2->ts is ever used are in the same local block or its
    subblocks (and only if it is dominated by the code emitted by
    alloc_scalar_allocatable_subcomponent, so e.g. not if that call is inside
    of a conditional code and use later unconditional), but doesn't work
    if expr2->ts is used before that block or after it.  So, the exact ICE is
    because of:
      slen.1 = 20;
        static character(kind=1) us[1][1:20] = {"foo                 "};
      x.u = 0B;
      x._u_length = 0;
      {
        struct t t.0;
        struct t D.4308;

        {
          integer(kind=8) slen.1;

          slen.1 = 20;
          t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR
<(sizetype) slen.1, 1>);
          t.0._u_length = slen.1;
          (void) __builtin_memcpy ((void *) t.0.u, (void *) &us[0], 20);
        }
    where the first slen.1 = 20; is emitted because it sees us has a VAR_DECL
    ts.u.cl->backend_decl and so it wants to initialize it to the actual
length.
    This is invalid GENERIC, because the slen.1 variable is only declared
inside
    of a {} later on and so uses outside of it are wrong.  Similarly wrong
would
    be if it is used later on.  E.g. in the same testcase if it has
      type(T) :: x, y
      x = T(u = us(1))
      y%u = us(1)
    then there is
        {
          integer(kind=8) slen.1;

          slen.1 = 20;
          t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR
<(sizetype) slen.1, 1>);
          t.0._u_length = slen.1;
          (void) __builtin_memcpy ((void *) t.0.u, (void *) &us[0], 20);
        }
    ...
        if (y.u != 0B) goto L.1;
        y.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype)
slen.1, 1>);
    i.e. another use of slen.1, this time after slen.1 got out of scope.

    I really don't understand why the code modifies
    expr2->ts.u.cl->backend_decl, expr2 isn't used there anywhere except for
    expr2->ts.u.cl->backend_decl expressions, so hacks like save the previous
    value, overwrite it temporarily over some call that will use expr2 and
    restore afterwards aren't needed - there are no such calls, so the
    following patch fixes it just by not messing up with
    expr2->ts.u.cl->backend_decl, only set it to size variable and overwrite
    that with a temporary if needed.

    2024-02-17  Jakub Jelinek  <jakub@redhat.com>

            PR fortran/113503
            * trans-expr.cc (alloc_scalar_allocatable_subcomponent): Don't
            overwrite expr2->ts.u.cl->backend_decl, instead set size to
            expr2->ts.u.cl->backend_decl first and use size instead of
            expr2->ts.u.cl->backend_decl.
            (gfc_trans_subcomponent_assign): Emit se.pre into block
            before calling alloc_scalar_allocatable_subcomponent instead of
            after it.

            * gfortran.dg/pr113503_1.f90: New test.
            * gfortran.dg/pr113503_2.f90: New test.

[Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.