[Bug fortran/47552] New: CTIME: Valgrind warning "depends on uninitialised value"

[Bug fortran/47552] New: CTIME: Valgrind warning "depends on uninitialised value"

[burnus at gcc dot gnu.org]

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

           Summary: CTIME: Valgrind warning "depends on uninitialised
                    value"
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: burnus@gcc.gnu.org
                CC: jb@gcc.gnu.org


The following does not seem to be a regression and it only occurs with the
function version and not with the subroutine version.

character(len=20) :: str
! OK:
!call ctime(time8(),str)

! Valgrind issue (2x line 7):
! Conditional jump or move depends on uninitialised value
str = ctime(time8()) ! << this line
print *, str
end

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[jb at gcc dot gnu.org]

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

Janne Blomqvist <jb at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.01.31 12:57:07
     Ever Confirmed|0                           |1

--- Comment #1 from Janne Blomqvist <jb at gcc dot gnu.org> 2011-01-31 12:57:07 UTC ---
FWIW, I see the same problem with gfortran 4.4 @work, so probably not something
due to my recent ctime_r() patch.

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[jb at gcc dot gnu.org]

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

--- Comment #2 from Janne Blomqvist <jb at gcc dot gnu.org> 2011-02-22 15:34:57 UTC ---
This seems to be because the libgfortran implementation uses gfc_charlen_type
for the length of the string, but the frontend passes the address of an
integer(8) variable. As a quick and dirty test, changing the libgfortran
implementation to use "long" removes the valgrind errors on x86_64. Though the
correct fix is to change the frontend to create a variable of the correct type.

If not before, perhaps something to fix when/if we change to use size_t for
string lengths, see http://gcc.gnu.org/wiki/LibgfortranAbiCleanup

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[fxcoudert at gcc dot gnu.org]

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

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
                 CC|                            |fxcoudert at gcc dot
                   |                            |gnu.org

--- Comment #3 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> 2011-03-11 21:32:33 UTC ---
(In reply to comment #2)
> This seems to be because the libgfortran implementation uses gfc_charlen_type
> for the length of the string,

Which is the correct thing the do!

> but the frontend passes the address of an
> integer(8) variable

Which is wrong. This is fixed by the patch:

Index: trans-intrinsic.c
===================================================================
--- trans-intrinsic.c    (revision 170612)
+++ trans-intrinsic.c    (working copy)
@@ -1501,7 +1501,7 @@
   args = XALLOCAVEC (tree, num_args);

   var = gfc_create_var (pchar_type_node, "pstr");
-  len = gfc_create_var (gfc_get_int_type (8), "len");
+  len = gfc_create_var (gfc_charlen_type_node, "len");

   gfc_conv_intrinsic_function_args (se, expr, &args[2], num_args - 2);
   args[0] = gfc_build_addr_expr (NULL_TREE, var);


> If not before, perhaps something to fix when/if we change to use size_t for
> string lengths, see http://gcc.gnu.org/wiki/LibgfortranAbiCleanup

Just as a remark: you're not going to use size_t, but the signed ssize_t,
right?

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[burnus at gcc dot gnu.org]

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

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-03-12 08:03:37 UTC ---
(In reply to comment #3)
> (In reply to comment #2)
> Which is wrong. This is fixed by the patch:

The patch is OK with a changelog and CCing the patch to gcc-patches@/fortran@

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[fxcoudert at gcc dot gnu.org]

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

--- Comment #5 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> 2011-03-12 10:28:04 UTC ---
Author: fxcoudert
Date: Sat Mar 12 10:28:01 2011
New Revision: 170898

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170898
Log:
        PR fortran/47552
        * trans-intrinsic.c (gfc_conv_intrinsic_ctime): Fix type of
        the string length variable.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-intrinsic.c

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[fxcoudert at gcc dot gnu.org]

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

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

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

--- Comment #6 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> 2011-03-12 10:35:59 UTC ---
Fixed on trunk. CTIME is a little used intrinsic, and we don't actually have
any example of real-world failure, so I don't plan on backporting the patch.

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[jb at gcc dot gnu.org]

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

--- Comment #7 from Janne Blomqvist <jb at gcc dot gnu.org> 2011-03-15 15:26:05 UTC ---
(In reply to comment #3)
> (In reply to comment #2)
> > If not before, perhaps something to fix when/if we change to use size_t for
> > string lengths, see http://gcc.gnu.org/wiki/LibgfortranAbiCleanup
> 
> Just as a remark: you're not going to use size_t, but the signed ssize_t,
> right?

I think the idea was to use the unsigned size_t. 

- size_t is the natural type for representing the size of a memory object in
bytes. There is no need for negative values, and SSIZE_MAX is smaller than the
largest possible object (=SIZE_MAX) (an obscure corner case, but still).

- size_t is what the C string.h functions use, which is used in the
implementation of various string handling functions in gfortran (in libgfortran
and code generated directly by the frontend). Using the same type might also
help mixed C/Fortran programs.

- size_t is what Intel Fortran uses

- Yes, Fortran itself does not have unsigned integers, but the string length
type is invisible to Fortran programs. The LEN intrinsic does return the string
length typecast to a signed integer kind depending on the optional KIND
argument, or to a default kind integer. Depending on whether the target
supports an integer kind > sizeof(size_t) it might be possible to represent all
possible string lengths, or then not. But IMHO this does not change the fact
that the correct type for the (Fortran invisible) string length is the unsigned
size_t. Also, consider that at the asm level, typecasting from an unsigned to a
signed value of the same size is a no-op, so there is no efficiency loss.

[Bug fortran/47552] CTIME: Valgrind warning "depends on uninitialised value"

[fxcoudert at gcc dot gnu.org]

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

--- Comment #8 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> 2011-03-15 15:36:16 UTC ---
(In reply to comment #7)
> - Yes, Fortran itself does not have unsigned integers, but the string length
> type is invisible to Fortran programs. The LEN intrinsic does return the string
> length typecast to a signed integer kind depending on the optional KIND
> argument, or to a default kind integer.

My point is just that, if you're merely changing the size of the variable, you
are less likely to unearth new bugs than if you change both that and
signedness.

Plus, as you say, the difference between SIZE_MAX and SSIZE_MAX is a corner
case, and maybe a huge size_t value isn't usable in common code.

Finally, you'd loose some compatibility with what used to be done

> Also, consider that at the asm level, typecasting from an unsigned to a
> signed value of the same size is a no-op, so there is no efficiency loss.

And casts from signed to unsigned, with checks for positivity, when string
lengths are passed as function arguments.