Bug 47552 - CTIME: Valgrind warning "depends on uninitialised value"
CTIME: Valgrind warning "depends on uninitialised value"
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: fortran
4.6.0
: P3 normal
: 4.6.0
Assigned To: Not yet assigned to anyone
: patch, wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-31 12:50 UTC by Tobias Burnus
Modified: 2011-03-15 15:36 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-01-31 12:57:07


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2011-01-31 12:50:45 UTC
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
Comment 1 Janne Blomqvist 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.
Comment 2 Janne Blomqvist 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
Comment 3 Francois-Xavier Coudert 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?
Comment 4 Tobias Burnus 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@
Comment 5 Francois-Xavier Coudert 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
Comment 6 Francois-Xavier Coudert 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.
Comment 7 Janne Blomqvist 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.
Comment 8 Francois-Xavier Coudert 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.