[Bug fortran/47633] New: Result of COMPILER_VERSION() has NULL byte appended

[Bug fortran/47633] New: Result of COMPILER_VERSION() has NULL byte appended

[thenlich at users dot sourceforge.net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

           Summary: Result of COMPILER_VERSION() has NULL byte appended
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: thenlich@users.sourceforge.net


The string returned by the COMPILER_VERSION function of the ISO_FORTRAN_ENV
module has a NULL byte at the last character position.

Expected result: Just the string, without the extra null byte.

PROGRAM TESTENV
    USE ISO_FORTRAN_ENV
    CHARACTER(LEN=256) V
    INTEGER L
    V = COMPILER_VERSION()
    L = LEN(COMPILER_VERSION())
    PRINT "(A)", V(1:L-1)
    PRINT *, ICHAR(V(L:L))
END PROGRAM TESTENV

GCC version 4.6.0 20110205 (experimental) [trunk revision 169855]
           0

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[kargl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011.02.07 16:02:20
                 CC|                            |kargl at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from kargl at gcc dot gnu.org 2011-02-07 16:02:20 UTC ---
Confirmed.  Here's a tentative patch and testcase.

! { dg-do run }
! PR fortran/47633
program testenv
    use iso_fortran_env
    character(len=60) v
    integer n
    v = compiler_version()
    n = len(compiler_version())
    if (ichar(v(n:n)) /= 32) call abort()
end program testenv


troutmask:sgk[291] svn diff simplify.c
Index: simplify.c
===================================================================
--- simplify.c  (revision 169830)
+++ simplify.c  (working copy)
@@ -6837,7 +6837,6 @@ gfc_simplify_compiler_options (void)
   return result;
 }

-
 gfc_expr *
 gfc_simplify_compiler_version (void)
 {
@@ -6845,8 +6844,10 @@ gfc_simplify_compiler_version (void)
   size_t len;

   len = strlen ("GCC version ") + strlen (version_string) + 1;
-  buffer = (char*) alloca (len);
+  buffer = (char *) alloca (len);
   snprintf (buffer, len, "GCC version %s", version_string);
+  /* Remove the terminating NULL character. */
+  buffer[strlen(buffer)] = ' ';
   return gfc_get_character_expr (gfc_default_character_kind,
                                 &gfc_current_locus, buffer, len);
 }

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-07 16:11:58 UTC ---
Why should there be the extra space at the end?  Doesn't make sense.
IMHO either pass len - 1 to gfc_get_character_expr or don't count '\0' in len
(i.e. remove the + 1) and use len + 1 in alloca.  You shouldn't use alloca btw,
but XALLOCAVEC (char, len).

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[sgk at troutmask dot apl.washington.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

--- Comment #3 from Steve Kargl <sgk at troutmask dot apl.washington.edu> 2011-02-07 17:43:25 UTC ---
On Mon, Feb 07, 2011 at 04:12:00PM +0000, jakub at gcc dot gnu.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-07 16:11:58 UTC ---
> Why should there be the extra space at the end?  Doesn't make sense.
> IMHO either pass len - 1 to gfc_get_character_expr or don't count '\0' in len
> (i.e. remove the + 1) and use len + 1 in alloca.  You shouldn't use alloca btw,
> but XALLOCAVEC (char, len).
> 

You're right. I could do the len - 1, but when I tried that
I misinterpreted the results of the test program.  Here's an
update diff and testcase.  I'll start a regression test shortly,
and submit a patch for approval.

troutmask:sgk[218] svn diff simplify.c
Index: simplify.c
===================================================================
--- simplify.c  (revision 169830)
+++ simplify.c  (working copy)
@@ -6845,8 +6845,8 @@ gfc_simplify_compiler_version (void)
   size_t len;

   len = strlen ("GCC version ") + strlen (version_string) + 1;
-  buffer = (char*) alloca (len);
+  buffer = XALLOCAVEC (char, len);
   snprintf (buffer, len, "GCC version %s", version_string);
   return gfc_get_character_expr (gfc_default_character_kind,
-                                &gfc_current_locus, buffer, len);
+                                &gfc_current_locus, buffer, len - 1);
 }

troutmask:sgk[407] cat h.f90
! { dg-do run }
! PR fortran/47633
program testenv
    use iso_fortran_env
    character(len=60) v
    integer n
    v = compiler_version()
    n = len(compiler_version())
    if (ichar(v(n:n)) /= 41 .or. ichar(v(n+1:n+1)) /= 32) call abort()
end program testenv

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-07 18:05:36 UTC ---
The testcase is bad, because for vanilla gcc releases there is no (prerelease)
etc. string at all (DEV-PHASE is empty), and because it hardcodes ASCII values,
so I'm afraid it could fail on EBCDIC or other targets.

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-02-07 18:58:24 UTC ---
>     if (ichar(v(n:n)) /= 41 .or. ichar(v(n+1:n+1)) /= 32) call abort()

(In reply to comment #4)
> The testcase is bad, because for vanilla gcc releases there is no (prerelease)
> etc. string at all (DEV-PHASE is empty), and because it hardcodes ASCII values,
> so I'm afraid it could fail on EBCDIC or other targets.

Regarding the char: using "iachar" instead of "ichar" should work.

 * * *

   len = strlen ("GCC version ") + strlen (version_string) + 1;
+  buffer = XALLOCAVEC (char, len);
   snprintf (buffer, len, "GCC version %s", version_string);
   return gfc_get_character_expr (gfc_default_character_kind,
+                                &gfc_current_locus, buffer, len - 1);

I wonder whether one should strip the "+1" in the first line and use "len+1"
for alloca/snprintf; it seems to be easier to follow than the "len - 1" in the
last line.

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[sgk at troutmask dot apl.washington.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

--- Comment #6 from Steve Kargl <sgk at troutmask dot apl.washington.edu> 2011-02-07 19:19:37 UTC ---
On Mon, Feb 07, 2011 at 06:58:39PM +0000, burnus at gcc dot gnu.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633
> 
> >     if (ichar(v(n:n)) /= 41 .or. ichar(v(n+1:n+1)) /= 32) call abort()
> 
> (In reply to comment #4)
> > The testcase is bad, because for vanilla gcc releases there is no (prerelease)
> > etc. string at all (DEV-PHASE is empty), and because it hardcodes ASCII values,
> > so I'm afraid it could fail on EBCDIC or other targets.
> 
> Regarding the char: using "iachar" instead of "ichar" should work.

That still doesn't help.  The testcase is looking for
the last valid character in the string and a following
space.  Jakub's point is that '(experimental)' in the string
'GCC version 4.6.0 20110204 (experimental)' is sometimes not
present, so checking for ')' won't work in all situations.

I'll suggest that we simply omit a testcase for this PR.

>    len = strlen ("GCC version ") + strlen (version_string) + 1;
> +  buffer = XALLOCAVEC (char, len);
>    snprintf (buffer, len, "GCC version %s", version_string);
>    return gfc_get_character_expr (gfc_default_character_kind,
> +                                &gfc_current_locus, buffer, len - 1);
> 
> I wonder whether one should strip the "+1" in the first line and use "len+1"
> for alloca/snprintf; it seems to be easier to follow than the "len - 1" in the
> last line.

Sure, no problem.  Here's a tested patch.

Index: simplify.c
===================================================================
--- simplify.c  (revision 169830)
+++ simplify.c  (working copy)
@@ -6844,9 +6844,9 @@ gfc_simplify_compiler_version (void)
   char *buffer;
   size_t len;

-  len = strlen ("GCC version ") + strlen (version_string) + 1;
-  buffer = (char*) alloca (len);
-  snprintf (buffer, len, "GCC version %s", version_string);
+  len = strlen ("GCC version ") + strlen (version_string);
+  buffer = XALLOCAVEC (char, len + 1);
+  snprintf (buffer, len + 1, "GCC version %s", version_string);
   return gfc_get_character_expr (gfc_default_character_kind,
                                 &gfc_current_locus, buffer, len);
 }

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[tkoenig at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

Thomas Koenig <tkoenig at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tkoenig at gcc dot gnu.org

--- Comment #7 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2011-02-12 23:29:25 UTC ---
(In reply to comment #4)
> The testcase is bad, because for vanilla gcc releases there is no (prerelease)
> etc. string at all (DEV-PHASE is empty), and because it hardcodes ASCII values,
> so I'm afraid it could fail on EBCDIC or other targets.

I don't think gfortran supports EBCDIC very well right now.  At least
the LLE and other functions don't take this into account.

subroutine ff(a,b,r1,r2)
  character(2) :: a,b
  logical :: r1, r2
  r1 = llt(a,b)
  r2 = a < b
end subroutine ff

gets translated in *.original to

{
  *r1 = _gfortran_compare_string (2, a, 2, b) < 0;
  *r2 = _gfortran_compare_string (2, a, 2, b) < 0;
}

This might merit its own PR.

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[sgk at troutmask dot apl.washington.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

--- Comment #8 from Steve Kargl <sgk at troutmask dot apl.washington.edu> 2011-02-13 00:05:09 UTC ---
On Sat, Feb 12, 2011 at 11:29:47PM +0000, tkoenig at gcc dot gnu.org wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633
> 
> Thomas Koenig <tkoenig at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |tkoenig at gcc dot gnu.org
> 
> --- Comment #7 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2011-02-12 23:29:25 UTC ---
> (In reply to comment #4)
> > The testcase is bad, because for vanilla gcc releases there is no (prerelease)
> > etc. string at all (DEV-PHASE is empty), and because it hardcodes ASCII values,
> > so I'm afraid it could fail on EBCDIC or other targets.
> 
> I don't think gfortran supports EBCDIC very well right now.  At least
> the LLE and other functions don't take this into account.
> 

These functions specifically assume ascii.  For Fortran 2003:

  The intrinsic functions LGT, LGE, LLE, and LLT (13.7.63-13.7.66)
  provide comparisons between strings based on the ASCII collating
  sequence.  International portability is guaranteed if the set of
  characters used is limited to the letters, digits, underscore,
  and special characters.

See also Note 4.15:

  The intrinsic functions ACHAR (13.7.2) and IACHAR (13.7.45)
  provide conversion between characters and corresponding
  integer values according to the ASCII collating sequence.

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

--- Comment #9 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-02-14 17:16:16 UTC ---
(In reply to comment #6)
> Sure, no problem.  Here's a tested patch.

The patch is OK with a ChangeLog. I think no test case is needed.

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[kargl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

--- Comment #10 from kargl at gcc dot gnu.org 2011-02-15 19:38:54 UTC ---
Author: kargl
Date: Tue Feb 15 19:38:51 2011
New Revision: 170195

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170195
Log:
2011-02-15  Steven G. Kargl  <kargl@gcc.gnu.org>

    PR fortran/47633
    . simplify.c (gfc_simplify_compiler_version): Fix off-by-one issue.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/simplify.c

[Bug fortran/47633] Result of COMPILER_VERSION() has NULL byte appended

[kargl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47633

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.6.0

--- Comment #11 from kargl at gcc dot gnu.org 2011-02-15 19:44:44 UTC ---
Fixed on trunk.  Closing.