public inbox for fortran@gcc.gnu.org
 help / color / mirror / Atom feed
* [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
@ 2024-04-04  1:33 Jerry D
  2024-04-04  8:17 ` Paul Richard Thomas
  2024-04-04  9:31 ` Tobias Burnus
  0 siblings, 2 replies; 11+ messages in thread
From: Jerry D @ 2024-04-04  1:33 UTC (permalink / raw)
  To: gfortran; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

Hi all,

The attached log entry and patch (git show) fixes this issue by adding 
logic to handle spaces in eat_separators. One or more spaces by 
themselves are a valid separator. So in this case we look at the 
character following the spaces to see if it is a comma or semicolon.

If so, I change it to the valid separator for the given decimal mode, 
point or comma. This allows the comma or semicolon to be interpreted as 
a null read on the next effective item in the formatted read.

I chose a permissive approach here that allows reads to proceed when the
input line is mal-formed with an incorrect separator as long as there is 
at least one space in front of it.

New test case included. Regression tested on X86-64.

OK for trunk?  Backport to 13 after some time.

Regards,

Jerry

[-- Attachment #2: submit.diff --]
[-- Type: text/x-patch, Size: 2961 bytes --]

commit 7d1a958d6b099ea88b6c51649baf5dbd5e598909
Author: Jerry DeLisle <jvdelisle@gcc.gnu.org>
Date:   Wed Apr 3 18:07:30 2024 -0700

    libfortran: Fix handling of formatted separators.
    
            PR libfortran/114304
    
    libgfortran/ChangeLog:
    
            * io/list_read.c (eat_separator): Add logic to handle spaces
            preceding a comma or semicolon such that that a 'null' read
            occurs without error at the end of comma or semicolon
            terminated input lines.
    
    gcc/testsuite/ChangeLog:
    
            * gfortran.dg/pr114304.f90: New test.

diff --git a/gcc/testsuite/gfortran.dg/pr114304.f90 b/gcc/testsuite/gfortran.dg/pr114304.f90
new file mode 100644
index 00000000000..57af619246b
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr114304.f90
@@ -0,0 +1,49 @@
+! { dg-do run }
+! pr114304
+real :: x(3)
+character(len=1) :: s
+integer :: ios
+
+s = 'x'
+
+open(99, decimal="comma", status='scratch')
+write(99, '(a)') '1,23435 1243,24 13,24 a'
+rewind(99)
+read(99, *, iostat=ios) x, s
+if (ios /= 0 .or. s /= 'a') stop 1
+
+rewind(99)
+write(99, '(a)') '1,23435;1243,24;13,24;a'
+rewind(99)
+read(99, *, iostat=ios) x, s
+if (ios /= 0 .or. s /= 'a') stop 2
+
+! Note: not reading 's'
+rewind(99)
+write(99, '(a)') '1,23435 1243,24 13,24 ,'
+rewind(99)
+read(99, *) x
+if (ios /= 0) stop 3
+
+rewind(99)
+write(99, '(a)') '1,23435;1243,24;13,24 ,'
+rewind(99)
+read(99, *, iostat=ios) x
+if (ios /= 0) stop 4
+
+! Now reading 's'
+s = 'w'
+rewind(99)
+write(99, '(a)') '1,23435 1243,24 13,24 ,'
+rewind(99)
+read(99, *, iostat=ios) x, s
+if (ios /= 0 .or. s /= 'w') stop 5
+
+s = 'w'
+rewind(99)
+write(99, '(a)') '1,23435;1243,24;13,24 ,'
+rewind(99)
+read(99, *, iostat=ios) x, s
+if (ios /= 0 .or. s /= 'w') stop 6
+close(99)
+end
diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index fb3f7dbc34d..f6f169043bf 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -461,11 +461,30 @@ eat_separator (st_parameter_dt *dtp)
   int c, n;
   int err = 0;
 
-  eat_spaces (dtp);
   dtp->u.p.comma_flag = 0;
+  c = next_char (dtp);
+  if (c == ' ')
+    {
+      eat_spaces (dtp);
+      c = next_char (dtp);
+      if (c == ',')
+	{
+	  if (dtp->u.p.current_unit->decimal_status == DECIMAL_COMMA)
+	    unget_char (dtp, ';');
+	  dtp->u.p.comma_flag = 1;
+	  eat_spaces (dtp);
+	  return err;
+	}
+      if (c == ';')
+	{
+	  if (dtp->u.p.current_unit->decimal_status == DECIMAL_POINT)
+	    unget_char (dtp, ',');
+	  dtp->u.p.comma_flag = 1;
+	  eat_spaces (dtp);
+	  return err;
+	}
+    }
 
-  if ((c = next_char (dtp)) == EOF)
-    return LIBERROR_END;
   switch (c)
     {
     case ',':
@@ -476,7 +495,10 @@ eat_separator (st_parameter_dt *dtp)
 	  unget_char (dtp, c);
 	  break;
 	}
-    /* Fall through. */
+      dtp->u.p.comma_flag = 1;
+      eat_spaces (dtp);
+      break;
+
     case ';':
       dtp->u.p.comma_flag = 1;
       eat_spaces (dtp);

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-04  1:33 [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'" Jerry D
@ 2024-04-04  8:17 ` Paul Richard Thomas
  2024-04-04  9:31 ` Tobias Burnus
  1 sibling, 0 replies; 11+ messages in thread
From: Paul Richard Thomas @ 2024-04-04  8:17 UTC (permalink / raw)
  To: Jerry D; +Cc: gfortran, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

Hi Jerry,

It looks good to me. Noting that this is not a regression, OK for mainline
on condition that you keep a sharp eye out for any associated problems.
Likewise with backporting to 13-branch.

Thanks

Paul


On Thu, 4 Apr 2024 at 02:34, Jerry D <jvdelisle2@gmail.com> wrote:

> Hi all,
>
> The attached log entry and patch (git show) fixes this issue by adding
> logic to handle spaces in eat_separators. One or more spaces by
> themselves are a valid separator. So in this case we look at the
> character following the spaces to see if it is a comma or semicolon.
>
> If so, I change it to the valid separator for the given decimal mode,
> point or comma. This allows the comma or semicolon to be interpreted as
> a null read on the next effective item in the formatted read.
>
> I chose a permissive approach here that allows reads to proceed when the
> input line is mal-formed with an incorrect separator as long as there is
> at least one space in front of it.
>
> New test case included. Regression tested on X86-64.
>
> OK for trunk?  Backport to 13 after some time.
>
> Regards,
>
> Jerry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-04  1:33 [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'" Jerry D
  2024-04-04  8:17 ` Paul Richard Thomas
@ 2024-04-04  9:31 ` Tobias Burnus
  2024-04-04 20:05   ` Jerry D
  2024-04-04 21:04   ` Jerry D
  1 sibling, 2 replies; 11+ messages in thread
From: Tobias Burnus @ 2024-04-04  9:31 UTC (permalink / raw)
  To: Jerry D, gfortran; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 3431 bytes --]

Hi Jerry,

Jerry D wrote:
> The attached log entry and patch (git show) fixes this issue by adding 
> logic to handle spaces in eat_separators. One or more spaces by 
> themselves are a valid separator. So in this case we look at the 
> character following the spaces to see if it is a comma or semicolon.
> 
> If so, I change it to the valid separator for the given decimal mode, 
> point or comma. This allows the comma or semicolon to be interpreted as 
> a null read on the next effective item in the formatted read.
> 
> I chose a permissive approach here that allows reads to proceed when the
> input line is mal-formed with an incorrect separator as long as there is 
> at least one space in front of it.

First: Consider also adding 'PR fortran/105473' to the commit log
as the PRs are closely related, albeit this PR is different-

The patch looks mostly like I would expect, except for decimal='point' 
and a ';' which is *not* preceded by a space.

Thanks for working on it.

Regarding the 'except' case:

* * *

If I try your patch with the testcase of at comment 19,

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114304#c19https://gcc.gnu.org/bugzilla/attachment.cgi?id=57695 ,

I do note that with 'decimal=point', a tailing semicolon is silently
accepted – even if not proceeded by a space.

I think such code is invalid – and you could consider to reject it.
Otherwise, the handling all seems to be in line with the Fortran spec.

i.e. for the following string, I had *expected an error*:

  point, isreal =  F , testinput = ";"n=          42  ios=           0
  point, isreal =  F , testinput = "5;"n=           5  ios=           0
  point, isreal =  T , testinput = "8;"r=   8.00000000      ios= 0
  point, isreal =  T , testinput = "3.3;"r=   3.29999995      ios= 0
  point, isreal =  T , testinput = "3,3;"r=   3.00000000      ios= 0

while I think the following is OK (i.e. no error is what I expect) due 
to the the space before the ';'.

  point, isreal =  F , testinput = "7 ;"n=           7  ios= 0
  point, isreal =  T , testinput = "9 ;"r=   9.00000000      ios= 0
  point, isreal =  T , testinput = "4.4 ;"r=   4.40000010      ios=0
  point, isreal =  T , testinput = "9 ;"r=   9.00000000      ios= 0
  point, isreal =  T , testinput = "4,4 ;"r=   4.00000000      ios= 0

* * *

Looking at the other compilers, ifort, ifx and Flang do issue an error 
here. Likewise, g95 seems to yield an error in this case (see below).

I do note that the Lapack testcase that triggered this PR did have such 
a code - but it was then changed because g95 did not like it:

https://github.com/Reference-LAPACK/lapack/commit/64e8a7500d817869e5fcde35afd39af8bc7a8086

In terms of gfortran: until recently did accept it (all versions, 
including 13+14); it then rejected it due to the change in PR105473 (GCC 
14/mainline, backported to 13)– but I now think it rightly did so. With 
the current patch, it is accepted again.

* * *

I have attached the modified testcase linked above; consider adding it 
as well. - Changes to the one of the attachment:
- I added a few additional (albeit boring) tests
- I added an expected output + error diagnostic.

The testcase assumes an error for ';' as separator (with 'point'), 
unless there is a space before it.

[If we want to not diagnose this as vendor extension, we really need to 
add a comment to that testcase besides changing valid = .false. to .true.]

Tobias

[-- Attachment #2: pr114304.f90 --]
[-- Type: text/x-fortran, Size: 4316 bytes --]

! { dg-do run }
!
! PR fortran/114304
!
! See also PR fortran/105473
!
! Testing: Does list-directed reading an integer/real allows some non-integer input?
!
! Note: GCC result comments before fix of this PR.

  implicit none
  call t(.true.,  'comma', ';') ! No error shown
  call t(.false., 'point', ';') ! /!\ gfortran: no error, others: error
  call t(.false., 'comma', ',') ! Error shown
  call t(.true.,  'point', ',') ! No error shown
  call t(.false., 'comma', '.') ! Error shown
  call t(.false., 'point', '.') ! Error shown
  call t(.false., 'comma', '5.') ! Error shown
  call t(.false., 'point', '5.') ! gfortran/flang: Error shown, ifort: no error
  call t(.false., 'comma', '5,') ! gfortran: error; others: no error
  call t(.true.,  'point', '5,') ! No error shown
  call t(.true.,  'comma', '5;') ! No error shown
  call t(.false., 'point', '5;') ! /!\ gfortran: no error shown, others: error
  call t(.true.,  'comma', '7 .') ! No error shown
  call t(.true.,  'point', '7 .') ! No error shown
  call t(.true.,  'comma', '7 ,') ! /!\ gfortran: error; others: no error
  call t(.true.,  'point', '7 ,') ! No error shown
  call t(.true.,  'comma', '7 ;') ! No error shown
  call t(.true.,  'point', '7 ;') ! No error shown

!  print *, '---------------'

  call t(.false., 'comma', '8.', .true.) ! Error shown
  call t(.true.,  'point', '8.', .true.) ! gfortran/flang: Error shown, ifort: no error
  call t(.true.,  'comma', '8,', .true.) ! gfortran: error; others: no error
  call t(.true.,  'point', '8,', .true.) ! No error shown
  call t(.true.,  'comma', '8;', .true.) ! No error shown
  call t(.false., 'point', '8;', .true.) ! /!\ gfortran: no error shown, others: error
  call t(.true.,  'comma', '9 .', .true.) ! No error shown
  call t(.true.,  'point', '9 .', .true.) ! No error shown
  call t(.true.,  'comma', '9 ,', .true.) ! /!\ gfortran: error; others: no error
  call t(.true.,  'point', '9 ,', .true.) ! No error shown
  call t(.true.,  'comma', '9 ;', .true.) ! No error shown
  call t(.true.,  'point', '9 ;', .true.) ! No error shown
  call t(.false., 'comma', '3,3.', .true.) ! Error shown
  call t(.false., 'point', '3.3.', .true.) ! Error shown
  call t(.false., 'comma', '3,3,', .true.) ! gfortran/flang: no error; ifort: error
  call t(.true.,  'comma', '3,3;', .true.) ! No error shown
  call t(.false., 'point', '3.3;', .true.) ! gfortran/flang: no error; ifort: error
  call t(.true.,  'comma', '4,4 .', .true.) ! N error shown
  call t(.true.,  'point', '4.4 .', .true.) ! No error shown
  call t(.true.,  'comma', '4,4 ,', .true.) ! /!\ gfortran: error; others: no error
  call t(.true.,  'point', '4.4 ,', .true.) ! No error shown
  call t(.true.,  'comma', '4,4 ;', .true.) ! No error shown
  call t(.true.,  'point', '4.4 ;', .true.) ! No error shown

!  print *, '---------------'

  call t(.true.,  'comma', '8', .true.)
  call t(.true.,  'point', '8', .true.)
  call t(.true.,  'point', '9 ;', .true.)
  call t(.true.,  'comma', '3;3.', .true.)
  call t(.true.,  'point', '3,3.', .true.)
  call t(.true.,  'comma', '3;3,', .true.)
  call t(.true.,  'comma', '3;3;', .true.)
  call t(.true.,  'point', '3,3;', .true.)
  call t(.true.,  'comma', '4;4 .', .true.)
  call t(.true.,  'point', '4,4 .', .true.)
  call t(.true.,  'comma', '4;4 ,', .true.)
  call t(.true.,  'point', '4,4 ,', .true.)
  call t(.true.,  'comma', '4;4 ;', .true.)
  call t(.true.,  'point', '4,4 ;', .true.)
contains
subroutine t(valid, dec, testinput, isreal)
  logical, value :: valid
  character(len=*) :: dec, testinput
  logical, optional :: isreal
  logical :: isreal2
  integer n,ios
  real :: r
  r = 42; n = 42
  isreal2 = .false.
  if (present(isreal)) isreal2 = isreal

  if (isreal2) then
    read(testinput,*,decimal=dec,iostat=ios) r
    if ((valid .and. ios /= 0) .or. (.not.valid .and. ios == 0)) then
      print '(*(g0))', valid, ', ', dec,', isreal = ',isreal2,', testinput = "',testinput,'"',', r=',r,' ios=',ios
      print *, 'ERROR'
      stop 1
    end if
  else
    read(testinput,*,decimal=dec,iostat=ios) n
    if ((valid .and. ios /= 0) .or. (.not.valid .and. ios == 0)) then
      print '(*(g0))', valid, ', ', dec,', isreal = ',isreal2,', testinput = "',testinput,'"',', n=',n,' ios=',ios
      print *, 'ERROR'
      stop 1
    end if
  end if
end
end program

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-04  9:31 ` Tobias Burnus
@ 2024-04-04 20:05   ` Jerry D
  2024-04-04 21:04   ` Jerry D
  1 sibling, 0 replies; 11+ messages in thread
From: Jerry D @ 2024-04-04 20:05 UTC (permalink / raw)
  To: Tobias Burnus, gfortran; +Cc: gcc-patches

On 4/4/24 2:31 AM, Tobias Burnus wrote:
> Hi Jerry,
> 
> Jerry D wrote:
>> The attached log entry and patch (git show) fixes this issue by adding 
>> logic to handle spaces in eat_separators. One or more spaces by 
>> themselves are a valid separator. So in this case we look at the 
>> character following the spaces to see if it is a comma or semicolon.
>>
>> If so, I change it to the valid separator for the given decimal mode, 
>> point or comma. This allows the comma or semicolon to be interpreted 
>> as a null read on the next effective item in the formatted read.
>>
>> I chose a permissive approach here that allows reads to proceed when the
>> input line is mal-formed with an incorrect separator as long as there 
>> is at least one space in front of it.
> 
> First: Consider also adding 'PR fortran/105473' to the commit log
> as the PRs are closely related, albeit this PR is different-
> 
> The patch looks mostly like I would expect, except for decimal='point' 
> and a ';' which is *not* preceded by a space.
> 
> Thanks for working on it.
> 
> Regarding the 'except' case:
> 
> * * *
> 
> If I try your patch with the testcase of at comment 19,
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114304#c19
> → https://gcc.gnu.org/bugzilla/attachment.cgi?id=57695 ,
> 
> I do note that with 'decimal=point', a tailing semicolon is silently
> accepted – even if not proceeded by a space.

I did that on purpose out of leniency and fear of breaking something.  I 
will add that in and do some testing.

> 
> I think such code is invalid – and you could consider to reject it.
> Otherwise, the handling all seems to be in line with the Fortran spec.
> 
> i.e. for the following string, I had *expected an error*:

I will see what it does. I agree in principle.

> 
>   point, isreal =  F , testinput = ";"n=          42  ios=           0
>   point, isreal =  F , testinput = "5;"n=           5  ios=           0
>   point, isreal =  T , testinput = "8;"r=   8.00000000      ios= 0
>   point, isreal =  T , testinput = "3.3;"r=   3.29999995      ios= 0
>   point, isreal =  T , testinput = "3,3;"r=   3.00000000      ios= 0
> 
> while I think the following is OK (i.e. no error is what I expect) due 
> to the the space before the ';'.

Agree and this is what I was attempting.

> 
>   point, isreal =  F , testinput = "7 ;"n=           7  ios= 0
>   point, isreal =  T , testinput = "9 ;"r=   9.00000000      ios= 0
>   point, isreal =  T , testinput = "4.4 ;"r=   4.40000010      ios=0
>   point, isreal =  T , testinput = "9 ;"r=   9.00000000      ios= 0
>   point, isreal =  T , testinput = "4,4 ;"r=   4.00000000      ios= 0
> 
> * * *
> 
> Looking at the other compilers, ifort, ifx and Flang do issue an error 
> here. Likewise, g95 seems to yield an error in this case (see below).
> 
> I do note that the Lapack testcase that triggered this PR did have such 
> a code - but it was then changed because g95 did not like it:
> 
> https://github.com/Reference-LAPACK/lapack/commit/64e8a7500d817869e5fcde35afd39af8bc7a8086
> 

I am glad they fixed that, it triggered a whole lot of cycles here.

> In terms of gfortran: until recently did accept it (all versions, 
> including 13+14); it then rejected it due to the change in PR105473 (GCC 
> 14/mainline, backported to 13)– but I now think it rightly did so. With 
> the current patch, it is accepted again.
> 
> * * *
> 
> I have attached the modified testcase linked above; consider adding it 
> as well. - Changes to the one of the attachment:
> - I added a few additional (albeit boring) tests
> - I added an expected output + error diagnostic.
> 
> The testcase assumes an error for ';' as separator (with 'point'), 
> unless there is a space before it.
> 
> [If we want to not diagnose this as vendor extension, we really need to 
> add a comment to that testcase besides changing valid = .false. to .true.]

I have gone back and forth on this issue many times trying to find the 
middle ground. The standard is fairly clear. To me it is on the user to 
not use malformed input so allowing with the intervening space we are 
simply ignoring the trailing ',' or ';' and calling the spaces 
sufficient as a valid separator.

Regardless I now have your modified test case passing. Much appreciated.

Thanks for the reviews.  I will resubmit when I can, hopefully today.

Cheers,

Jerry




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-04  9:31 ` Tobias Burnus
  2024-04-04 20:05   ` Jerry D
@ 2024-04-04 21:04   ` Jerry D
  2024-04-04 21:41     ` Tobias Burnus
  1 sibling, 1 reply; 11+ messages in thread
From: Jerry D @ 2024-04-04 21:04 UTC (permalink / raw)
  To: Tobias Burnus, gfortran; +Cc: gcc-patches, Jerry D

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

On 4/4/24 2:31 AM, Tobias Burnus wrote:
> Hi Jerry,
> 
--- snip ---
> The patch looks mostly like I would expect, except for decimal='point' 
> and a ';' which is *not* preceded by a space.
> 
> Thanks for working on it.
> 
> Regarding the 'except' case:
> 
--- snip ---
> i.e. for the following string, I had *expected an error*:
> 
>   point, isreal =  F , testinput = ";"n=          42  ios=           0
>   point, isreal =  F , testinput = "5;"n=           5  ios=           0
>   point, isreal =  T , testinput = "8;"r=   8.00000000      ios= 0
>   point, isreal =  T , testinput = "3.3;"r=   3.29999995      ios= 0
>   point, isreal =  T , testinput = "3,3;"r=   3.00000000      ios= 0
> 
--- snip ---
> I have attached the modified testcase linked above; consider adding it 
> as well. - Changes to the one of the attachment:
> - I added a few additional (albeit boring) tests
> - I added an expected output + error diagnostic.
> 
> The testcase assumes an error for ';' as separator (with 'point'), 
> unless there is a space before it.
> 
--- snip ---

Here attached is the revised patch.  I replaced the new test case I had 
with the one you provided modified and it now passes.

I modified the test case pr105473.f90 to expect the error on semicolon.

Regression tested on X86_64.  OK for trunk and a bit later back port to 13?

Cheers,

Jerry

[-- Attachment #2: submit2.diff --]
[-- Type: text/x-patch, Size: 7455 bytes --]

commit 9c8318cd8703d49ad7563e89765f8849ebc14385
Author: Jerry DeLisle <jvdelisle@gcc.gnu.org>
Date:   Thu Apr 4 13:48:20 2024 -0700

    libfortran: Fix handling of formatted separators.
    
            PR libfortran/114304
            PR libfortran/105473
    
    libgfortran/ChangeLog:
    
            * io/list_read.c (eat_separator): Add logic to handle spaces
            preceding a comma or semicolon such that that a 'null' read
            occurs without error at the end of comma or semicolon
            terminated input lines. Add check and error message for ';'.
    
    gcc/testsuite/ChangeLog:
    
            * gfortran.dg/pr105473.f90: Modify to verify new error message.
            * gfortran.dg/pr114304.f90: New test.

diff --git a/gcc/testsuite/gfortran.dg/pr105473.f90 b/gcc/testsuite/gfortran.dg/pr105473.f90
index 2679f6bb447..863a312c794 100644
--- a/gcc/testsuite/gfortran.dg/pr105473.f90
+++ b/gcc/testsuite/gfortran.dg/pr105473.f90
@@ -9,11 +9,11 @@
   n = 999; m = 777; r=1.2345
   z = cmplx(0.0,0.0)
 
-! Check that semi-colon is allowed as separator with decimal=point.
+! Check that semi-colon is not allowed as separator with decimal=point.
   ios=0
   testinput = '1;17;3.14159'
   read(testinput,*,decimal='point',iostat=ios) n, m, r
-  if (ios /= 0) stop 1
+  if (ios /= 5010) stop 1
 
 ! Check that semi-colon allowed as a separator with decimal=point.
   ios=0
diff --git a/gcc/testsuite/gfortran.dg/pr114304.f90 b/gcc/testsuite/gfortran.dg/pr114304.f90
new file mode 100644
index 00000000000..8344a9ea857
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr114304.f90
@@ -0,0 +1,101 @@
+! { dg-do run }
+!
+! PR fortran/114304
+!
+! See also PR fortran/105473
+!
+! Testing: Does list-directed reading an integer/real allow some non-integer input?
+!
+! Note: GCC result comments before fix of this PR.
+
+  implicit none
+  call t(.true.,  'comma', ';') ! No error shown
+  call t(.false., 'point', ';') ! /!\ gfortran: no error, others: error
+  call t(.false., 'comma', ',') ! Error shown
+  call t(.true.,  'point', ',') ! No error shown
+  call t(.false., 'comma', '.') ! Error shown
+  call t(.false., 'point', '.') ! Error shown
+  call t(.false., 'comma', '5.') ! Error shown
+  call t(.false., 'point', '5.') ! gfortran/flang: Error shown, ifort: no error
+  call t(.false., 'comma', '5,') ! gfortran: error; others: no error
+  call t(.true.,  'point', '5,') ! No error shown
+  call t(.true.,  'comma', '5;') ! No error shown
+  call t(.false., 'point', '5;') ! /!\ gfortran: no error shown, others: error
+  call t(.true.,  'comma', '7 .') ! No error shown
+  call t(.true.,  'point', '7 .') ! No error shown
+  call t(.true.,  'comma', '7 ,') ! /!\ gfortran: error; others: no error
+  call t(.true.,  'point', '7 ,') ! No error shown
+  call t(.true.,  'comma', '7 ;') ! No error shown
+  call t(.true.,  'point', '7 ;') ! No error shown
+
+!  print *, '---------------'
+
+  call t(.false., 'comma', '8.', .true.) ! Error shown
+  call t(.true.,  'point', '8.', .true.) ! gfortran/flang: Error shown, ifort: no error
+  call t(.true.,  'comma', '8,', .true.) ! gfortran: error; others: no error
+  call t(.true.,  'point', '8,', .true.) ! No error shown
+  call t(.true.,  'comma', '8;', .true.) ! No error shown
+  call t(.false., 'point', '8;', .true.) ! /!\ gfortran: no error shown, others: error
+  call t(.true.,  'comma', '9 .', .true.) ! No error shown
+  call t(.true.,  'point', '9 .', .true.) ! No error shown
+  call t(.true.,  'comma', '9 ,', .true.) ! /!\ gfortran: error; others: no error
+  call t(.true.,  'point', '9 ,', .true.) ! No error shown
+  call t(.true.,  'comma', '9 ;', .true.) ! No error shown
+  call t(.true.,  'point', '9 ;', .true.) ! No error shown
+  call t(.false., 'comma', '3,3.', .true.) ! Error shown
+  call t(.false., 'point', '3.3.', .true.) ! Error shown
+  call t(.false., 'comma', '3,3,', .true.) ! gfortran/flang: no error; ifort: error
+  call t(.true.,  'comma', '3,3;', .true.) ! No error shown
+  call t(.false., 'point', '3.3;', .true.) ! gfortran/flang: no error; ifort: error
+  call t(.true.,  'comma', '4,4 .', .true.) ! N error shown
+  call t(.true.,  'point', '4.4 .', .true.) ! No error shown
+  call t(.true.,  'comma', '4,4 ,', .true.) ! /!\ gfortran: error; others: no error
+  call t(.true.,  'point', '4.4 ,', .true.) ! No error shown
+  call t(.true.,  'comma', '4,4 ;', .true.) ! No error shown
+  call t(.true.,  'point', '4.4 ;', .true.) ! No error shown
+
+!  print *, '---------------'
+
+  call t(.true.,  'comma', '8', .true.)
+  call t(.true.,  'point', '8', .true.)
+  call t(.true.,  'point', '9 ;', .true.)
+  call t(.true.,  'comma', '3;3.', .true.)
+  call t(.true.,  'point', '3,3.', .true.)
+  call t(.true.,  'comma', '3;3,', .true.)
+  call t(.true.,  'comma', '3;3;', .true.)
+  call t(.true.,  'point', '3,3;', .true.)
+  call t(.true.,  'comma', '4;4 .', .true.)
+  call t(.true.,  'point', '4,4 .', .true.)
+  call t(.true.,  'comma', '4;4 ,', .true.)
+  call t(.true.,  'point', '4,4 ,', .true.)
+  call t(.true.,  'comma', '4;4 ;', .true.)
+  call t(.true.,  'point', '4,4 ;', .true.)
+contains
+subroutine t(valid, dec, testinput, isreal)
+  logical, value :: valid
+  character(len=*) :: dec, testinput
+  logical, optional :: isreal
+  logical :: isreal2
+  integer n,ios
+  real :: r
+  r = 42; n = 42
+  isreal2 = .false.
+  if (present(isreal)) isreal2 = isreal
+
+  if (isreal2) then
+    read(testinput,*,decimal=dec,iostat=ios) r
+    if ((valid .and. ios /= 0) .or. (.not.valid .and. ios == 0)) then
+      print '(*(g0))', valid, ', ', dec,', isreal = ',isreal2,', testinput = "',testinput,'"',', r=',r,' ios=',ios
+      print *, 'ERROR'
+      stop 1
+    end if
+  else
+    read(testinput,*,decimal=dec,iostat=ios) n
+    if ((valid .and. ios /= 0) .or. (.not.valid .and. ios == 0)) then
+      print '(*(g0))', valid, ', ', dec,', isreal = ',isreal2,', testinput = "',testinput,'"',', n=',n,' ios=',ios
+      print *, 'ERROR'
+      stop 1
+    end if
+  end if
+end
+end program
diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index fb3f7dbc34d..6bf59329add 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -461,11 +461,30 @@ eat_separator (st_parameter_dt *dtp)
   int c, n;
   int err = 0;
 
-  eat_spaces (dtp);
   dtp->u.p.comma_flag = 0;
+  c = next_char (dtp);
+  if (c == ' ')
+    {
+      eat_spaces (dtp);
+      c = next_char (dtp);
+      if (c == ',')
+	{
+	  if (dtp->u.p.current_unit->decimal_status == DECIMAL_COMMA)
+	    unget_char (dtp, ';');
+	  dtp->u.p.comma_flag = 1;
+	  eat_spaces (dtp);
+	  return err;
+	}
+      if (c == ';')
+	{
+	  if (dtp->u.p.current_unit->decimal_status == DECIMAL_POINT)
+	    unget_char (dtp, ',');
+	  dtp->u.p.comma_flag = 1;
+	  eat_spaces (dtp);
+	  return err;
+	}
+    }
 
-  if ((c = next_char (dtp)) == EOF)
-    return LIBERROR_END;
   switch (c)
     {
     case ',':
@@ -476,8 +495,18 @@ eat_separator (st_parameter_dt *dtp)
 	  unget_char (dtp, c);
 	  break;
 	}
-    /* Fall through. */
+      dtp->u.p.comma_flag = 1;
+      eat_spaces (dtp);
+      break;
+
     case ';':
+      if (dtp->u.p.current_unit->decimal_status == DECIMAL_POINT)
+	{
+	  generate_error (&dtp->common, LIBERROR_READ_VALUE,
+	   "Semicolon not allowed as separator with DECIMAL='point'");
+	  unget_char (dtp, c);
+	  break;
+	}
       dtp->u.p.comma_flag = 1;
       eat_spaces (dtp);
       break;

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-04 21:04   ` Jerry D
@ 2024-04-04 21:41     ` Tobias Burnus
  2024-04-05 17:47       ` Jerry D
  0 siblings, 1 reply; 11+ messages in thread
From: Tobias Burnus @ 2024-04-04 21:41 UTC (permalink / raw)
  To: Jerry D, gfortran; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

Hi Jerry,

I think for the current testcases, I like the patch – the question is 
only what's about:

   ',3' as input for 'comma'   (or '.3' as input for 'point')

For 'point' – 0.3 is read and ios = 0 (as expected)
But for 'comma':
* GCC 12 reads nothing and has ios = 0.
* GCC 13/mainline has an error (ios != 0 – and reads nothing)
* GCC with your patch: Same result: ios != 0 and nothing read.

Expected: Same as with ','/'comma' – namely: read-in value is 0.3.
→ https://godbolt.org/z/4rc8fz4sT for the full example, which works with 
ifort, ifx and flang

* * *

Can you check and fix this? It looks perfectly valid to me to have 
remove the '0' in the floating point numbers '0.3' or '0,3' seems to be 
permitted – and it works for '.' (with 'point') but not for ',' (with 
'comma').

F2023's "13.10.3.1 List-directed input forms" refers to "13.7.2.3.2 F 
editing", which states:

"The standard form of the input field [...] The form of the mantissa is 
an optional sign, followed by a string of one or more digits optionally 
containing a decimal symbol."

The latter does not require that the digit has to be before the decimal 
sign and as for output, it is optional, it is surely intended that ",3" 
is a valid floating-point number for decimal='comma'.

* * *

I extended the testcase to check for this – see attached diff. All 
'point' work, all 'comma' fail.

Thanks for working on this!

Tobias

[-- Attachment #2: pr114304.f90.diff --]
[-- Type: text/x-patch, Size: 970 bytes --]

diff --git a/gcc/testsuite/gfortran.dg/pr114304.f90 b/gcc/testsuite/gfortran.dg/pr114304.f90
index 8344a9ea857..2bcf9bc7f57 100644
--- a/gcc/testsuite/gfortran.dg/pr114304.f90
+++ b/gcc/testsuite/gfortran.dg/pr114304.f90
@@ -70,7 +70,25 @@
   call t(.true.,  'point', '4,4 ,', .true.)
   call t(.true.,  'comma', '4;4 ;', .true.)
   call t(.true.,  'point', '4,4 ;', .true.)
+
+  call t2('comma', ',2')
+  call t2('point', '.2')
+  call t2('comma', ',2;')
+  call t2('point', '.2,')
+  call t2('comma', ',2 ,')
+  call t2('point', '.2 .')
 contains
+subroutine t2(dec, testinput)
+  character(*) :: dec, testinput
+  integer ios
+  real :: r
+  r = 42
+  read(testinput,*,decimal=dec,iostat=ios) r
+  if (ios /= 0 .or.  abs(r - 0.2) > epsilon(r)) then
+    print '(*(g0))', dec, ', testinput = "',testinput,'"',', r=',r,' ios=',ios
+    stop 3 
+  end if
+end
 subroutine t(valid, dec, testinput, isreal)
   logical, value :: valid
   character(len=*) :: dec, testinput

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-04 21:41     ` Tobias Burnus
@ 2024-04-05 17:47       ` Jerry D
  2024-04-06  2:38         ` Jerry D
  0 siblings, 1 reply; 11+ messages in thread
From: Jerry D @ 2024-04-05 17:47 UTC (permalink / raw)
  To: Tobias Burnus, gfortran; +Cc: gcc-patches, Jerry D

On 4/4/24 2:41 PM, Tobias Burnus wrote:
> Hi Jerry,
> 
> I think for the current testcases, I like the patch – the question is 
> only what's about:
> 
>    ',3' as input for 'comma'   (or '.3' as input for 'point')
> 
> For 'point' – 0.3 is read and ios = 0 (as expected)
> But for 'comma':
> * GCC 12 reads nothing and has ios = 0.
> * GCC 13/mainline has an error (ios != 0 – and reads nothing)
> * GCC with your patch: Same result: ios != 0 and nothing read.
> 
> Expected: Same as with ','/'comma' – namely: read-in value is 0.3.
> → https://godbolt.org/z/4rc8fz4sT for the full example, which works with 
> ifort, ifx and flang
> 
> * * *
> 
> Can you check and fix this? It looks perfectly valid to me to have 
> remove the '0' in the floating point numbers '0.3' or '0,3' seems to be 
> permitted – and it works for '.' (with 'point') but not for ',' (with 
> 'comma').
> 
Yes, I found the spot to fix this.

> F2023's "13.10.3.1 List-directed input forms" refers to "13.7.2.3.2 F 
> editing", which states:
> 
> "The standard form of the input field [...] The form of the mantissa is 
> an optional sign, followed by a string of one or more digits optionally 
> containing a decimal symbol."
> 
> The latter does not require that the digit has to be before the decimal 
> sign and as for output, it is optional, it is surely intended that ",3" 
> is a valid floating-point number for decimal='comma'.
> 

Agree

> * * *
> 
> I extended the testcase to check for this – see attached diff. All 
> 'point' work, all 'comma' fail.
> 
> Thanks for working on this!
> 
> Tobias

Thanks much. After I fix it, I will use your extended test case in the 
test suite.

Jerry -

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-05 17:47       ` Jerry D
@ 2024-04-06  2:38         ` Jerry D
  2024-04-06  5:17           ` Tobias Burnus
  2024-04-08  9:53           ` [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] (was: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'") Tobias Burnus
  0 siblings, 2 replies; 11+ messages in thread
From: Jerry D @ 2024-04-06  2:38 UTC (permalink / raw)
  To: Tobias Burnus, gfortran; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

On 4/5/24 10:47 AM, Jerry D wrote:
> On 4/4/24 2:41 PM, Tobias Burnus wrote:
>> Hi Jerry,
>>
>> I think for the current testcases, I like the patch – the question is 
>> only what's about:
>>
>>    ',3' as input for 'comma'   (or '.3' as input for 'point')
>>
>> For 'point' – 0.3 is read and ios = 0 (as expected)
>> But for 'comma':
>> * GCC 12 reads nothing and has ios = 0.
>> * GCC 13/mainline has an error (ios != 0 – and reads nothing)
>> * GCC with your patch: Same result: ios != 0 and nothing read.
>>
>> Expected: Same as with ','/'comma' – namely: read-in value is 0.3.
>> → https://godbolt.org/z/4rc8fz4sT for the full example, which works 
>> with ifort, ifx and flang
>>
>> * * *
>>
>> Can you check and fix this? It looks perfectly valid to me to have 
>> remove the '0' in the floating point numbers '0.3' or '0,3' seems to 
>> be permitted – and it works for '.' (with 'point') but not for ',' 
>> (with 'comma').
>>
> Yes, I found the spot to fix this.
> 
>> F2023's "13.10.3.1 List-directed input forms" refers to "13.7.2.3.2 F 
>> editing", which states:
>>
>> "The standard form of the input field [...] The form of the mantissa 
>> is an optional sign, followed by a string of one or more digits 
>> optionally containing a decimal symbol."
>>
>> The latter does not require that the digit has to be before the 
>> decimal sign and as for output, it is optional, it is surely intended 
>> that ",3" is a valid floating-point number for decimal='comma'.
>>
> 
> Agree
> 
>> * * *
>>
>> I extended the testcase to check for this – see attached diff. All 
>> 'point' work, all 'comma' fail.
>>
>> Thanks for working on this!
>>
>> Tobias
> 
> Thanks much. After I fix it, I will use your extended test case in the 
> test suite.
> 
> Jerry -


See attached updated patch.

Regressions tested on x86-64. OK for trunk and 13 after a bit.

Jerry -

[-- Attachment #2: submit-3.diff --]
[-- Type: text/x-patch, Size: 8065 bytes --]

commit 690b9fa57d95796ba0e92a172e1490601f25e03a
Author: Jerry DeLisle <jvdelisle@gcc.gnu.org>
Date:   Fri Apr 5 19:25:13 2024 -0700

    libfortran: Fix handling of formatted separators.
    
            PR libfortran/114304
            PR libfortran/105473
    
    libgfortran/ChangeLog:
    
            * io/list_read.c (eat_separator): Add logic to handle spaces
            preceding a comma or semicolon such that that a 'null' read
            occurs without error at the end of comma or semicolon
            terminated input lines. Add check and error message for ';'.
            (list_formatted_read_scalar): Treat comma as a decimal point
            when specified by the decimal mode on the first item.
    
    gcc/testsuite/ChangeLog:
    
            * gfortran.dg/pr105473.f90: Modify to verify new error message.
            * gfortran.dg/pr114304.f90: New test.

diff --git a/gcc/testsuite/gfortran.dg/pr105473.f90 b/gcc/testsuite/gfortran.dg/pr105473.f90
index 2679f6bb447..863a312c794 100644
--- a/gcc/testsuite/gfortran.dg/pr105473.f90
+++ b/gcc/testsuite/gfortran.dg/pr105473.f90
@@ -9,11 +9,11 @@
   n = 999; m = 777; r=1.2345
   z = cmplx(0.0,0.0)
 
-! Check that semi-colon is allowed as separator with decimal=point.
+! Check that semi-colon is not allowed as separator with decimal=point.
   ios=0
   testinput = '1;17;3.14159'
   read(testinput,*,decimal='point',iostat=ios) n, m, r
-  if (ios /= 0) stop 1
+  if (ios /= 5010) stop 1
 
 ! Check that semi-colon allowed as a separator with decimal=point.
   ios=0
diff --git a/gcc/testsuite/gfortran.dg/pr114304.f90 b/gcc/testsuite/gfortran.dg/pr114304.f90
new file mode 100644
index 00000000000..2f913f1ab34
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr114304.f90
@@ -0,0 +1,114 @@
+! { dg-do run }
+!
+! PR fortran/114304
+!
+! See also PR fortran/105473
+!
+! Testing: Does list-directed reading an integer/real allow some non-integer input?
+!
+! Note: GCC result comments before fix of this PR.
+
+  implicit none
+  call t(.true.,  'comma', ';') ! No error shown
+  call t(.false., 'point', ';') ! /!\ gfortran: no error, others: error
+  call t(.false., 'comma', ',') ! Error shown
+  call t(.true.,  'point', ',') ! No error shown
+  call t(.false., 'comma', '.') ! Error shown
+  call t(.false., 'point', '.') ! Error shown
+  call t(.false., 'comma', '5.') ! Error shown
+  call t(.false., 'point', '5.') ! gfortran/flang: Error shown, ifort: no error
+  call t(.false., 'comma', '5,') ! gfortran: error; others: no error
+  call t(.true.,  'point', '5,') ! No error shown
+  call t(.true.,  'comma', '5;') ! No error shown
+  call t(.false., 'point', '5;') ! /!\ gfortran: no error shown, others: error
+  call t(.true.,  'comma', '7 .') ! No error shown
+  call t(.true.,  'point', '7 .') ! No error shown
+  call t(.true.,  'comma', '7 ,') ! /!\ gfortran: error; others: no error
+  call t(.true.,  'point', '7 ,') ! No error shown
+  call t(.true.,  'comma', '7 ;') ! No error shown
+  call t(.true.,  'point', '7 ;') ! No error shown
+
+!  print *, '---------------'
+
+  call t(.false., 'comma', '8.', .true.) ! Error shown
+  call t(.true.,  'point', '8.', .true.) ! gfortran/flang: Error shown, ifort: no error
+  call t(.true.,  'comma', '8,', .true.) ! gfortran: error; others: no error
+  call t(.true.,  'point', '8,', .true.) ! No error shown
+  call t(.true.,  'comma', '8;', .true.) ! No error shown
+  call t(.false., 'point', '8;', .true.) ! /!\ gfortran: no error shown, others: error
+  call t(.true.,  'comma', '9 .', .true.) ! No error shown
+  call t(.true.,  'point', '9 .', .true.) ! No error shown
+  call t(.true.,  'comma', '9 ,', .true.) ! /!\ gfortran: error; others: no error
+  call t(.true.,  'point', '9 ,', .true.) ! No error shown
+  call t(.true.,  'comma', '9 ;', .true.) ! No error shown
+  call t(.true.,  'point', '9 ;', .true.) ! No error shown
+  call t(.false., 'comma', '3,3.', .true.) ! Error shown
+  call t(.false., 'point', '3.3.', .true.) ! Error shown
+  call t(.false., 'comma', '3,3,', .true.) ! gfortran/flang: no error; ifort: error
+  call t(.true.,  'comma', '3,3;', .true.) ! No error shown
+  call t(.false., 'point', '3.3;', .true.) ! gfortran/flang: no error; ifort: error
+  call t(.true.,  'comma', '4,4 .', .true.) ! N error shown
+  call t(.true.,  'point', '4.4 .', .true.) ! No error shown
+  call t(.true.,  'comma', '4,4 ,', .true.) ! /!\ gfortran: error; others: no error
+  call t(.true.,  'point', '4.4 ,', .true.) ! No error shown
+  call t(.true.,  'comma', '4,4 ;', .true.) ! No error shown
+  call t(.true.,  'point', '4.4 ;', .true.) ! No error shown
+
+!  print *, '---------------'
+
+  call t(.true.,  'comma', '8', .true.)
+  call t(.true.,  'point', '8', .true.)
+  call t(.true.,  'point', '9 ;', .true.)
+  call t(.true.,  'comma', '3;3.', .true.)
+  call t(.true.,  'point', '3,3.', .true.)
+  call t(.true.,  'comma', '3;3,', .true.)
+  call t(.true.,  'comma', '3;3;', .true.)
+  call t(.true.,  'point', '3,3;', .true.)
+  call t(.true.,  'comma', '4;4 .', .true.)
+  call t(.true.,  'point', '4,4 .', .true.)
+  call t(.true.,  'comma', '4;4 ,', .true.)
+  call t(.true.,  'point', '4,4 ,', .true.)
+  call t(.true.,  'comma', '4;4 ;', .true.)
+  call t(.true.,  'point', '4,4 ;', .true.)
+
+  call t2('comma', ',2')
+  call t2('point', '.2')
+  call t2('comma', ',2;')
+  call t2('point', '.2,')
+  call t2('comma', ',2 ,')
+  call t2('point', '.2 .')
+contains
+subroutine t2(dec, testinput)
+  character(*) :: dec, testinput
+  integer ios
+  real :: r
+  r = 42
+  read(testinput,*,decimal=dec, iostat=ios) r
+  if (ios /= 0 .or.  abs(r - 0.2) > epsilon(r)) then
+    stop 3 
+  end if
+end
+subroutine t(valid, dec, testinput, isreal)
+  logical, value :: valid
+  character(len=*) :: dec, testinput
+  logical, optional :: isreal
+  logical :: isreal2
+  integer n,ios
+  real :: r
+  r = 42; n = 42
+  isreal2 = .false.
+  if (present(isreal)) isreal2 = isreal
+
+  if (isreal2) then
+    read(testinput,*,decimal=dec,iostat=ios) r
+    if ((valid .and. ios /= 0) .or. (.not.valid .and. ios == 0)) then
+      stop 1
+    end if
+  else
+    read(testinput,*,decimal=dec,iostat=ios) n
+    if ((valid .and. ios /= 0) .or. (.not.valid .and. ios == 0)) then
+      stop 1
+    end if
+  end if
+end
+end program
diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index fb3f7dbc34d..b56f2a4e6d6 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -461,11 +461,30 @@ eat_separator (st_parameter_dt *dtp)
   int c, n;
   int err = 0;
 
-  eat_spaces (dtp);
   dtp->u.p.comma_flag = 0;
+  c = next_char (dtp);
+  if (c == ' ')
+    {
+      eat_spaces (dtp);
+      c = next_char (dtp);
+      if (c == ',')
+	{
+	  if (dtp->u.p.current_unit->decimal_status == DECIMAL_COMMA)
+	    unget_char (dtp, ';');
+	  dtp->u.p.comma_flag = 1;
+	  eat_spaces (dtp);
+	  return err;
+	}
+      if (c == ';')
+	{
+	  if (dtp->u.p.current_unit->decimal_status == DECIMAL_POINT)
+	    unget_char (dtp, ',');
+	  dtp->u.p.comma_flag = 1;
+	  eat_spaces (dtp);
+	  return err;
+	}
+    }
 
-  if ((c = next_char (dtp)) == EOF)
-    return LIBERROR_END;
   switch (c)
     {
     case ',':
@@ -476,8 +495,18 @@ eat_separator (st_parameter_dt *dtp)
 	  unget_char (dtp, c);
 	  break;
 	}
-    /* Fall through. */
+      dtp->u.p.comma_flag = 1;
+      eat_spaces (dtp);
+      break;
+
     case ';':
+      if (dtp->u.p.current_unit->decimal_status == DECIMAL_POINT)
+	{
+	  generate_error (&dtp->common, LIBERROR_READ_VALUE,
+	   "Semicolon not allowed as separator with DECIMAL='point'");
+	  unget_char (dtp, c);
+	  break;
+	}
       dtp->u.p.comma_flag = 1;
       eat_spaces (dtp);
       break;
@@ -2144,7 +2173,9 @@ list_formatted_read_scalar (st_parameter_dt *dtp, bt type, void *p,
 	  err = LIBERROR_END;
 	  goto cleanup;
 	}
-      if (is_separator (c))
+      if (c == ',' && dtp->u.p.current_unit->decimal_status == DECIMAL_COMMA)
+	c = '.';
+      else if (is_separator (c))
 	{
 	  /* Found a null value.  */
 	  dtp->u.p.repeat_count = 0;

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
  2024-04-06  2:38         ` Jerry D
@ 2024-04-06  5:17           ` Tobias Burnus
  2024-04-08  9:53           ` [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] (was: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'") Tobias Burnus
  1 sibling, 0 replies; 11+ messages in thread
From: Tobias Burnus @ 2024-04-06  5:17 UTC (permalink / raw)
  To: Jerry D, gfortran; +Cc: gcc-patches

Hi Jerry, hello world,

Jerry D wrote:
> On 4/5/24 10:47 AM, Jerry D wrote:
>> On 4/4/24 2:41 PM, Tobias Burnus wrote:
>>> I think for the current testcases, I like the patch – the question 
>>> is only what's about:
>>>    ',3' as input for 'comma'   (or '.3' as input for 'point')
>>> [...]
>>> But for 'comma': [...]
>>> * GCC with your patch: Same result: ios != 0 and nothing read.
>>>
>>> Expected: [...] read-in value is 0.3. [...]

> See attached updated patch.
> Regressions tested on x86-64. OK for trunk and 13 after a bit.

OK. Thanks for the patch!

Tobias


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] (was: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'")
  2024-04-06  2:38         ` Jerry D
  2024-04-06  5:17           ` Tobias Burnus
@ 2024-04-08  9:53           ` Tobias Burnus
  2024-04-08 18:21             ` [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] Jerry D
  1 sibling, 1 reply; 11+ messages in thread
From: Tobias Burnus @ 2024-04-08  9:53 UTC (permalink / raw)
  To: Jerry D, gfortran; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Jerry D wrote:
> See attached updated patch.

It turned rather quickly out that this patch – committed as 
r14-9822-g93adf88cc6744a – caused regressions.

Namely, real-world code use tab(s) as separator instead of spaces.

[For instance, PR114304 which contains a named-list input file from SPEC 
CPU 2017; that example uses tabs before the '=' sign, but the issue is 
more generic.]

I think the ISO Fortran standard only permits spaces, but as it feels 
natural and is widely supported, tabs are used and should remain supported.

It is not quite clear how '\r' are or should be handled, but as 
eat_spaces did use it, I thought I would add one testcase using them as 
well.

That test is not affected by my change; it did work before with GCC and 
still does – but it does fail with ifort/ifx/flang. I have not thought 
deeply whether it should be supported or not – and looking at the 
libgfortran source file, it often but (→ testcase) not consistently 
requires that an \n follows the \r.

OK for mainline? [And: When the previous patch gets backported, this 
surely needs to be included as well.]

Tobias

[-- Attachment #2: fix-tab-io.diff --]
[-- Type: text/x-patch, Size: 3467 bytes --]

Fortran: Accept again tab as alternative to space as separator [PR114304]

This fixes a side-effect of/regression caused by r14-9822-g93adf88cc6744a,
which was for the same PR.

	PR libfortran/114304

libgfortran/ChangeLog:

	* io/list_read.c (eat_separator): Accept tab as alternative to space.

gcc/testsuite/ChangeLog:

	* gfortran.dg/pr114304-2.f90: New test.

 gcc/testsuite/gfortran.dg/pr114304-2.f90 | 82 ++++++++++++++++++++++++++++++++
 libgfortran/io/list_read.c               |  2 +-
 2 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gfortran.dg/pr114304-2.f90 b/gcc/testsuite/gfortran.dg/pr114304-2.f90
new file mode 100644
index 00000000000..5ef5874f528
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr114304-2.f90
@@ -0,0 +1,82 @@
+! { dg-do run }
+!
+! PR fortran/114304
+!
+! Ensure that '\t' (tab) is supported as separator in list-directed input
+! While not really standard conform, this is widely used in user input and
+! widely supported.
+!
+
+use iso_c_binding
+implicit none
+character(len=*,kind=c_char), parameter :: tab = C_HORIZONTAL_TAB
+
+! Accept '<tab>' as variant to ' ' as separator
+! Check that <carriage_return><new line> and <new_line> are handled
+
+character(len=*,kind=c_char), parameter :: nml_str &
+   = '&inparm'//C_CARRIAGE_RETURN // C_NEW_LINE // &
+     'first'//tab//'='//tab//' .true.'// C_NEW_LINE // &
+     ' , other'//tab//' ='//tab//'3'//tab//', 2'//tab//'/'
+
+! Check that <carriage_return> is handled,
+
+! Note: For new line, Unix uses \n, Windows \r\n but old Apple systems used '\r'
+!
+! Gfortran does not seem to support all \r, but the following is supported
+! since ages, ! which seems to be a gfortran extension as ifort and flang don't like it.
+
+character(len=*,kind=c_char), parameter :: nml_str2 &
+   = '&inparm'//C_CARRIAGE_RETURN // C_NEW_LINE // &
+     'first'//C_NEW_LINE//'='//tab//' .true.'// C_CARRIAGE_RETURN // &
+     ' , other'//tab//' ='//tab//'3'//tab//', 2'//tab//'/'
+
+character(len=*,kind=c_char), parameter :: str &
+   = tab//'1'//tab//'2,'//tab//'3'//tab//',4'//tab//','//tab//'5'//tab//'/'
+character(len=*,kind=c_char), parameter :: str2 &
+   = tab//'1'//tab//'2;'//tab//'3'//tab//';4'//tab//';'//tab//'5'//tab//'/'
+logical :: first
+integer :: other(4)
+integer :: ints(6)
+namelist /inparm/ first , other
+
+other = 1
+
+open(99, file="test.inp")
+write(99, '(a)') nml_str
+rewind(99)
+read(99,nml=inparm)
+close(99, status="delete")
+
+if (.not.first .or. any (other /= [3,2,1,1])) stop 1
+
+other = 9
+
+open(99, file="test.inp")
+write(99, '(a)') nml_str2
+rewind(99)
+read(99,nml=inparm)
+close(99, status="delete")
+
+if (.not.first .or. any (other /= [3,2,9,9])) stop 2
+
+ints = 66
+
+open(99, file="test.inp", decimal='point')
+write(99, '(a)') str
+rewind(99)
+read(99,*) ints
+close(99, status="delete")
+
+if (any (ints /= [1,2,3,4,5,66])) stop 3
+
+ints = 77 
+
+open(99, file="test.inp", decimal='comma')
+write(99, '(a)') str2
+rewind(99)
+read(99,*) ints
+close(99, status="delete")
+
+if (any (ints /= [1,2,3,4,5,77])) stop 4
+end
diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index b56f2a4e6d6..5bbbef26c26 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -463,7 +463,7 @@ eat_separator (st_parameter_dt *dtp)
 
   dtp->u.p.comma_flag = 0;
   c = next_char (dtp);
-  if (c == ' ')
+  if (c == ' ' || c == '\t')
     {
       eat_spaces (dtp);
       c = next_char (dtp);

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304]
  2024-04-08  9:53           ` [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] (was: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'") Tobias Burnus
@ 2024-04-08 18:21             ` Jerry D
  0 siblings, 0 replies; 11+ messages in thread
From: Jerry D @ 2024-04-08 18:21 UTC (permalink / raw)
  To: Tobias Burnus, gfortran; +Cc: gcc-patches, Jerry D

On 4/8/24 2:53 AM, Tobias Burnus wrote:
> Jerry D wrote:
>> See attached updated patch.
> 
> It turned rather quickly out that this patch – committed as r14-9822-g93adf88cc6744a – caused regressions.
> 
> Namely, real-world code use tab(s) as separator instead of spaces.
> 
> [For instance, PR114304 which contains a named-list input file from SPEC CPU 2017; that example uses tabs before the '=' sign, but the issue is more generic.]
> 
> I think the ISO Fortran standard only permits spaces, but as it feels natural and is widely supported, tabs are used and should remain supported.
> 
> It is not quite clear how '\r' are or should be handled, but as eat_spaces did use it, I thought I would add one testcase using them as well.
> 
> That test is not affected by my change; it did work before with GCC and still does – but it does fail with ifort/ifx/flang. I have not thought deeply whether it should be supported or not – and 
> looking at the libgfortran source file, it often but (→ testcase) not consistently requires that an \n follows the \r.
> 
> OK for mainline? [And: When the previous patch gets backported, this surely needs to be included as well.]
> 
> Tobias

Good catch. I did not even think about tabs.

OK to commit and I will take care of it when I do the backport to 13.

Thanks!

Jerry

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-04-08 18:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-04  1:33 [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'" Jerry D
2024-04-04  8:17 ` Paul Richard Thomas
2024-04-04  9:31 ` Tobias Burnus
2024-04-04 20:05   ` Jerry D
2024-04-04 21:04   ` Jerry D
2024-04-04 21:41     ` Tobias Burnus
2024-04-05 17:47       ` Jerry D
2024-04-06  2:38         ` Jerry D
2024-04-06  5:17           ` Tobias Burnus
2024-04-08  9:53           ` [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] (was: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'") Tobias Burnus
2024-04-08 18:21             ` [Patch] Fortran: List-directed read - accept again tab as alternative to space as separator [PR114304] Jerry D

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).