[patch, fortran] Asynchronous I/O, take 3

[Thomas König]

Hello world,

the attached patch is the third take on Nicolas' and my patch
for implementing asynchronous I/O. Some parts have been reworked, and
several bugs which caused either incorrect I/O or hangs have been
fixed in the process.

I have to say that getting out these bugs has been much harder
than Nicolas and I originally thought, and that this has cost more
working hours than any other patch I have been involved in.

This has been regression-tested on x86_64-pc-linux-gnu. The new test
cases have also been tested in a tight loop with

n=1; while ./a.out; do echo -n $n " " ; n=$((n+1)); done

or (for async_io_3.f90, which is supposed to fail)

while true ; do ./a.out > /dev/null 2>&1 ; echo -n $n " " ; n=$((n+1));
done

and the test cases also come up clean with valgrind --tool=drd
(which is a _very_ strict tool which, after this experience, I
wholeheartedly recommend for doing pthreads debugging).

The interface remains as before - link in pthread to get asynchronous
I/O, which matches what ifort does.

So, OK for trunk?

Regards

Thomas

2018-07-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* gfortran.texi: Add description of asynchronous I/O.
* trans-decl.c (gfc_finish_var_decl): Treat asynchronous variables
as volatile.
* trans-io.c (gfc_build_io_library_fndecls): Rename st_wait to
st_wait_async and change argument spec from ".X" to ".w".
(gfc_trans_wait): Pass ID argument via reference.

2018-07-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* gfortran.dg/f2003_inquire_1.f03: Add write statement.
* gfortran.dg/f2003_io_1.f03: Add wait statement.

2018-01-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* Makefile.am: Add async.c to gfor_io_src.
Add async.h to gfor_io_headers.
* Makefile.in: Regenerated.
* gfortran.map: Add _gfortran_st_wait_async.
* io/async.c: New file.
* io/async.h: New file.
* io/close.c: Include async.h.
(st_close): Call async_wait for an asynchronous unit.
* io/file_pos.c (st_backspace): Likewise.
(st_endfile): Likewise.
(st_rewind): Likewise.
(st_flush): Likewise.
* io/inquire.c: Add handling for asynchronous PENDING
and ID arguments.
* io/io.h (st_parameter_dt): Add async bit.
(st_parameter_wait): Correct.
(gfc_unit): Add au pointer.
(st_wait_async): Add prototype.
(transfer_array_inner): Likewise.
(st_write_done_worker): Likewise.
* io/open.c: Include async.h.
(new_unit): Initialize asynchronous unit.
* io/transfer.c (async_opt): New struct.
(wrap_scalar_transfer): New function.
(transfer_integer): Call wrap_scalar_transfer to do the work.
(transfer_real): Likewise.
(transfer_real_write): Likewise.
(transfer_character): Likewise.
(transfer_character_wide): Likewise.
(transfer_complex): Likewise.
(transfer_array_inner): New function.
(transfer_array): Call transfer_array_inner.
(transfer_derived): Call wrap_scalar_transfer.
(data_transfer_init): Check for asynchronous I/O.
Perform a wait operation on any pending asynchronous I/O
if the data transfer is synchronous. Copy PDT and enqueue
thread for data transfer.
(st_read_done_worker): New function.
(st_read_done): Enqueue transfer or call st_read_done_worker.
(st_write_done_worker): New function.
(st_write_done): Enqueue transfer or call st_read_done_worker.
(st_wait): Document as no-op for compatibility reasons.
(st_wait_async): New function.
* io/unit.c (insert_unit): Use macros LOCK, UNLOCK and TRYLOCK;
add NOTE where necessary.
(get_gfc_unit): Likewise.
(init_units): Likewise.
(close_unit_1): Likewise. Call async_close if asynchronous.
(close_unit): Use macros LOCK and UNLOCK.
(finish_last_advance_record): Likewise.
(newunit_alloc): Likewise.
* io/unix.c (find_file): Likewise.
(flush_all_units_1): Likewise.
(flush_all_units): Likewise.
* libgfortran.h (generate_error_common): Add prototype.
* runtime/error.c: Include io.h and async.h.
(generate_error_common): New function.

2018-07-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* testsuite/libgfomp.fortran/async_io_1.f90: New test.
* testsuite/libgfomp.fortran/async_io_2.f90: New test.
* testsuite/libgfomp.fortran/async_io_3.f90: New test.

[Rainer Orth]

Hi Thomas,

Post by Thomas König
the attached patch is the third take on Nicolas' and my patch
for implementing asynchronous I/O. Some parts have been reworked, and
several bugs which caused either incorrect I/O or hangs have been
fixed in the process.
I have to say that getting out these bugs has been much harder
than Nicolas and I originally thought, and that this has cost more
working hours than any other patch I have been involved in.
This has been regression-tested on x86_64-pc-linux-gnu. The new test
cases have also been tested in a tight loop with
n=1; while ./a.out; do echo -n $n " " ; n=$((n+1)); done
or (for async_io_3.f90, which is supposed to fail)
while true ; do ./a.out > /dev/null 2>&1 ; echo -n $n " " ; n=$((n+1));
done
and the test cases also come up clean with valgrind --tool=drd
(which is a _very_ strict tool which, after this experience, I
wholeheartedly recommend for doing pthreads debugging).
The interface remains as before - link in pthread to get asynchronous
I/O, which matches what ifort does.

another test run on i386-pc-solaris2.11 is underway. However, may
(all?) gfortran tests now SEGV. One example is

Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Backtrace for this error:
Segmentation Fault

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1 (LWP 1)]
0xfe1b1f03 in pthread_mutex_unlock () from /lib/libc.so.1
(gdb) where
#0 0xfe1b1f03 in pthread_mutex_unlock () from /lib/libc.so.1
#1 0xfe5d1b7c in __gthread_mutex_unlock (__mutex=0x18)
at ../libgcc/gthr-default.h:778
#2 _gfortran_st_rewind (fpp=0xfeffda9c)
at /vol/gcc/src/hg/trunk/solaris/libgfortran/io/file_pos.c:486
#3 0x0805110f in MAIN__ ()
at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gfortran.dg/backslash_2.f90:6

Obviously __mutex above hasn't been properly initialized.

Post by Thomas König
PR fortran/25829
* testsuite/libgfomp.fortran/async_io_1.f90: New test.
* testsuite/libgfomp.fortran/async_io_2.f90: New test.
* testsuite/libgfomp.fortran/async_io_3.f90: New test.

You seem to have a special fondness for libgfomp ;-)

Rainer

[Rainer Orth]

Hi Thomas,

Post by Rainer Orth
another test run on i386-pc-solaris2.11 is underway. However, may
(all?) gfortran tests now SEGV. One example is

the good news is: all three libgomp.fortran/async_io_?.f90 tests now
PASS for both 32 and 64-bit, as do gfortran.dg/f2003_inquire_1.f03 and
gfortran.dg/f2003_io_1.f03.

Rainer

[Thomas König]

Hi Rainer,

Post by Rainer Orth
However, may
(all?) gfortran tests now SEGV. One example is
Program received signal SIGSEGV: Segmentation fault - invalid memory reference.
Segmentation Fault
Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1 (LWP 1)]
0xfe1b1f03 in pthread_mutex_unlock () from /lib/libc.so.1
(gdb) where
#0 0xfe1b1f03 in pthread_mutex_unlock () from /lib/libc.so.1
#1 0xfe5d1b7c in __gthread_mutex_unlock (__mutex=0x18)
at ../libgcc/gthr-default.h:778
#2 _gfortran_st_rewind (fpp=0xfeffda9c)
at /vol/gcc/src/hg/trunk/solaris/libgfortran/io/file_pos.c:486
#3 0x0805110f in MAIN__ ()
at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gfortran.dg/backslash_2.f90:6

Ah, I see what was wrong.

The attached patch should fix this.

I have also attached a new test case which detects this error
even on Linux systems, plus a ChangeLog which fixes the typo :-)

Again regression-tested.

So, OK for trunk?

Regards

Thomas

2018-07-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* gfortran.texi: Add description of asynchronous I/O.
* trans-decl.c (gfc_finish_var_decl): Treat asynchronous variables
as volatile.
* trans-io.c (gfc_build_io_library_fndecls): Rename st_wait to
st_wait_async and change argument spec from ".X" to ".w".
(gfc_trans_wait): Pass ID argument via reference.

2018-07-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* gfortran.dg/f2003_inquire_1.f03: Add write statement.
* gfortran.dg/f2003_io_1.f03: Add wait statement.

2018-01-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* Makefile.am: Add async.c to gfor_io_src.
Add async.h to gfor_io_headers.
* Makefile.in: Regenerated.
* gfortran.map: Add _gfortran_st_wait_async.
* io/async.c: New file.
* io/async.h: New file.
* io/close.c: Include async.h.
(st_close): Call async_wait for an asynchronous unit.
* io/file_pos.c (st_backspace): Likewise.
(st_endfile): Likewise.
(st_rewind): Likewise.
(st_flush): Likewise.
* io/inquire.c: Add handling for asynchronous PENDING
and ID arguments.
* io/io.h (st_parameter_dt): Add async bit.
(st_parameter_wait): Correct.
(gfc_unit): Add au pointer.
(st_wait_async): Add prototype.
(transfer_array_inner): Likewise.
(st_write_done_worker): Likewise.
* io/open.c: Include async.h.
(new_unit): Initialize asynchronous unit.
* io/transfer.c (async_opt): New struct.
(wrap_scalar_transfer): New function.
(transfer_integer): Call wrap_scalar_transfer to do the work.
(transfer_real): Likewise.
(transfer_real_write): Likewise.
(transfer_character): Likewise.
(transfer_character_wide): Likewise.
(transfer_complex): Likewise.
(transfer_array_inner): New function.
(transfer_array): Call transfer_array_inner.
(transfer_derived): Call wrap_scalar_transfer.
(data_transfer_init): Check for asynchronous I/O.
Perform a wait operation on any pending asynchronous I/O
if the data transfer is synchronous. Copy PDT and enqueue
thread for data transfer.
(st_read_done_worker): New function.
(st_read_done): Enqueue transfer or call st_read_done_worker.
(st_write_done_worker): New function.
(st_write_done): Enqueue transfer or call st_read_done_worker.
(st_wait): Document as no-op for compatibility reasons.
(st_wait_async): New function.
* io/unit.c (insert_unit): Use macros LOCK, UNLOCK and TRYLOCK;
add NOTE where necessary.
(get_gfc_unit): Likewise.
(init_units): Likewise.
(close_unit_1): Likewise. Call async_close if asynchronous.
(close_unit): Use macros LOCK and UNLOCK.
(finish_last_advance_record): Likewise.
(newunit_alloc): Likewise.
* io/unix.c (find_file): Likewise.
(flush_all_units_1): Likewise.
(flush_all_units): Likewise.
* libgfortran.h (generate_error_common): Add prototype.
* runtime/error.c: Include io.h and async.h.
(generate_error_common): New function.

2018-07-02 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* testsuite/libgomp.fortran/async_io_1.f90: New test.
* testsuite/libgomp.fortran/async_io_2.f90: New test.
* testsuite/libgomp.fortran/async_io_3.f90: New test.
* testsuite/libgomp.fortran/async_io_4.f90: New test.

Post by Rainer Orth
Obviously __mutex above hasn't been properly initialized.

Post by Thomas König
PR fortran/25829
* testsuite/libgfomp.fortran/async_io_1.f90: New test.
* testsuite/libgfomp.fortran/async_io_2.f90: New test.
* testsuite/libgfomp.fortran/async_io_3.f90: New test.

You seem to have a special fondness for libgfomp ;-)
Rainer

[Rainer Orth]

Hi Thomas,

Post by Thomas König
Ah, I see what was wrong.
The attached patch should fix this.

it almost did the trick indeed. With the new patch, only two failures
remain:

FAIL: gfortran.dg/flush_1.f90 -O0 execution test
FAIL: gfortran.dg/flush_1.f90 -O1 execution test
FAIL: gfortran.dg/flush_1.f90 -O2 execution test
FAIL: gfortran.dg/flush_1.f90 -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions execution test
FAIL: gfortran.dg/flush_1.f90 -O3 -g execution test
FAIL: gfortran.dg/flush_1.f90 -Os execution test
FAIL: gfortran.dg/newunit_1.f90 -O0 execution test
FAIL: gfortran.dg/newunit_1.f90 -O1 execution test
FAIL: gfortran.dg/newunit_1.f90 -O2 execution test
FAIL: gfortran.dg/newunit_1.f90 -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions execution test
FAIL: gfortran.dg/newunit_1.f90 -O3 -g execution test
FAIL: gfortran.dg/newunit_1.f90 -Os execution test

Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Backtrace for this error:
Segmentation Fault

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1 (LWP 1)]
0xfe1b1773 in mutex_lock_impl () from /lib/libc.so.1
(gdb) where
#0 0xfe1b1773 in mutex_lock_impl () from /lib/libc.so.1
#1 0xfe1b1a03 in pthread_mutex_lock () from /lib/libc.so.1
#2 0xfe5d1da4 in __gthread_mutex_lock (__mutex=0x18)
at ../libgcc/gthr-default.h:748
#3 _gfortran_st_flush (fpp=0xfeffda40)
at /vol/gcc/src/hg/trunk/solaris/libgfortran/io/file_pos.c:514
#4 0x0805106f in flush_1 ()
at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gfortran.dg/flush_1.f90:11
#5 0x080512a7 in main (argc=1, argv=0xfeffdbfa)
at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gfortran.dg/flush_1.f90:28
#6 0x08050de6 in _start ()

I've not yet investigated why they only occur for 32-bit. I tried the
following snippet to fix them

[Thomas Koenig]

Hi Rainer,

thanks a lot for your testing! This is really making a lot
of difference.

This patch should hopefully fix what you have experienced.
I've left out the boilerplate. The tests that failed for you
will be added to the libgomp.fortran directory as async_io_5.f90,
async_io_6.f90 and async_io_7.f90.

Regards

Thomas

[Dominique d'Humières]

I have done a full regression testing with the patch at
https://gcc.gnu.org/ml/fortran/2018-07/msg00004.html + Rainer’s patch
at https://gcc.gnu.org/ml/fortran/2018-07/msg00007.html

I am currently testing the patch at
https://gcc.gnu.org/ml/fortran/2018-07/msg00008.html

so far, so good!

IMO the tests should go to gfortran.dg (they pass my tests).

I think there is a spurious STOP in libgomp.fortran/async_io_1.f90 and the last STOP 1 should be STOP 7, as in:

--- libgomp/testsuite/libgomp.fortran/async_io_1.f90 2018-07-03 12:26:41.000000000 +0200
+++ gcc/testsuite/gfortran.dg/asynchronous_6.f90 2018-07-05 12:57:35.000000000 +0200
@@ -21,7 +21,6 @@ program main
write (10,'(A)', asynchronous=yes) 'asdf'
write (10,*, asynchronous=yes) cc
close (10)
- stop
open (20, file='a.dat', asynchronous=yes)
read (20, *, asynchronous=yes) i, j
read (20, *, asynchronous=yes) k, l
@@ -42,6 +41,6 @@ program main
open(20, file='c.dat', asynchronous=yes)
read(20, *, asynchronous=yes) res
wait (20)
- if (any(res /= is)) stop 1
+ if (any(res /= is)) stop 7
close (20,status="delete")
end program

Thanks for the hard work!

Dominique

[Thomas Koenig]

Hi everybody,

Post by Dominique d'Humières
I am currently testing the patch at
https://gcc.gnu.org/ml/fortran/2018-07/msg00008.html
so far, so good!
IMO the tests should go to gfortran.dg (they pass my tests).

I put the asycn_io_*.f90 tests into libgomp.fortran because,
under Linux, gfortran.dg does not link in pthreads, so the
tests would not be executed in parallel, and some of them
would fail.

So, here is the final version. I would really like to get this
into trunk, and out of the way, so Nicolas and I can focus on
other things.

So, OK?

Regards

Thomas

2018-07-15 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* gfortran.texi: Add description of asynchronous I/O.
* trans-decl.c (gfc_finish_var_decl): Treat asynchronous variables
as volatile.
* trans-io.c (gfc_build_io_library_fndecls): Rename st_wait to
st_wait_async and change argument spec from ".X" to ".w".
(gfc_trans_wait): Pass ID argument via reference.

2018-07-15 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* gfortran.dg/f2003_inquire_1.f03: Add write statement.
* gfortran.dg/f2003_io_1.f03: Add wait statement.

2018-01-15 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* Makefile.am: Add async.c to gfor_io_src.
Add async.h to gfor_io_headers.
* Makefile.in: Regenerated.
* gfortran.map: Add _gfortran_st_wait_async.
* io/async.c: New file.
* io/async.h: New file.
* io/close.c: Include async.h.
(st_close): Call async_wait for an asynchronous unit.
* io/file_pos.c (st_backspace): Likewise.
(st_endfile): Likewise.
(st_rewind): Likewise.
(st_flush): Likewise.
* io/inquire.c: Add handling for asynchronous PENDING
and ID arguments.
* io/io.h (st_parameter_dt): Add async bit.
(st_parameter_wait): Correct.
(gfc_unit): Add au pointer.
(st_wait_async): Add prototype.
(transfer_array_inner): Likewise.
(st_write_done_worker): Likewise.
* io/open.c: Include async.h.
(new_unit): Initialize asynchronous unit.
* io/transfer.c (async_opt): New struct.
(wrap_scalar_transfer): New function.
(transfer_integer): Call wrap_scalar_transfer to do the work.
(transfer_real): Likewise.
(transfer_real_write): Likewise.
(transfer_character): Likewise.
(transfer_character_wide): Likewise.
(transfer_complex): Likewise.
(transfer_array_inner): New function.
(transfer_array): Call transfer_array_inner.
(transfer_derived): Call wrap_scalar_transfer.
(data_transfer_init): Check for asynchronous I/O.
Perform a wait operation on any pending asynchronous I/O
if the data transfer is synchronous. Copy PDT and enqueue
thread for data transfer.
(st_read_done_worker): New function.
(st_read_done): Enqueue transfer or call st_read_done_worker.
(st_write_done_worker): New function.
(st_write_done): Enqueue transfer or call st_read_done_worker.
(st_wait): Document as no-op for compatibility reasons.
(st_wait_async): New function.
* io/unit.c (insert_unit): Use macros LOCK, UNLOCK and TRYLOCK;
add NOTE where necessary.
(get_gfc_unit): Likewise.
(init_units): Likewise.
(close_unit_1): Likewise. Call async_close if asynchronous.
(close_unit): Use macros LOCK and UNLOCK.
(finish_last_advance_record): Likewise.
(newunit_alloc): Likewise.
* io/unix.c (find_file): Likewise.
(flush_all_units_1): Likewise.
(flush_all_units): Likewise.
* libgfortran.h (generate_error_common): Add prototype.
* runtime/error.c: Include io.h and async.h.
(generate_error_common): New function.

2018-07-15 Nicolas Koenig <***@gcc.gnu.org>
Thomas Koenig <***@gcc.gnu.org>

PR fortran/25829
* testsuite/libgomp.fortran/async_io_1.f90: New test.
* testsuite/libgomp.fortran/async_io_2.f90: New test.
* testsuite/libgomp.fortran/async_io_3.f90: New test.
* testsuite/libgomp.fortran/async_io_4.f90: New test.
* testsuite/libgomp.fortran/async_io_5.f90: New test.
* testsuite/libgomp.fortran/async_io_6.f90: New test.
* testsuite/libgomp.fortran/async_io_7.f90: New test.

[Rainer Orth]

Hi Thomas,

Post by Thomas Koenig

Post by Dominique d'Humières
I am currently testing the patch at
https://gcc.gnu.org/ml/fortran/2018-07/msg00008.html
so far, so good!
IMO the tests should go to gfortran.dg (they pass my tests).

I put the asycn_io_*.f90 tests into libgomp.fortran because,
under Linux, gfortran.dg does not link in pthreads, so the
tests would not be executed in parallel, and some of them
would fail.
So, here is the final version. I would really like to get this
into trunk, and out of the way, so Nicolas and I can focus on
other things.

I've now regtested the patch on i386-pc-solaris2.11 and
sparc-sun-solaris2.11: no regressions and the new tests all PASS.

However, I still don't understand why you insist on the hack with
putting the async_io_*.f90 tests into the libgomp testsuite. Why not
just make the pthread requirement explicit with

{ dg-require-effective-target pthread }
{ dg-additional-options "-pthread" }

and put them in gfortran.dg where they belong?

Rainer

[Thomas Koenig]

Hi Rainer,

Post by Rainer Orth
I've now regtested the patch on i386-pc-solaris2.11 and
sparc-sun-solaris2.11: no regressions and the new tests all PASS.

Thanks, that is good news!

Post by Rainer Orth
However, I still don't understand why you insist on the hack with
putting the async_io_*.f90 tests into the libgomp testsuite. Why not
just make the pthread requirement explicit with
{ dg-require-effective-target pthread }
{ dg-additional-options "-pthread" }
and put them in gfortran.dg where they belong?

Because this does not appear to work with Linux. I, like
most gfortran developers, work on Linux, and I would like to
catch any failure during regression-testing on my own system,
if possible.

We have had this discussion with Jakub, and he advised
us to put all the stuff requiring pthreads into libgomp.

It is debatable if this is a good thing, or if we should
at least make one round of tests with -pthread enabled.
However, this is something for the future, and requires knowledge
of dejagnu that I don't currently have :-)

Regards

Thomas

[Rainer Orth]

Hi Thomas,

Post by Thomas Koenig

Post by Rainer Orth
However, I still don't understand why you insist on the hack with
putting the async_io_*.f90 tests into the libgomp testsuite. Why not
just make the pthread requirement explicit with
{ dg-require-effective-target pthread }
{ dg-additional-options "-pthread" }
and put them in gfortran.dg where they belong?

Because this does not appear to work with Linux. I, like
most gfortran developers, work on Linux, and I would like to
catch any failure during regression-testing on my own system,
if possible.

huh, what doesn't work? I've just finished an x86_64-pc-linux-gnu
bootstrap with your patch included, added the above to the
async_io_?.f90 tests, linked them to gfortran.dg and ran the tests there
(both 32 and 64-bit multilibs), all PASSed and I verified that they were
linked with -lpthread.

Post by Thomas Koenig
We have had this discussion with Jakub, and he advised
us to put all the stuff requiring pthreads into libgomp.

Do you have a pointer to that previous discussion?

Post by Thomas Koenig
It is debatable if this is a good thing, or if we should
at least make one round of tests with -pthread enabled.
However, this is something for the future, and requires knowledge
of dejagnu that I don't currently have :-)

First of all, we need to see and understand the failure mode, if any.
Making this work with the testsuite is a secondary matter only, and I
can certainly help with that if necessary.

Rainer

[Jerry DeLisle]

Post by Rainer Orth
Hi Thomas,

Post by Thomas Koenig

Post by Rainer Orth
However, I still don't understand why you insist on the hack with
putting the async_io_*.f90 tests into the libgomp testsuite. Why not
just make the pthread requirement explicit with
{ dg-require-effective-target pthread }
{ dg-additional-options "-pthread" }
and put them in gfortran.dg where they belong?

Because this does not appear to work with Linux. I, like
most gfortran developers, work on Linux, and I would like to
catch any failure during regression-testing on my own system,
if possible.

huh, what doesn't work? I've just finished an x86_64-pc-linux-gnu
bootstrap with your patch included, added the above to the
async_io_?.f90 tests, linked them to gfortran.dg and ran the tests there
(both 32 and 64-bit multilibs), all PASSed and I verified that they were
linked with -lpthread.

Post by Thomas Koenig
We have had this discussion with Jakub, and he advised
us to put all the stuff requiring pthreads into libgomp.

Do you have a pointer to that previous discussion?

Post by Thomas Koenig
It is debatable if this is a good thing, or if we should
at least make one round of tests with -pthread enabled.
However, this is something for the future, and requires knowledge
of dejagnu that I don't currently have :-)

First of all, we need to see and understand the failure mode, if any.
Making this work with the testsuite is a secondary matter only, and I
can certainly help with that if necessary.
Rainer

Hmm, interesting. Which linux are you using?

I will try it here as well.

Jerry

[Rainer Orth]

Hi Jerry,

Post by Jerry DeLisle
Hmm, interesting. Which linux are you using?

Fedora 27.

Rainer

[Jerry DeLisle]

Post by Rainer Orth
Hi Jerry,

Post by Jerry DeLisle
Hmm, interesting. Which linux are you using?

Fedora 27.
Rainer

Works for me. Fedora 28. Do not know for other OS's

Jerry

[Rainer Orth]

Hi Jerry,

Post by Jerry DeLisle

Post by Rainer Orth
Hi Jerry,

Post by Jerry DeLisle
Hmm, interesting. Which linux are you using?

Fedora 27.

Works for me. Fedora 28. Do not know for other OS's

just tried Solaris 11/x86: works just as well. I could try Mac OS X
10.7, but that build would take quite a while...

Rainer

[Dominique d'Humières]

Post by Rainer Orth
Hi Jerry,

Post by Jerry DeLisle

Post by Rainer Orth
Hi Jerry,

Post by Jerry DeLisle
Hmm, interesting. Which linux are you using?

Fedora 27.

Works for me. Fedora 28. Do not know for other OS's

just tried Solaris 11/x86: works just as well. I could try Mac OS X
10.7, but that build would take quite a while…

Works on OSX.

Dominique

Post by Rainer Orth
Rainer
--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

[Thomas Koenig]

Post by Rainer Orth

Post by Thomas Koenig
Because this does not appear to work with Linux. I, like
most gfortran developers, work on Linux, and I would like to
catch any failure during regression-testing on my own system,
if possible.

huh, what doesn't work? I've just finished an x86_64-pc-linux-gnu
bootstrap with your patch included, added the above to the
async_io_?.f90 tests, linked them to gfortran.dg and ran the tests there
(both 32 and 64-bit multilibs), all PASSed and I verified that they were
linked with -lpthread.

Post by Thomas Koenig
We have had this discussion with Jakub, and he advised
us to put all the stuff requiring pthreads into libgomp.

Do you have a pointer to that previous discussion?

https://gcc.gnu.org/ml/fortran/2018-04/msg00048.html is what I based
my recollection on.

Regards

Thomas

[Jerry DeLisle]

Post by Rainer Orth
Hi Thomas,

Post by Thomas Koenig

Post by Dominique d'Humières
I am currently testing the patch at
https://gcc.gnu.org/ml/fortran/2018-07/msg00008.html
so far, so good!
IMO the tests should go to gfortran.dg (they pass my tests).

I put the asycn_io_*.f90 tests into libgomp.fortran because,
under Linux, gfortran.dg does not link in pthreads, so the
tests would not be executed in parallel, and some of them
would fail.
So, here is the final version. I would really like to get this
into trunk, and out of the way, so Nicolas and I can focus on
other things.

I've now regtested the patch on i386-pc-solaris2.11 and
sparc-sun-solaris2.11: no regressions and the new tests all PASS.
However, I still don't understand why you insist on the hack with
putting the async_io_*.f90 tests into the libgomp testsuite. Why not
just make the pthread requirement explicit with
{ dg-require-effective-target pthread }
{ dg-additional-options "-pthread" }
and put them in gfortran.dg where they belong?
Rainer

Rainer, your point well understood. However, I would like to get the
patch in and out to the world to get exercised by users so we can catch
anything we miss. We can work on the gfortran.dg incantations as a
follow-on effort. (maybe a separate PR for it)

Jerry

[Jerry DeLisle]

Post by Thomas Koenig
Hi everybody,

Post by Dominique d'Humières
I am currently testing the patch at
https://gcc.gnu.org/ml/fortran/2018-07/msg00008.html
so far, so good!
IMO the tests should go to gfortran.dg (they pass my tests).

I put the asycn_io_*.f90 tests into libgomp.fortran because,
under Linux, gfortran.dg does not link in pthreads, so the
tests would not be executed in parallel, and some of them
would fail.
So, here is the final version. I would really like to get this
into trunk, and out of the way, so Nicolas and I can focus on
other things.
So, OK?

OK, Go for it.

Jerry

PS and big kudos for all the effort. This is a major improvement!

[Bernhard Reutner-Fischer]

Post by Thomas Koenig
So, here is the final version. I would really like to get this
into trunk, and out of the way, so Nicolas and I can focus on
other things.
So, OK?

[I know i'm late as it was already applied]

For me it would be easier to read the locking if struct async_unit had
it's queue_lock named q_lock/qlock instead of plain lock.
The io_lock is named nicely already.

Furthermore there is a mixture of (correctly wrapped) __gthread_ in
struct adv_cond versus unwrapped pthread_mutex_t in struct async_unit
where i'd have expected the latter to also use the __gthread wrappers.

struct adv_cond member pending should not be an int but an unsigned
int or, even better, a bool, i'd say.

transfer_array_inner () is named unintuitively IMHO. Maybe
transfer_array_now, transfer_array_scalar or transfer_array_1.

Index: libgfortran/io/async.c
===================================================================
--- libgfortran/io/async.c (nicht existent)
+++ libgfortran/io/async.c (Arbeitskopie)
[]
+static void
+update_pdt (st_parameter_dt **old, st_parameter_dt *new) {
+ st_parameter_dt *temp;
+ NOTE ("Changing pdts, current_unit = %p", (void *) (new->u.p.current_unit));
+ temp = *old;
+ *old = new;
+ if (temp)
+ free (temp);
+}

free (NULL) is perfectly valid, please remove the if.


+static void *
+async_io (void *arg)
+{
[]
+ while (true)
+ {
+ /* Main loop. At this point, au->lock is always held. */

dot space space at the end of a sentence please.
[]
+ while (ctq)
+ {
+ if (prev)
+ free (prev);

Likewise, drop if.

+ prev = ctq;
+ if (!au->error.has_error)

I'd flag that as likely.

Likewise, i'd flag finish_thread as unlikely; Being a label you can
hint the predictor that it's rather unlikely jumped to by flagging it
cold:

finish_thread: __attribute__((cold));

+/* Initialize an asyncronous unit, returning zero on success,
+ nonzero on failure. It also sets u->au. */
+
+void
+init_async_unit (gfc_unit *u)

s/asyncronous/asynchronous/

+{
+ async_unit *au;
+ if (!__gthread_active_p ())
+ {
+ u->au = NULL;
+ return;
+ }
+

Excess horizontal space on the empty line above.

+ au = (async_unit *) xmalloc (sizeof (async_unit));

I'd XCNEW (async_unit) and omit all those NULL and 0 stores.
You should use the scalar allocators provided in include/libiberty.h
throughout, so s/xmalloc/XNEW/ and s/free/XDELETE/ and so on.

+/* Enqueue a transfer statement. */
+
+void
+enqueue_transfer (async_unit *au, transfer_args *arg, enum aio_do type)
+{
+ transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+ tq->arg = *arg;

boom on OOM. XCNEW (transfer_queue), please.

+ tq->arg = *arg;
+ tq->type = type;
+ tq->has_id = 0;

redundant store to has_id.

+ LOCK (&au->lock);
+ if (!au->tail)
+ au->head = tq;
+ else
+ au->tail->next = tq;
+ au->tail = tq;
+ REVOKE_SIGNAL (&(au->emptysignal));
+ au->empty = false;
+ UNLOCK (&au->lock);
+ SIGNAL (&au->work);
+}

+/* Enqueue an st_write_done or st_read_done which contains an ID. */
+
+int
+enqueue_done_id (async_unit *au, enum aio_do type)
+{
+ int ret;
+ transfer_queue *tq = calloc (sizeof (transfer_queue), 1);

XCNEW.

+/* Enqueue an st_write_done or st_read_done without an ID. */
+
+void
+enqueue_done (async_unit *au, enum aio_do type)
+{
+ transfer_queue *tq = calloc (sizeof (transfer_queue), 1);

XCNEW.

+ tq->type = type;
+ tq->has_id = 0;

Redundant store to has_id. Maybe just comment it out if you do want to
emphasis this side-effect of zeroing.

/* tq->has_id = 0; already done by XCNEW */
or the like.

+/* Enqueue a CLOSE statement. */
+
+void
+enqueue_close (async_unit *au)
+{
+ transfer_queue *tq = calloc (sizeof (transfer_queue), 1);

XCNEW.

And i think enqueue_close does not need internal_proto but could be
and should be static.
Or, even better, remove it completely and call
enqueue_done (au, AIO_CLOSE)
in async_close directly.

+/* The asynchronous unit keeps the currently active PDT around.
+ This function changes that to the current one. */
+
+void
+enqueue_data_transfer_init (async_unit *au, st_parameter_dt *dt, int read_flag)
+{
+ st_parameter_dt *new = xmalloc (sizeof (st_parameter_dt));

XNEW (st_parameter_dt);

+ transfer_queue *tq = xmalloc (sizeof (transfer_queue));

XNEW (transfer_queue);
+
+ memcpy ((void *) new, (void *) dt, sizeof (st_parameter_dt));
+
+ NOTE ("dt->internal_unit_desc = %p", dt->internal_unit_desc);
+ NOTE ("common.flags & mask = %d", dt->common.flags & IOPARM_LIBRETURN_MASK);
+ tq->next = NULL;
+ tq->type = AIO_DATA_TRANSFER_INIT;
+ tq->read_flag = read_flag;
+ tq->has_id = 0;

ah, that should be bool, not _Bool and hence s/0/false/ and s/1/true/
here and elsewhere when storing to has_id.

since read_flag seems to be a boolean too, i'd
unsigned has_id : 1;
unsiged read_flag : 1;
fwiw.

+ tq->new_pdt = new;
+ LOCK (&au->lock);
+
+ if (!au->tail)
+ au->head = tq;
+ else
+ au->tail->next = tq;
+ au->tail = tq;
+ REVOKE_SIGNAL (&(au->emptysignal));
+ au->empty = 0;

s/0/false/ please.

+ UNLOCK (&au->lock);
+ SIGNAL (&au->work);
+}

+/* Perform a wait operation on an asynchronous unit with an ID specified,
+ which means collecting the errors that may have happened asynchronously.
+ Return true if an error has been encountered. */
+
+bool
+async_wait_id (st_parameter_common *cmp, async_unit *au, int i)

I'd rename the parameter i to id for clarity.

+ WAIT_SIGNAL_MUTEX (&(au->id.done),
+ (au->id.low >= au->id.waiting || au->empty), &au->lock);

I'd test au->empty first.

Not sure why it's ok to clear has_error in collect_async_errors --
especially without locking -- but i guess you tested such async
failure conditions in both async_wait_id and async_wait.


Index: libgfortran/io/async.h
===================================================================
--- libgfortran/io/async.h (nicht existent)
+++ libgfortran/io/async.h (Arbeitskopie)
@@ -0,0 +1,378 @@

+/* Thread - local storage of the current unit we are looking at. Needed for
+ error reporting. */

dot space space at the end of a sentence.

The layout of struct async_unit does not look too good on a 64bit box.

+bool collect_async_errors (st_parameter_common *, async_unit *);
+internal_proto (collect_async_errors);

superfluous trailing space

Index: libgfortran/io/file_pos.c
===================================================================
--- libgfortran/io/file_pos.c (Revision 259739)
+++ libgfortran/io/file_pos.c (Arbeitskopie)

@@ -267,8 +280,13 @@ st_backspace (st_parameter_filepos *fpp)

done:
if (u != NULL)
- unlock_unit (u);
+ {
+ unlock_unit (u);

+ if (u->au && needs_unlock)
+ UNLOCK (&u->au->io_lock);
+ }
+
library_end ();
}

in st_backspace you first unlock the unit and only afterwards unlock
the async io_lock.
I would settle on first unlocking the async io_lock and only then
unlocking the unit, no?

@@ -376,9 +406,12 @@ st_endfile (st_parameter_filepos *fpp)
}
}

- done:
- unlock_unit (u);
+ done:
+ if (u->au && needs_unlock)
+ UNLOCK (&u->au->io_lock);

+ unlock_unit (u);
+
library_end ();
}

like you do here, in st_endfile.

Here in st_endfile, why do you async_wait before the
LIBERROR_OPTION_CONFLICT handling by
if (u->flags.access == ACCESS_SEQUENTIAL
&& u->endfile == AFTER_ENDFILE)
and not afterwards?

@@ -450,6 +499,7 @@ void
st_flush (st_parameter_filepos *fpp)
{
gfc_unit *u;
+ bool needs_unlock = false;

library_start (&fpp->common);

@@ -456,6 +506,17 @@ st_flush (st_parameter_filepos *fpp)
u = find_unit (fpp->common.unit);
if (u != NULL)
{
+ if (u->au)
+ {
+ if (async_wait (&(fpp->common), u->au))
+ return;
+ else
+ {
+ needs_unlock = true;
+ LOCK (&u->au->io_lock);
+ }
+ }
+
/* Make sure format buffer is flushed. */
if (u->flags.form == FORM_FORMATTED)
fbuf_flush (u, u->mode);
@@ -469,5 +530,8 @@ st_flush (st_parameter_filepos *fpp)
generate_error (&fpp->common, LIBERROR_BAD_OPTION,
"Specified UNIT in FLUSH is not connected");

+ if (needs_unlock)
+ UNLOCK (&u->au->io_lock);

I would change the condition to if (ASYNC_IO && needs_unlock) for consistency.

+
library_end ();
}
Index: libgfortran/io/inquire.c
===================================================================
--- libgfortran/io/inquire.c (Revision 259739)
+++ libgfortran/io/inquire.c (Arbeitskopie)
@@ -26,6 +26,7 @@ see the files COPYING3 and COPYING.RUNTIME respect
/* Implement the non-IOLENGTH variant of the INQUIRY statement */

#include "io.h"
+#include "async.h"
#include "unix.h"
#include <string.h>

please include async.h *after* unix.h like you do everwhere else.

Index: libgfortran/io/read.c
===================================================================
--- libgfortran/io/read.c (Revision 259739)
+++ libgfortran/io/read.c (Arbeitskopie)
@@ -30,6 +30,7 @@ see the files COPYING3 and COPYING.RUNTIME respect
#include <string.h>
#include <ctype.h>
#include <assert.h>
+#include "async.h"

typedef unsigned char uchar;

@@ -42,6 +43,7 @@ typedef unsigned char uchar;
void
set_integer (void *dest, GFC_INTEGER_LARGEST value, int length)
{
+ NOTE ("set_integer: %lld %p", (long long int) value, dest);
switch (length)
{
#ifdef HAVE_GFC_INTEGER_16

Debugging leftover? Please remove the include and the NOTE.

--- libgfortran/io/transfer.c (Revision 259739)
+++ libgfortran/io/transfer.c (Arbeitskopie)

+/* Wrapper function for I/O of scalar types. If this should be an async I/O
+ request, queue it. For a synchronous write on an async unit, perform the
+ wait operation and return an error. For all synchronous writes, call the
+ right transfer function. */

+static void
+wrap_scalar_transfer (st_parameter_dt *dtp, bt type, void *p, int kind,
+ size_t size, size_t n_elem)
+{
+ if (dtp->u.p.current_unit && dtp->u.p.current_unit->au)
+ {
+ if (dtp->u.p.async)
+ {

Please move this second nested if into the first one.

+ transfer_args args;
+ args.scalar.transfer = dtp->u.p.transfer;
+ args.scalar.arg_bt = type;
+ args.scalar.data = p;
+ args.scalar.i = kind;
+ args.scalar.s1 = size;
+ args.scalar.s2 = n_elem;
+ enqueue_transfer (dtp->u.p.current_unit->au, &args,
+ AIO_TRANSFER_SCALAR);
+ return;
+ }
+ }

void
+transfer_array (st_parameter_dt *dtp, gfc_array_char *desc, int kind,
+ gfc_charlen_type charlen)
+{
+ if ((dtp->common.flags & IOPARM_LIBRETURN_MASK) != IOPARM_LIBRETURN_OK)
+ return;
+
+ if (dtp->u.p.current_unit && dtp->u.p.current_unit->au)
+ {
+ if (dtp->u.p.async)
+ {

Likewise:
Please move this second nested if into the first one.

+ transfer_args args;
+ size_t sz = sizeof (gfc_array_char)
+ + sizeof (descriptor_dimension)
+ * GFC_DESCRIPTOR_RANK (desc);
+ args.array.desc = xmalloc (sz);
+ NOTE ("desc = %p", (void *) args.array.desc);
+ memcpy (args.array.desc, desc, sz);
+ args.array.kind = kind;
+ args.array.charlen = charlen;
+ enqueue_transfer (dtp->u.p.current_unit->au, &args,
+ AIO_TRANSFER_ARRAY);
+ return;
+ }
+ }
+ /* Come here if there was no asynchronous I/O to be scheduled. */
+ transfer_array_inner (dtp, desc, kind, charlen);
+}

@@ -2770,6 +2839,42 @@ data_transfer_init (st_parameter_dt *dtp, int read
else if (dtp->u.p.current_unit->internal_unit_kind > 0)
dtp->u.p.unit_is_internal = 1;

+ if ((cf & IOPARM_DT_HAS_ASYNCHRONOUS) != 0)
+ {
+ int f;
+ f = find_option (&dtp->common, dtp->asynchronous, dtp->asynchronous_len,
+ async_opt, "Bad ASYNCHRONOUS in data transfer "
+ "statement");
+ if (f == ASYNC_YES && dtp->u.p.current_unit->flags.async != ASYNC_YES)
+ {
+ generate_error (&dtp->common, LIBERROR_OPTION_CONFLICT,
+ "ASYNCHRONOUS transfer without "
+ "ASYHCRONOUS='YES' in OPEN");

s/ASYHCRONOUS/ASYNCHRONOUS/

+ return;
+ }
+ dtp->u.p.async = f == ASYNC_YES;
+ }
[]
+void
+data_transfer_init_worker (st_parameter_dt *dtp, int read_flag)

Missing function comment.

+{
+ GFC_INTEGER_4 cf = dtp->common.flags;
+
+ NOTE ("starting worker...");
+
+ if (read_flag && dtp->u.p.current_unit->flags.form != FORM_UNFORMATTED
+ && ((cf & IOPARM_DT_LIST_FORMAT) != 0)

Excess braces

+ && dtp->u.p.current_unit->child_dtio == 0)

Surplus horizontal whitespace before ==

+ dtp->u.p.current_unit->last_char = EOF - 1;
[]
+void
+st_read_done (st_parameter_dt *dtp)
+{
+ if (dtp->u.p.current_unit)
+ {
+ if (dtp->u.p.current_unit->au)
+ {
+ if (dtp->common.flags & IOPARM_DT_HAS_ID)
+ *dtp->id = enqueue_done_id (dtp->u.p.current_unit->au,
AIO_READ_DONE);

Surplus trailing whitespace.

+ else
+ {
+ enqueue_done (dtp->u.p.current_unit->au, AIO_READ_DONE);
+ /* An asynchronous unit without ASYNCHRONOUS="YES" - make this
+ synchronous by performing a wait operation. */
+ if (!dtp->u.p.async)
+ async_wait (&dtp->common, dtp->u.p.current_unit->au);

Don't we have to honour handled errors from async_wait?
Same for st_write_done ().

+ }
+ }
+ else
+ st_read_done_worker (dtp);

--- libgfortran/io/unit.c (Revision 259739)
+++ libgfortran/io/unit.c (Arbeitskopie)

@@ -922,7 +930,7 @@ newunit_alloc (void)
memset (newunits + old_size, 0, old_size);
newunits[old_size] = true;
newunit_lwi = old_size + 1;
- __gthread_mutex_unlock (&unit_lock);
+ UNLOCK (&unit_lock);

This should be indented by 2 spaces, not 4.

return -old_size + NEWUNIT_START;
}


--- libgfortran/libgfortran.h (Revision 259739)
+++ libgfortran/libgfortran.h (Arbeitskopie)
@@ -743,6 +743,9 @@ internal_proto(translate_error);
extern void generate_error (st_parameter_common *, int, const char *);
iexport_proto(generate_error);

+extern bool generate_error_common (st_parameter_common *, int, const char *);
+iexport_proto(generate_error_common);

why is that exported and not just internal_proto() ?
+
extern void generate_warning (st_parameter_common *, const char *);
internal_proto(generate_warning);


@@ -1748,5 +1751,7 @@ void cshift1_16_c16 (gfc_array_c16 * const restric
internal_proto(cshift1_16_c16);
#endif

+/* Define this if we support asynchronous I/O on this platform. This
+ currently requires weak symbols. */

#endif /* LIBGFOR_H */

obsolete comment, please remove.

Thanks for the async support!
cheers,

[Janne Blomqvist]

On Mon, Sep 3, 2018 at 1:39 AM Bernhard Reutner-Fischer <

Post by Bernhard Reutner-Fischer
+ au = (async_unit *) xmalloc (sizeof (async_unit));
I'd XCNEW (async_unit) and omit all those NULL and 0 stores.
You should use the scalar allocators provided in include/libiberty.h
throughout, so s/xmalloc/XNEW/ and s/free/XDELETE/ and so on.

libgfortran uses it's own allocator wrappers in
libgfortran/runtime/memory.c.

Perhaps there is a case for switching to use these macros instead (why???),
but if so, IMHO that should be done as a separate patch, also fixing all
the other uses of the allocator wrappers.

Post by Bernhard Reutner-Fischer
+/* Enqueue a transfer statement. */
+
+void
+enqueue_transfer (async_unit *au, transfer_args *arg, enum aio_do type)
+{
+ transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+ tq->arg = *arg;
boom on OOM. XCNEW (transfer_queue), please.

xcalloc, rather.

[Bernhard Reutner-Fischer]

Post by Bernhard Reutner-Fischer
+ au = (async_unit *) xmalloc (sizeof (async_unit));
I'd XCNEW (async_unit) and omit all those NULL and 0 stores.
You should use the scalar allocators provided in include/libiberty.h
throughout, so s/xmalloc/XNEW/ and s/free/XDELETE/ and so on.

libgfortran uses it's own allocator wrappers in libgfortran/runtime/memory.c.
Perhaps there is a case for switching to use these macros instead (why???), but if so, IMHO that should be done as a separate patch, also fixing all the other uses of the allocator wrappers.

The macros are shorter to type. Furthermore we use them in the
frontend already so maybe using them also in the runtime too would be
easier to remember.
As to eventually doing so in a separate patch, that's fair enough.

Post by Bernhard Reutner-Fischer
+/* Enqueue a transfer statement. */
+
+void
+enqueue_transfer (async_unit *au, transfer_args *arg, enum aio_do type)
+{
+ transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+ tq->arg = *arg;
boom on OOM. XCNEW (transfer_queue), please.

xcalloc, rather.

right.

thanks,