[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[McIDAS #LRH-227193]: Incorrect typing of 3rd parameter to Mcdaytimetosec

Hi DaveP, DaveS, and Rick,

DaveP said:
> We actually did exactly what Tom is suggesting; change time_t to int.  I don't
> think we ever tried to be exhaustive in the search/replace, we just fixed the
> ones that jumped out at the time or that failed testing.

That is exactly what I did to begin with... I ran into problems serving GVAR
data off of unidata2.ssec.wisc.edu when I built McX v2007x in 64-bit mode.  I
reported weird behavior to the Data Center folks when I first saw the problem
thinking that it was related to the data being sent via LDM to unidata2.  When
DC folks, including Dee, sent back a note that indicated that some things worked
and some things didn't, I figured that the problem was with the newly rebuilt
code.  I rebuilt in 32-bit mode and the problems went away.  That's when I dug
into the gvaradir.cp and gvaraget.cp code.  Voila, I found the 'time_t *' typing
of parameters to Mcdaytimetosec while the function declaration in daytime.c
specified an 'int *'.

I didn't look any farther until I discussed the situation with Scott L. and
Russ D. on Tuesday morning last week.  Scott did a quick grep of source code
and noticed the problem in poesadir.cp and poesaget.cp.  When I returned from
Madison, I did made a complete survey of the source code and found the errors
in just under half of the routines that called Mcdaytimetosec.  While looking
through daytime.c, I noticed that the comment section for Mcdaytimetosec 
incorrectly
stated the need for use of a 'time_t *' as the third argument.  I then verified
that the man page for Mcdaytimetosec called for a 'time_t *' for the third
argument.  I also must note that the Remarks section of the Mcdaytimetosec
documentation is incorrect:

*$      This function should work fine until approximately the year
*$      2099. At that time the number of seconds since 1 Jan 1970 will
*$      no longer fit in a 32 bit unsigned integer.

This would be true if the parameter used was an 'unsigned int'.  Since it
is _not_ an 'unsigned int', the 'stop working' year is 2038.

> I don't know if "time_t" is technically *incorrect*, it's just that some
> compilers/headers don't have correct size definitions for time_t (and size_t) 
> on
> 64bit platforms.  I think.

The use of a 'time_t *' variable is only incorrect because the function 
declaration
(and function prototype in mcidas.h) specifies use of an 'int *'.  If there was
no Fortran interface, then use of 'time_t *' would have been preferable.

Since the size of a 'time_t *' is implementation dependent, one can not assume 
that
it will be the same size as an 'int *' which is pretty standard.  It is 
possible that
use of a 'time_t *' is "OK" (meaning it works) on some 64-bit systems and 
incorrect
(meaning it doesn't work) on others. Not passing the type called for in the 
function
declaration is an error no matter what.

DaveS asked:
> DaveP: Didn't we have problems with Fortran calling C on 64-bit when the
> C routine expected time_t? How was is resolved?

It appears that someone rewrote the Mcdaytimetosec routine so that the Fortran
interface to it, mcdaytimetosec_, could pass along the argument that it 
received.
It seems to me that whoever rewrote Mcdaytimetosec (or first implemented it) did
it the hard way: it would have been easier to populate a 'tm' structure with
the information passed in and then call 'mktime'.  Since the return value from
'mktime' is a 'time_t', one would have to convert back to an 'int' so that the
Fortran interface would work correctly.

By the way, I implemented all of the changes included in the list that I sent
to DaveS and rebuilt Unidata McIDAS-X on all of the systems that I have access
to.  I have seen no problems with the modifications on any of the systems 
involved
and those include:

32-bit machines
----------------
cacimbo.ggy.uga.edu            - RedHat Enterprise WS 4 Linux
buran.ggy.uga.edu              - MacOS-X
weather.admin.niu.edu          - Fedora 7 Linux
weather2.admin.niu.edu         - Fedora 7 Linux
weather3.admin.niu.edu         - Fedora 7 Linux
zeldagillroy.silversprings.net - Fedora Core 6 Linux (my home development 
machine)

64-bit machines
-------------------
yakov.unidata.ucar.edu         - Fedora Core 5 Linux
adde.ucar.edu (aka motherlode) - SunOS 5.9 SPARC
unidata2.ssec.wisc.edu         - SunOS 5.8 SPARC
stratus.al.noaa.gov            - SunOS 10 x86_64
idd.unl.edu                    - Fedora Core 6 Linux

Since adde.ucar.edu, unidata2.ssec.wisc.edu, and stratus.al.noaa.gov get a LOT
of exercise serving data via ADDE, I am pretty confident that the fixes
(time_t -> int) have had no unintended side effects.

Cheers,

Tom
****************************************************************************
Unidata User Support                                    UCAR Unidata Program
(303) 497-8642                                                 P.O. Box 3000
address@hidden                                   Boulder, CO 80307
----------------------------------------------------------------------------
Unidata HomePage                       http://www.unidata.ucar.edu
****************************************************************************


Ticket Details
===================
Ticket ID: LRH-227193
Department: Support McIDAS
Priority: Normal
Status: Closed