Patch to expand tz checking scope in TimeZone_md.c

Mon Nov 14 18:22:42 UTC 2011

On Monday 14 November 2011 03:40:01 am Jonathan Lu wrote:
> Hi Kurt,
> 
> On 11/10/2011 11:09 AM, Kurt Miller wrote:
> > On 11/09/11 03:01, Jonathan Lu wrote:
> >> On 11/04/2011 01:26 PM, David Holmes wrote:
> >>> On 4/11/2011 2:53 PM, David Holmes wrote:
> >>>> On 4/11/2011 2:13 PM, Masayoshi Okutsu wrote:
> >>>>> Probably the difference isn't documented. I tried Solaris 10 and Ubuntu
> >>>>> 10.03. The difference still exists.
> >>>>>
> >>>>> Solaris 10:
> >>>>> $ unset TZ
> >>>>> $ date
> >>>>> Fri Nov 4 13:04:45 JST 2011
> >>>>> $ TZ="" date
> >>>>> Fri Nov 4 13:04:53 JST 2011
> >>>>>
> >>>>> Ubuntu 10.04:
> >>>>> $ unset TZ
> >>>>> $ date
> >>>>> Fri Nov 4 13:05:50 JST 2011
> >>>>> $ TZ="" date
> >>>>> Fri Nov 4 04:05:55 UTC 2011
> >>>>>
> >>>>> When the TZ value is an empty string, Ubuntu uses UTC while Solaris
> >>>>> still looks up the system default.
> >>>> I have to take back my comment regarding this not seeming to be platform
> >>>> specific code - it is highly platform specific! It seems that on Linux
> >>>> we are happy to report a TZ of "" but not so on Solaris. I presume this
> >>>> is an attempt to keep Java's use of TZ consistent with how other apps
> >>>> handle it on that platform. (environ(5) gives a little insight on
> >>>> Solaris as to how TZ is used.)
> >>>>
> >>>> So the key thing here is to not disturb the existing behaviour on either
> >>>> linux or Solaris - which suggests the original patch. That said I'm not
> >>>> convinced - given this is so platform specific - that simply treating
> >>>> non-linux the same as Solaris is a reasonable thing to do. I think it
> >>>> would be useful to see what the BSD/OSX port(s) had to do with this code
> >>>> - if anything.
> >>> To answer my own queries BSD/OSX does
> >>>
> >>>        511 #if defined(__linux__) || defined(_ALLBSD_SOURCE)
> >>>        512     if (tz == NULL) {
> >>>        513 #else
> >>>        514 #ifdef __solaris__
> >>>        515     if (tz == NULL || *tz == '\0') {
> >>>        516 #endif
> >>>        517 #endif
> >>>
> >>> so the suggested patch would at least not interfere.
> >>>
> >>> Anyway this needs input from other core-libs folk. I didn't intend to
> >>> get quite so heavily involved. ;-)
> >>>
> >>> David
> >>> -----
> >>>
> >>>
> >>>
> >>>> David
> >>>> -----
> >>>>
> >>>>
> >>>>> Thanks,
> >>>>> Masayoshi
> >>>>>
> >>>>> On 11/3/2011 4:16 PM, Jonathan Lu wrote:
> >>>>>> Hi Masayoshi,
> >>>>>>
> >>>>>> I did find some references about date-time related functions / TZ
> >>>>>> variables on Linux but got only a few about Solaris, so could not see
> >>>>>> any differences between those two platforms about the changes
> >>>>>> described in my patch. Have you got any links or references about
> >>>>>> these differences? I'm interested in it and may update the patch again
> >>>>>> after reading them.
> >>>>>>
> >>>>>> Thanks a lot!
> >>>>>> - Jonathan
> >>>>>>
> >>>>>> On 11/02/2011 10:27 PM, Masayoshi Okutsu wrote:
> >>>>>>> Hi Jonathan,
> >>>>>>>
> >>>>>>> IIRC, the difference came from some behavioral difference between the
> >>>>>>> Linux and Solaris libc date-time functions and/or the date command,
> >>>>>>> and TimeZone_md.c tries to follow the difference. But the code was
> >>>>>>> written looooong ago. The difference may no longer exist.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Masayoshi
> >>>>>>>
> >>>>>>> On 11/2/2011 8:39 PM, Jonathan Lu wrote:
> >>>>>>>> On 11/02/2011 07:00 PM, David Holmes wrote:
> >>>>>>>>> On 2/11/2011 7:01 PM, Jonathan Lu wrote:
> >>>>>>>>>> On 11/02/2011 04:56 PM, Jonathan Lu wrote:
> >>>>>>>>>>> Hi core-libs-dev,
> >>>>>>>>>>>
> >>>>>>>>>>> In jdk/src/solaris/native/java/util/TimeZone_md.c, starting from
> >>>>>>>>>>> line
> >>>>>>>>>>> 626, I found that the scope of "#ifdef __solaris__" might be too
> >>>>>>>>>>> narrow, since it also works for some kind of OS which I'm
> >>>>>>>>>>> currently
> >>>>>>>>>>> working on, such as AIX.
> >>>>>>>>>>> So I suggest to just remove the '#ifdef __solaris__' and leave
> >>>>>>>>>>> the
> >>>>>>>>>>> "#else" to accommodate more conditions, see attachment
> >>>>>>>>>>> 'patch.diff'. I
> >>>>>>>>>>> think this may enhance the cross-platform ability, any ideas
> >>>>>>>>>>> about
> >>>>>>>>>>> this modification?
> >>>>>>>>>>>
> >>>>>>>>>>> Regards
> >>>>>>>>>>> - Jonathan Lu
> >>>>>>>>>> I'm not sure why the attachment got filtered, here paste it to the
> >>>>>>>>>> mail
> >>>>>>>>>> content directly.
> >>>>>>>>>>
> >>>>>>>>>> diff -r 4788745572ef src/solaris/native/java/util/TimeZone_md.c
> >>>>>>>>>> --- a/src/solaris/native/java/util/TimeZone_md.c Mon Oct 17
> >>>>>>>>>> 19:06:53
> >>>>>>>>>> 2011 -0700
> >>>>>>>>>> +++ b/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20
> >>>>>>>>>> 13:43:47
> >>>>>>>>>> 2011 +0800
> >>>>>>>>>> @@ -626,10 +626,8 @@
> >>>>>>>>>> #ifdef __linux__
> >>>>>>>>>> if (tz == NULL) {
> >>>>>>>>>> #else
> >>>>>>>>>> -#ifdef __solaris__
> >>>>>>>>>> if (tz == NULL || *tz == '\0') {
> >>>>>>>>>> #endif
> >>>>>>>>>> -#endif
> >>>>>>>>>> tz = getPlatformTimeZoneID();
> >>>>>>>>>> freetz = tz;
> >>>>>>>>>> }
> >>>>>>>>> I'm unclear why any of that code needs to be platform specific - is
> >>>>>>>>> an empty TZ string somehow valid on linux? I would have thought the
> >>>>>>>>> following would be platform neutral:
> >>>>>>>>>
> >>>>>>>>> if (tz == NULL || *tz == '\0') {
> >>>>>>>>> tz = getPlatformTimeZoneID();
> >>>>>>>>> freetz = tz;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>> Hi David,
> >>>>>>>>
> >>>>>>>> getenv("TZ") returns NULL when TZ environment variable is not set at
> >>>>>>>> all and returns '\0' when TZ was exported as empty string. After
> >>>>>>>> more checking for both cases, I agree with you, nothing useful can
> >>>>>>>> be retrieved from that environment variable.
> >>>>>>>> So I changed the patch to this,
> >>>>>>>>
> >>>>>>>> diff -r 7ab0d613cd1a src/solaris/native/java/util/TimeZone_md.c
> >>>>>>>> --- a/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20 10:32:47
> >>>>>>>> 2011 -0700
> >>>>>>>> +++ b/src/solaris/native/java/util/TimeZone_md.c Wed Nov 02 19:34:51
> >>>>>>>> 2011 +0800
> >>>>>>>> @@ -623,13 +623,7 @@
> >>>>>>>>
> >>>>>>>> tz = getenv("TZ");
> >>>>>>>>
> >>>>>>>> -#ifdef __linux__
> >>>>>>>> - if (tz == NULL) {
> >>>>>>>> -#else
> >>>>>>>> -#ifdef __solaris__
> >>>>>>>> if (tz == NULL || *tz == '\0') {
> >>>>>>>> -#endif
> >>>>>>>> -#endif
> >>>>>>>> tz = getPlatformTimeZoneID();
> >>>>>>>> freetz = tz;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>> David
> >>>>>>>>> -----
> >>>>>>>>>
> >>>>>>>>>> Regards
> >>>>>>>>>> - Jonathan Lu
> >>>>>>>> Regards
> >>>>>>>>
> >>>>>>>> - Jonathan
> >>>>>>>>
> >> Copy to bsd-port-dev and macosx-port-dev lists to see if anybody here
> >> has some ideas about this issue.
> > Hi Jonathan,
> >
> > The above email is a bit hard to follow due to the mixture of top and
> > bottom replies.
> >
> > I can confirm that OpenBSD and Mac OS X 10.5.8 follow the Linux behavior
> > which confirms the need for platform ifdef's in this code.
> >
> > Seems like you need to make the following change:
> >
> > -#ifdef __solaris__
> > +#if defined(__solaris__) || defined(__AIX__)
> >
> > or something similar to maintain compatibility.
> >
> > In general the approach taken for adding BSD support was to never
> > assume you can change other supported code paths. If your architecture
> > follows an existing code path behavior add it like I did above.
> > Otherwise just create a #ifdef myarch section for it.
> >
> 
> But for this case, I think it is a good idea to leave a default code 
> path here.
> Since in src/solaris/native/java/util/TimeZone_md.c starting from line 
> 624, the return value of getenv("TZ"); has to be tested afterward on any 
> platforms.
> So to improve portability for OpenJDK, how about leaving Solaris style 
> checking as the default approach?

Ah I see what you're getting at now. As a bsd-port only committer my
opinion has limited value, but for what its worth I can give you my
perspective on the issue. The code base is large and there are
many places like this where unifying the behavior of different archs
is the goal. Keeping with the non-configure approach where ifdef
__MYARCH__ is used for differing behaviors then I see the ideal
situation being that the porter/developer is required to make the
correct choice for the new __MYARCH__ and no default be assumed.

If a default behavior is assumed, which one do you pick? Also what
happens when your done porting but many of the defaults don't 
match your new arch? I can tell you it is a lot of work to find the
bits of code embedded into the jdk code base to fix when the TCK
fails tests. I rather do that work up front and be forced to pick
the correct behavior due to a failure to compile. For example,
had this code compiled for you it is quite posible you would not
have know that there are different behaviors to pick from and to
select the correct one for you.

Regards,
-Kurt