public inbox for fortran@gcc.gnu.org
 help / color / mirror / Atom feed
From: Salvatore Filippone <filippone.salvatore@gmail.com>
To: Paul Richard Thomas <paul.richard.thomas@gmail.com>
Cc: Jerry D <jvdelisle2@gmail.com>,
	Andrew Benson <abenson@carnegiescience.edu>,
	Damian Rouson <rouson@lbl.gov>,
	Fortran List <fortran@gcc.gnu.org>
Subject: Re: FINAL subroutines
Date: Fri, 28 Jan 2022 09:05:33 +0100	[thread overview]
Message-ID: <CANSzZf5ATqf5WHywvhmyhRaKQ+UKsFB1NednTpJzLFhwV60BVg@mail.gmail.com> (raw)
In-Reply-To: <CAGkQGiKDCH2cWVzwCi8nwrmaT=35F+ErH849uZ1HN1xXtqW6iQ@mail.gmail.com>

So, you are saying that three calls is the correct number, one for the LHS,
one for the RHS, and one for the deallocation in the main program.
I have (re)-read the standard, I would concur with your interpretation.

On Thu, Jan 27, 2022 at 11:10 PM Paul Richard Thomas <
paul.richard.thomas@gmail.com> wrote:

> Hi Salvatore,
>
>
> My reading of F2018: 7.5.6.3 "When finalization occurs" is that three
> calls is correct. (i) Paragraph 1 stipulates that the 'var' expression be
> evaluated before the reallocation and intrinsic assignment; (ii)stipulates
> that the function result be finalised "the result is finalized after
> execution of the innermost
> executable construct containing the reference." ; and (iii) Finalisation
> occurs on going out of scope.
>
> That is what is implemented in the attached. I am working my way through
> the testcase dependencies of PR37336 making sure that finalizat
> ion occurs as required by 7.5.6.3 and in the order defined in the previous
> section. It will all be done in the next few days.
>
> What will remain is finalization of function results within array
> constructors and one or two other corner cases. Following that, the
> existing finalization calls will be brought into the framework as the new
> calls.
>
> Best regards
>
> Paul
>
>
> On Thu, 27 Jan 2022 at 07:17, Salvatore Filippone <
> filippone.salvatore@gmail.com> wrote:
>
>> One more data point: Cray FTN issues TWO calls to the FINAL.
>> Which begs the question: what is the correct number of calls one, two or
>> three?
>> Salvatore
>>
>> fsalvato@daint102:/project/prce01/fsalvato/NUMERICAL/PSBLAS/V4/psblas4/test/newstuff>
>> ftn --version
>> Cray Fortran : Version 11.0.0
>> fsalvato@daint102:/project/prce01/fsalvato/NUMERICAL/PSBLAS/V4/psblas4/test/newstuff>
>> ftn -o testfinal testfinal.f90
>> fsalvato@daint102:/project/prce01/fsalvato/NUMERICAL/PSBLAS/V4/psblas4/test/newstuff>
>> ./testfinal
>>  Allocating wrapper
>>  Calling new_outer_type
>>  Assigning outer%test_item
>>  Called delete_test_type
>>  End of new_outer_type
>>  DeAllocating wrapper
>>  Called delete_test_type
>>
>> On Wed, Jan 26, 2022 at 11:59 PM Paul Richard Thomas <
>> paul.richard.thomas@gmail.com> wrote:
>>
>>> Hi Jerry,
>>>
>>> I am trying to fix the failure of my latest patch with this very test
>>> case. Otherwise it fixes most of the remaining dependencies in PR37336.
>>>
>>> At a pinch, I could submit the earlier patch that Andrew mentions and
>>> work from there. However, you will note that it does miss one of the
>>> finalizations. This is critical because function results, which should be
>>> finalized, are not.
>>>
>>> I'll keep an eye on the state of the branch. By and large, release
>>> occurs 3-4 months after the start of stage 4. I will leave 2 months maximum.
>>>
>>> Best regards
>>>
>>> Paul
>>>
>>>
>>> On Wed, 26 Jan 2022 at 21:29, Jerry D via Fortran <fortran@gcc.gnu.org>
>>> wrote:
>>>
>>>> Is there any reason these patches can not be applied and use this test
>>>> as a test case?
>>>>
>>>> Regards,
>>>>
>>>> Jerry
>>>>
>>>> On 1/24/22 8:11 AM, Salvatore Filippone via Fortran wrote:
>>>> > Thanks a lot
>>>> > (yes, I suspected both gfortran and intel were wrong, precisely
>>>> because I
>>>> > could see why you'd need two FINAL calls, but not three).
>>>> >
>>>> > Salvatore
>>>> >
>>>> > On Mon, Jan 24, 2022 at 4:45 PM Andrew Benson <
>>>> abenson@carnegiescience.edu>
>>>> > wrote:
>>>> >
>>>> >> Hi Salvatore,
>>>> >>
>>>> >> This looks like it's related to some of the missing finalization
>>>> >> functionality
>>>> >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37336). Paul has some
>>>> >> patches
>>>> >> (e.g. https://gcc.gnu.org/pipermail/fortran/2022-January/057415.html
>>>> )
>>>> >> which
>>>> >> implement most of the missing functionality. With those patches
>>>> >> incorporated
>>>> >> your code gives the following output with gfortran:
>>>> >>
>>>> >> $ ./testfinal
>>>> >>   Allocating wrapper
>>>> >>   Calling new_outer_type
>>>> >>   Assigning outer%test_item
>>>> >>   Called delete_test_type
>>>> >>   End of new_outer_type
>>>> >>   DeAllocating wrapper
>>>> >>   Called delete_test_type
>>>> >>
>>>> >> So there is one more call to the finalizer than you found - I haven't
>>>> >> checked
>>>> >> carefully but I would guess this is a deallocation of LHS on
>>>> assignment.
>>>> >>
>>>> >> In testing these patches using the Intel compiler we found that it
>>>> seems
>>>> >> to
>>>> >> call the finalization wrapper more than it should, sometimes on
>>>> objects
>>>> >> that
>>>> >> have already been deallocated. Your code, compiled with the Intel
>>>> compiler
>>>> >> and
>>>> >> then run under valgrind shows the following:
>>>> >>
>>>> >> $ valgrind ./testfinal
>>>> >> ==7340== Memcheck, a memory error detector
>>>> >> ==7340== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et
>>>> al.
>>>> >> ==7340== Using Valgrind-3.13.0 and LibVEX; rerun with -h for
>>>> copyright info
>>>> >> ==7340== Command: ./testfinal
>>>> >> ==7340==
>>>> >> ==7340== Conditional jump or move depends on uninitialised value(s)
>>>> >> ==7340==    at 0x493A51: __intel_sse2_strcpy (in
>>>> /home/abensonca/Scratch/
>>>> >> ifortTests/testfinal)
>>>> >> ==7340==    by 0x45D70E: for__add_to_lf_table (in
>>>> /home/abensonca/Scratch/
>>>> >> ifortTests/testfinal)
>>>> >> ==7340==    by 0x4410CB: for__open_proc (in /home/abensonca/Scratch/
>>>> >> ifortTests/testfinal)
>>>> >> ==7340==    by 0x423A64: for__open_default (in
>>>> /home/abensonca/Scratch/
>>>> >> ifortTests/testfinal)
>>>> >> ==7340==    by 0x4305A9: for_write_seq_lis (in
>>>> /home/abensonca/Scratch/
>>>> >> ifortTests/testfinal)
>>>> >> ==7340==    by 0x4047E1: MAIN__ (testfinal.f90:62)
>>>> >> ==7340==    by 0x403CE1: main (in /home/abensonca/Scratch/ifortTests/
>>>> >> testfinal)
>>>> >> ==7340==
>>>> >>   Allocating wrapper
>>>> >>   Calling new_outer_type
>>>> >>   Assigning outer%test_item
>>>> >>   Called delete_test_type
>>>> >> ==7340== Conditional jump or move depends on uninitialised value(s)
>>>> >> ==7340==    at 0x40572A: do_alloc_copy (in
>>>> >> /home/abensonca/Scratch/ifortTests/
>>>> >> testfinal)
>>>> >> ==7340==    by 0x406B9A: do_alloc_copy (in
>>>> >> /home/abensonca/Scratch/ifortTests/
>>>> >> testfinal)
>>>> >> ==7340==    by 0x4084ED: for_alloc_assign_v2 (in
>>>> /home/abensonca/Scratch/
>>>> >> ifortTests/testfinal)
>>>> >> ==7340==    by 0x404474: target_mod_mp_new_outer_type_
>>>> (testfinal.f90:48)
>>>> >> ==7340==    by 0x40485E: MAIN__ (testfinal.f90:65)
>>>> >> ==7340==    by 0x403CE1: main (in /home/abensonca/Scratch/ifortTests/
>>>> >> testfinal)
>>>> >> ==7340==
>>>> >>   Called delete_test_type
>>>> >>   End of new_outer_type
>>>> >>   DeAllocating wrapper
>>>> >>   Called delete_test_type
>>>> >> ==7340==
>>>> >> ==7340== HEAP SUMMARY:
>>>> >> ==7340==     in use at exit: 48 bytes in 1 blocks
>>>> >> ==7340==   total heap usage: 14 allocs, 13 frees, 12,879 bytes
>>>> allocated
>>>> >> ==7340==
>>>> >> ==7340== LEAK SUMMARY:
>>>> >> ==7340==    definitely lost: 48 bytes in 1 blocks
>>>> >> ==7340==    indirectly lost: 0 bytes in 0 blocks
>>>> >> ==7340==      possibly lost: 0 bytes in 0 blocks
>>>> >> ==7340==    still reachable: 0 bytes in 0 blocks
>>>> >> ==7340==         suppressed: 0 bytes in 0 blocks
>>>> >> ==7340== Rerun with --leak-check=full to see details of leaked memory
>>>> >> ==7340==
>>>> >> ==7340== For counts of detected and suppressed errors, rerun with: -v
>>>> >> ==7340== Use --track-origins=yes to see where uninitialised values
>>>> come
>>>> >> from
>>>> >> ==7340== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from
>>>> 0)
>>>> >>
>>>> >> so there are some cases of what look like incorrect accesses (and
>>>> some
>>>> >> leaked
>>>> >> memory).
>>>> >>
>>>> >> Your code compiled  with gfortran (with Paul's patches in place)
>>>> shows no
>>>> >> errors or leaks from valgrind.
>>>> >>
>>>> >> So, in summary, in this case I think the current gfortran is missing
>>>> some
>>>> >> finalizations (which are fixed by Paul's patches), and ifort is
>>>> likely
>>>> >> doing
>>>> >> something wrong and probably calling the finalizer more times than it
>>>> >> should.
>>>> >>
>>>> >> -Andrew
>>>> >>
>>>> >> On Monday, January 24, 2022 6:49:23 AM PST Salvatore Filippone via
>>>> Fortran
>>>> >> wrote:
>>>> >>> And here is the code embedded as text............ sorry  about
>>>> sending an
>>>> >>> attachment that was purged
>>>> >>> ------------------------- testfinal.f90 ---------------------
>>>> >>> module test_type_mod
>>>> >>>
>>>> >>>    type :: my_test_type
>>>> >>>      integer, allocatable :: i
>>>> >>>    contains
>>>> >>>      final :: delete_test_type
>>>> >>>    end type my_test_type
>>>> >>>
>>>> >>>    interface my_test_type
>>>> >>>      module procedure  new_test_type_object
>>>> >>>    end interface my_test_type
>>>> >>>
>>>> >>> contains
>>>> >>>
>>>> >>>    subroutine delete_test_type(this)
>>>> >>>      type(my_test_type) :: this
>>>> >>>
>>>> >>>      write(*,*) 'Called delete_test_type'
>>>> >>>      if (allocated(this%i)) deallocate(this%i)
>>>> >>>
>>>> >>>    end subroutine delete_test_type
>>>> >>>
>>>> >>>
>>>> >>>    function new_test_type_object(item) result(res)
>>>> >>>      type(my_test_type)  :: res
>>>> >>>      integer, intent(in) :: item
>>>> >>>      !Allocation on assignment
>>>> >>>      res%i=item
>>>> >>>    end function new_test_type_object
>>>> >>>
>>>> >>>
>>>> >>> end module test_type_mod
>>>> >>>
>>>> >>> module target_mod
>>>> >>>    use test_type_mod
>>>> >>>    type :: outer_type
>>>> >>>      type(my_test_type), allocatable  :: test_item
>>>> >>>    end type outer_type
>>>> >>>
>>>> >>> contains
>>>> >>>
>>>> >>>    subroutine new_outer_type(outer,item)
>>>> >>>      type(outer_type), intent(out) :: outer
>>>> >>>      integer :: item
>>>> >>>
>>>> >>>      allocate(outer%test_item)
>>>> >>>      write(*,*) 'Assigning outer%test_item'
>>>> >>>      outer%test_item = my_test_type(itemi)
>>>> >>>      write(*,*) 'End of new_outer_type'
>>>> >>>    end subroutine new_outer_type
>>>> >>>
>>>> >>> end module target_mod
>>>> >>>
>>>> >>> program testfinal
>>>> >>>    use target_mod
>>>> >>>
>>>> >>>    implicit none
>>>> >>>
>>>> >>>    integer :: i=10
>>>> >>>    type(outer_type), allocatable  :: wrapper
>>>> >>>
>>>> >>>    write(*,*) 'Allocating wrapper '
>>>> >>>    allocate(wrapper)
>>>> >>>    write(*,*) 'Calling new_outer_type '
>>>> >>>    call new_outer_type(wrapper,i)
>>>> >>>    write(*,*) 'DeAllocating wrapper '
>>>> >>>    deallocate(wrapper)
>>>> >>>
>>>> >>> end program testfinal
>>>> >>>
>>>> >>> On Mon, Jan 24, 2022 at 2:50 PM Salvatore Filippone <
>>>> >>>
>>>> >>> filippone.salvatore@gmail.com> wrote:
>>>> >>>> Hi all
>>>> >>>> The attached code compiles and runs fine under both GNU and Intel,
>>>> but
>>>> >> it
>>>> >>>> produces different results, in particular the FINAL subroutine is
>>>> >> invoked
>>>> >>>> just once with GNU, three times with Intel.
>>>> >>>>
>>>> >>>> It seems to me that they cannot both be right; I am not sure what
>>>> the
>>>> >>>> standard is mandating in this case.
>>>> >>>> Any ideas?
>>>> >>>> Salvatore
>>>> >>>> ---------------  Intel
>>>> >>>> [pr1eio03@login1: newstuff]$ ifort -v
>>>> >>>> ifort version 19.1.1.217
>>>> >>>> [pr1eio03@login1: newstuff]$ ifort -o testfinal testfinal.f90
>>>> >>>> [pr1eio03@login1: newstuff]$ ./testfinal
>>>> >>>>
>>>> >>>>   Allocating wrapper
>>>> >>>>   Calling new_outer_type
>>>> >>>>   Assigning outer%test_item
>>>> >>>>   Called delete_test_type
>>>> >>>>   Called delete_test_type
>>>> >>>>   End of new_outer_type
>>>> >>>>   DeAllocating wrapper
>>>> >>>>   Called delete_test_type
>>>> >>>>
>>>> >>>> ----------------------------- GNU
>>>> >>>> sfilippo@lagrange newstuff]$ gfortran -v
>>>> >>>> Using built-in specs.
>>>> >>>> COLLECT_GCC=gfortran
>>>> >>>>
>>>> COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/11/lto-wrapper
>>>> >>>> OFFLOAD_TARGET_NAMES=nvptx-none
>>>> >>>> OFFLOAD_TARGET_DEFAULT=1
>>>> >>>> Target: x86_64-redhat-linux
>>>> >>>> Configured with: ../configure --enable-bootstrap
>>>> >>>> --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto
>>>> >> --prefix=/usr
>>>> >>>> --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=
>>>> >>>> http://bugzilla.redhat.com/bugzilla --enable-shared
>>>> >>>> --enable-threads=posix --enable-checking=release --enable-multilib
>>>> >>>> --with-system-zlib --enable-__cxa_atexit
>>>> --disable-libunwind-exceptions
>>>> >>>> --enable-gnu-unique-object --enable-linker-build-id
>>>> >>>> --with-gcc-major-version-only --with-linker-hash-style=gnu
>>>> >> --enable-plugin
>>>> >>>> --enable-initfini-array
>>>> >>>>
>>>> >>
>>>> --with-isl=/builddir/build/BUILD/gcc-11.2.1-20210728/obj-x86_64-redhat-lin
>>>> >>>> ux/isl-install --enable-offload-targets=nvptx-none
>>>> >> --without-cuda-driver
>>>> >>>> --enable-gnu-indirect-function --enable-cet --with-tune=generic
>>>> >>>> --with-arch_32=i686 --build=x86_64-redhat-linux
>>>> >>>> Thread model: posix
>>>> >>>> Supported LTO compression algorithms: zlib zstd
>>>> >>>> gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC)
>>>> >>>> [sfilippo@lagrange newstuff]$ gfortran -o testfinal testfinal.f90
>>>> >>>> [sfilippo@lagrange newstuff]$ ./testfinal
>>>> >>>>
>>>> >>>>   Allocating wrapper
>>>> >>>>   Calling new_outer_type
>>>> >>>>   Assigning outer%test_item
>>>> >>>>   End of new_outer_type
>>>> >>>>   DeAllocating wrapper
>>>> >>>>   Called delete_test_type
>>>> >>>>
>>>> >>>> ---------------------
>>>> >>
>>>> >> --
>>>> >>
>>>> >> * Andrew Benson: https://abensonca.github.io
>>>> >>
>>>> >> * Galacticus: https://github.com/galacticusorg/galacticus
>>>> >>
>>>> >>
>>>> >>
>>>> >>
>>>>
>>>>
>>>
>>> --
>>> "If you can't explain it simply, you don't understand it well enough" -
>>> Albert Einstein
>>>
>>
>
> --
> "If you can't explain it simply, you don't understand it well enough" -
> Albert Einstein
>

  reply	other threads:[~2022-01-28  8:05 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-24 13:50 Salvatore Filippone
2022-01-24 14:49 ` Salvatore Filippone
2022-01-24 15:45   ` Andrew Benson
2022-01-24 16:11     ` Salvatore Filippone
2022-01-26 21:29       ` Jerry D
2022-01-26 22:59         ` Paul Richard Thomas
2022-01-27  7:17           ` Salvatore Filippone
2022-01-27 22:10             ` Paul Richard Thomas
2022-01-28  8:05               ` Salvatore Filippone [this message]
2022-01-28  9:01                 ` Paul Richard Thomas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANSzZf5ATqf5WHywvhmyhRaKQ+UKsFB1NednTpJzLFhwV60BVg@mail.gmail.com \
    --to=filippone.salvatore@gmail.com \
    --cc=abenson@carnegiescience.edu \
    --cc=fortran@gcc.gnu.org \
    --cc=jvdelisle2@gmail.com \
    --cc=paul.richard.thomas@gmail.com \
    --cc=rouson@lbl.gov \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).