[PATCH 3/3] Cygwin: tzcode resync v2: details

[Corinna Vinschen]

Hi Mark,

On May 18 22:02, Mark Geisert wrote:
> Add tz_posixrules.h with data generated from most recent Cygwin tzdata
> package.  Establish localtime.cc as primarily a wrapper around a patched
> copy of localtime.c.  See README for more information.

The idea is nice, but there are a few problems.

> diff --git a/winsup/cygwin/tzcode/localtime.c.patch b/winsup/cygwin/tzcode/localtime.c.patch
> new file mode 100644
> index 000000000..a17d9ee90
> --- /dev/null
> +++ b/winsup/cygwin/tzcode/localtime.c.patch
> @@ -0,0 +1,399 @@
> +*** localtime.c	2020-05-16 21:54:00.533111800 -0700
> +--- localtime.c.patched	2020-05-16 22:42:40.486924300 -0700
> +***************
> +*** 1,3 ****

Ideally this patch should be in unified (-u) diff format.

I also wonder if some of these patches may be dropped or made
less invasive, given that localtime.cc wraps localtime.c.

For example:

> +***************
> +*** 23,29 ****
> +  
> +  /*LINTLIBRARY*/
> +  
> +- #include "namespace.h"

Adding a local namesapce.h file?

> +  #include <assert.h>
> +  #define LOCALTIME_IMPLEMENTATION
> +  #include "private.h"
> +--- 24,29 ----
> +***************
> +*** 182,188 ****
> +  
> +  
> +  #if !defined(__LIBC12_SOURCE__)
> +! timezone_t __lclptr;
> +  #ifdef _REENTRANT
> +  rwlock_t __lcl_lock = RWLOCK_INITIALIZER;
> +  #endif
> +--- 182,188 ----
> +  
> +  
> +  #if !defined(__LIBC12_SOURCE__)
> +! static timezone_t __lclptr;

Do we really need this static?  Even if it's not static, it won't
be exported from the DLL anyway, so it seems we can drop this, no?

> +  #ifdef _REENTRANT
> +  rwlock_t __lcl_lock = RWLOCK_INITIALIZER;
> +  #endif
> +***************
> +*** 198,204 ****
> +  
> +  static struct tm	tm;
> +  
> +! #if !HAVE_POSIX_DECLS || TZ_TIME_T || defined(__NetBSD__)
> +  # if !defined(__LIBC12_SOURCE__)
> +  
> +  __aconst char *		tzname[2] = {
> +--- 198,204 ----
> +  
> +  static struct tm	tm;
> +  
> +! #if !HAVE_POSIX_DECLS || TZ_TIME_T || defined(__NetBSD__) || defined(__CYGWIN__)

What about just #defining TZ_TIME_T or HAVE_POSIX_DECLS in localtime.cc
or even (see above) namespace.h?

> +  # if !defined(__LIBC12_SOURCE__)
> +  
> +  __aconst char *		tzname[2] = {
> +***************
> +*** 413,419 ****
> +  };
> +  
> +  /* TZDIR with a trailing '/' rather than a trailing '\0'.  */
> +! static char const tzdirslash[sizeof TZDIR] = TZDIR "/";

Yay, this one is tricky (and soooo dumb)

> +  
> +  /* Local storage needed for 'tzloadbody'.  */
> +  union local_storage {
> +--- 413,420 ----
> +  };
> +  
> +  /* TZDIR with a trailing '/' rather than a trailing '\0'.  */
> +! static char const tzdirslash[] = TZDIR "/";
> +! #define sizeof_tzdirslash (sizeof tzdirslash - 1)

What about this instead:

  - static char const tzdirslash[sizeof TZDIR] = TZDIR "/";
  + static char const tzdirslash[sizeof TZDIR + 1] = TZDIR "/";

checking what "sizeof tzdirslash" is used for, it doesn't matter
at all:

> +***************
> +*** 428,434 ****
> +  
> +  	/* The file name to be opened.  */
> +  	char fullname[/*CONSTCOND*/BIGGEST(sizeof (struct file_analysis),
> +! 	    sizeof tzdirslash + 1024)];

This makes fullname one byte longer than in NetBSD...

> +*** 466,479 ****
> +  	if (!doaccess) {
> +  		char const *dot;
> +  		size_t namelen = strlen(name);
> +! 		if (sizeof lsp->fullname - sizeof tzdirslash <= namelen)

lsp->fullname is one byte longer, tzdirslash is one byte longer,
therefore

  Cygwin:  sizeof lsp->fullname - sizeof tzdirslash

is equivalent to 

  NetBSD: (sizeof lsp->fullname + 1) - (sizeof tzdirslash + 1)
  ==       sizeof lsp->fullname + 1 - sizeof tzdirslash - 1
  ==       sizeof lsp->fullname - sizeof tzdirslash

So it's in fact the same expression and no change is required.

> +  			return ENAMETOOLONG;
> +  
> +  		/* Create a string "TZDIR/NAME".  Using sprintf here
> +  		   would pull in stdio (and would fail if the
> +  		   resulting string length exceeded INT_MAX!).  */
> +! 		memcpy(lsp->fullname, tzdirslash, sizeof tzdirslash);
> +! 		strcpy(lsp->fullname + sizeof tzdirslash, name);

This could then be changed to just

  -		strcpy(lsp->fullname + sizeof tzdirslash, name);
  +		strcpy(lsp->fullname + sizeof tzdirslash - 1, name);

> +***************
> +*** 488,498 ****
> +  		name = lsp->fullname;
> +  	}
> +  	if (doaccess && access(name, R_OK) != 0)
> +! 		return errno;
> +  
> +  	fid = open(name, OPEN_MODE);
> +  	if (fid < 0)
> +! 		return errno;
> +  	nread = read(fid, up->buf, sizeof up->buf);
> +  	if (nread < (ssize_t)tzheadsize) {
> +  		int err = nread < 0 ? errno : EINVAL;
> +--- 489,499 ----
> +  		name = lsp->fullname;
> +  	}
> +  	if (doaccess && access(name, R_OK) != 0)
> +! 		goto trydefrules;
> +  
> +  	fid = open(name, OPEN_MODE);
> +  	if (fid < 0)
> +! 		goto trydefrules;
> +  	nread = read(fid, up->buf, sizeof up->buf);
> +  	if (nread < (ssize_t)tzheadsize) {
> +  		int err = nread < 0 ? errno : EINVAL;
> +***************
> +*** 501,506 ****
> +--- 502,516 ----
> +  	}
> +  	if (close(fid) < 0)
> +  		return errno;
> ++ 	if (0) {
> ++ trydefrules:
> ++ 		const char *base = strrchr(name, '/');
> ++ 		base = base ? base + 1 : name;
> ++ 		if (strcmp(base, TZDEFRULES))
> ++ 		    return errno;
> ++ 		nread = sizeof _posixrules_data;
> ++ 		memcpy(up->buf, _posixrules_data, nread);
> ++ 	}

Couldn't this be wrapped in localtime.cc?  If this function in
localtime.c returns an error, just do the trydefrules stuff?
Just an idea, that may be too tricky depending on context.

> +*** 793,799 ****
> +  static int
> +  tzload(char const *name, struct state *sp, bool doextend)
> +  {
> +! 	union local_storage *lsp = malloc(sizeof *lsp);
> +  	if (!lsp)
> +  		return errno;
> +  	else {
> +--- 803,809 ----
> +  static int
> +  tzload(char const *name, struct state *sp, bool doextend)
> +  {
> +! 	union local_storage *lsp = (union local_storage *) calloc(1, sizeof *lsp);

This is where -Wall -Werror is getting annoying.  We should contemplate
to drop -Werror for localtime.cc, or better to drop certain warnings by
using `#pragma GCC diagnostic ignored "..." from within localtime.cc to
allow certain warnings to go unpunished.

I'm also not quite sure anymore why we use calloc instead of malloc
where malloc is sufficient for the original code.  We should probably
just drop this (but better check every call).


These are just examples, but the idea is clear I guess.  The less
changes in localtime.c, the better for maintenance.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer