Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
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 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
Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
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 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. ios= 0
point, isreal = T , testinput = "3.3;"r= 3.2995 ios= 0
point, isreal = T , testinput = "3,3;"r= 3. 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. ios= 0
point, isreal = T , testinput = "4.4 ;"r= 4.4010 ios=0
point, isreal = T , testinput = "9 ;"r= 9. ios= 0
point, isreal = T , testinput = "4,4 ;"r= 4. 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! { 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 erro
[PATCH 0/1] libgfortran: Fix compilation of gf_vsnprintf
The fallback function (gf_vsnprintf) to provide a vsnprintf function if the system library doesn't have one would not compile due to the variable name for the string's destination buffer not being updated after the refactor in 2018 in edaaef601d0d6d263fba87b42d6d04c99dd23dba. This updates the internal logic of gf_vsnprintf to now use the str variable defined in the function signature. I am not actually sure what configurations are using this fallback, since it was added in 2018 and no patches have been made to fix this compilation error. Testing this also isn't straightforward, and I had to do a bit of a hack to get it to use the codepath to show the compilation error: 1) Configure and build as normal to generate the config.h header 2) Modify config.h directly to undefine HAVE_VSNPRINTF 3) Directly call the libgfortran compilation step Ian McInerney (1): libgfortran: Fix compilation of gf_vsnprintf libgfortran/runtime/error.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) base-commit: b7bd2ec73d66f7487bc8842b24daecaa802a72e6 -- 2.43.0
[PATCH 1/1] libgfortran: Fix compilation of gf_vsnprintf
The fallback function (gf_vsnprintf) to provide a vsnprintf function
if the system library doesn't have one would not compile due to the
variable name for the string's destination buffer not being updated
after the refactor in 2018 in edaaef601d0d6d263fba87b42d6d04c99dd23dba.
This updates the internal logic of gf_vsnprintf to now use the str
variable defined in the function signature.
libgfortran/ChangeLog:
2024-04-04 Ian McInerney
* runtime/error.c (gf_vsnprintf): Fix compilation
Signed off by: Ian McInerney
---
libgfortran/runtime/error.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index e840f539599..044298294d8 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -142,15 +142,15 @@ gf_vsnprintf (char *str, size_t size, const char *format,
va_list ap)
{
int written;
- written = vsprintf(buffer, format, ap);
+ written = vsprintf(str, format, ap);
if (written >= size - 1)
{
- /* The error message was longer than our buffer. Ouch. Because
+ /* The error message was longer than the string size. Ouch. Because
we may have messed up things badly, report the error and
quit. */
-#define ERROR_MESSAGE "Internal error: buffer overrun in gf_vsnprintf()\n"
- write (STDERR_FILENO, buffer, size - 1);
+#define ERROR_MESSAGE "Internal error: string overrun in gf_vsnprintf()\n"
+ write (STDERR_FILENO, str, size - 1);
write (STDERR_FILENO, ERROR_MESSAGE, strlen (ERROR_MESSAGE));
sys_abort ();
#undef ERROR_MESSAGE
--
2.43.0
Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
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. ios= 0 point, isreal = T , testinput = "3.3;"r= 3.2995 ios= 0 point, isreal = T , testinput = "3,3;"r= 3. 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. ios= 0 point, isreal = T , testinput = "4.4 ;"r= 4.4010 ios=0 point, isreal = T , testinput = "9 ;"r= 9. ios= 0 point, isreal = T , testinput = "4,4 ;"r= 4. 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
Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
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. ios= 0
point, isreal = T , testinput = "3.3;"r= 3.2995 ios= 0
point, isreal = T , testinput = "3,3;"r= 3. 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
commit 9c8318cd8703d49ad7563e89765f8849ebc14385
Author: Jerry DeLisle
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 000..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'
Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"
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!
Tobiasdiff --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
