Re: Windows 64 port

[Paul Eggert <eggert@cs.ucla.edu>]

On 02/28/2012 01:32 PM, Fabrice Popineau wrote:

> If I define tzname as _tzname is ms-w32.h, I get an error when
> including MS time.h on some function returning an array.

It should be possible to work around this problem without having to
modify lib/strftime.c.  For example, src/s/ms-w32.h can do something like this:

   /* Include <time.h> before defining tzname, to avoid DLL clash.  */
   #include <time.h>
   #define tzname _tzname

This should avoid the diagnostic, since tzname isn't defined until
after <time.h> is included.  It's better to keep Microsoft-specific
stuff in Microsoft-specific source files when possible, as seems to
be the case here.


>     >  #ifdef WINDOWSNT
>     >  extern Lisp_Object w32_get_internal_run_time (void);
>     > +
>     > +extern struct tm *localtime (const time_t *t);
>     >  #endif
> 
>     Why is this needed?  It seems also unrelated to 64-bit Windows.
> 
> To remove a warning on localtime not define.

Surely this should be done via "#include <time.h>", not by repeating
some of the internals of time.h.

> - const problems in regex.c (large patch, but harmless)

None of the regex.c changes are needed for Windows 64.  The code works
fine without it and the patch does not fix any bugs.  Perhaps regex.c
should be made cleaner, const-wise, but that's an independent issue
and if we want to make that change it be as part of an independent patch.

From now on, when I say "Not needed for Windows 64", I mean that any
such patch should be proposed independently of the Windows 64 patch,
since the code works fine as-is on Windows 64.  This is not to say or
imply that the patch isn't helpful; just that it should be considered
separately and independently.

> - turn off compiler warnings of 3 kinds (harmless, but frequent in emacs code)

Not needed for Windows 64, since these warnings are harmless.


Here is a more-detailed review of the latest patch you sent.

> -      int i = 0, aligned = (intptr_t) ABLOCKS_BUSY (abase);
> +      int i = 0, aligned = (ABLOCKS_BUSY (abase) != NULL);

Not needed for Windows 64.  The change slows the code down a bit, and
it might not work on unusual hosts that have filler bits in their
pointers.  Anyway, if we're going to change the usage of ABLOCKS_BUSY
when it is (ab)used as an integer, such a change should be systematic,
and not just to the single use that MSVC happens to diagnose.

> -allocate_pseudovector (int memlen, int lisplen, EMACS_INT tag)
> +allocate_pseudovector (size_t memlen, size_t lisplen, EMACS_INT tag)

Not needed for Windows 64, since the arguments are all in int range.

> -	    int size = min (2000, max (20, (nextb->text->z_byte / 10)));
> +	    size_t size = min (2000, max (20, (nextb->text->z_byte / 10)));

Not needed for Windows 64, since 'size' is in int range.

> -		make_gap (-(nextb->text->gap_size - size));
> +		make_gap ((size - nextb->text->gap_size));

Not needed for Windows 64, since the expressions are equivalent.

> +      extern Lisp_Object x_get_focus_frame(struct frame *);

This should be done via an include file, not by putting an
extern function declaration inside some block.

> -  unsigned long int adj;
> +  intptr_t adj;

Not needed for Windows 64; the value is always in unsigned long range.

> -  adj = (unsigned long int) ((unsigned long int) ((char *) result -
> -						  (char *) NULL)) % BLOCKSIZE;
> +  adj = (intptr_t) (((char *) result -
> +                     (char *) NULL)) % BLOCKSIZE;

Not needed for Windows 64, since BLOCKSIZE is a power of 2 and the
arithmetic is well-defined.  Also, the proposed replacement code is
wrong, since it might make adj go negative.

> -    _heapinfo[block + blocks].busy.info.size = -blocks;
> +    _heapinfo[block + blocks].busy.info.size = -((__malloc_ptrdiff_t)blocks);

Not needed for Windows 64.

> - 	      (*__morecore) (-size);
> + 	      (*__morecore) (-((__malloc_ptrdiff_t)size));

Not needed for Windows 64.

> - 	  (*__morecore) (- newsize * sizeof (malloc_info));
> + 	  (*__morecore) (- ((ptrdiff_t)(newsize * sizeof (malloc_info))));

Not needed for Windows 64.

> -	    _heapinfo[block].busy.info.frag.first = (unsigned long int)
> -	      ((unsigned long int) ((char *) next->next - (char *) NULL)
> +	    _heapinfo[block].busy.info.frag.first = (intptr_t)
> +	      (((char *) next->next - (char *) NULL)
>  	       % BLOCKSIZE) >> log;

Not needed for Windows 64, since BLOCKSIZE is a small power of 2.
Also, the replacement mishandles the sign bit of the pointer.

> -	_heapinfo[block + blocks].busy.info.size = -blocks;
> +	_heapinfo[block + blocks].busy.info.size = -((ptrdiff_t)blocks);

Not needed for Windows 64.

> -	      (*__morecore) (-bytes);
> +	      (*__morecore) (-((__malloc_ptrdiff_t)bytes));

Not needed for Windows 64.

> -	  _heapinfo[block].busy.info.frag.first = (unsigned long int)
> -	    ((unsigned long int) ((char *) ptr - (char *) NULL)
> +	  _heapinfo[block].busy.info.frag.first = (intptr_t)
> +	    ((intptr_t) ((char *) ptr - (char *) NULL)
>  	     % BLOCKSIZE >> type);

Not needed for Windows 64.  Also, mishandles the sign bit of the difference.

> +#ifdef min
> +#undef min
> +#endif

Why is this needed?  If it's merely to suppress a warning then it's
not needed for Windows 64.  If not, please explain.

> -  unsigned long int adj, lastadj;
> +  intptr_t adj, lastadj;

Not needed for Windows 64.

> -  adj = (unsigned long int) ((char *) result - (char *) NULL) % alignment;
> +  adj = (intptr_t) ((char *) result - (char *) NULL) % alignment;

Not needed for Windows 64, since the alignment is a small power of two.
(This is also buggy with the sign bit.)

> -      adj = (unsigned long int) ((char *) result - (char *) NULL) % alignment;
> +      adj = (intptr_t) ((char *) result - (char *) NULL) % alignment;

Not needed for Windows 64, since the alignment is a small power of two.
(This is also buggy with the sign bit.)

> -    unsigned long int magic;	/* Magic number to check header integrity.  */
> +    uint64_t magic;	/* Magic number to check header integrity.  */

Not needed for Windows 64, since the magic number fits in 32 bits.

> --- src/image.c	2012-02-15 06:40:08 +0000
> +++ src/image.c	2012-02-19 21:54:33 +0000
> @@ -16,6 +16,10 @@
>  You should have received a copy of the GNU General Public License
>  along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> +#ifdef _MSC_VER
> +#define _W32COMPAT_H_ 1
> +#endif

Why is this patch needed in mainline code?  Can't it be put into
src/s/ms-w32.h?

> -#define XFASTINT(a) ((a) + 0)
> +#define XFASTINT(a) (((a) + 0))

Not needed at all.  (Didn't we already discuss this one?)

> -extern struct Lisp_Vector *allocate_pseudovector (int memlen, int lisplen, EMACS_INT tag);
> +extern struct Lisp_Vector *allocate_pseudovector (size_t memlen, size_t lisplen, EMACS_INT tag);

Not needed for Windows 64, since the values fit in 'int'.

> === modified file 'src/m/amdx86-64.h'
> --- src/m/amdx86-64.h	2012-01-19 07:21:25 +0000
> +++ src/m/amdx86-64.h	2012-02-28 07:00:30 +0000
> @@ -17,7 +17,13 @@
>  You should have received a copy of the GNU General Public License
>  along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> -#define BITS_PER_LONG           64
> +#ifdef _WIN64
> +# define BITS_PER_LONG           32
> +# define BITS_PER_LONG_LONG      64
> +#else
> +# define BITS_PER_LONG           64
> +#endif

Since this stuff is normally defined by 'configure', it should be put
into nt/config.nt rather than into a src/m/* file; there shouldn't be
a need to use or to modify src/m/amdx86-64.h.  Similarly for the other
changes to src/m/amdx86-64.h.  I don't see how that file is used under
Windows 64; but if it is, we should fix that, and not change the file.

> +#define SIZE ESIZE

I don't see the reason for this #define; could you please explain?
If it is needed, let's just dump SIZE entirely, and use size_t instead.

> -#define ALIGNED(addr) (((unsigned long int) (addr) & (page_size - 1)) == 0)
> -#define ROUNDUP(size) (((unsigned long int) (size) + page_size - 1) \
> +#define ALIGNED(addr) (((intptr_t) (addr) & (page_size - 1)) == 0)
> +#define ROUNDUP(size) (((intptr_t) (size) + page_size - 1) \
>  		       & ~(page_size - 1))

Please use uintptr_t rather than intptr_t, since the original uses
the unsigned types.  That's safer.

> -#define MEM_ROUNDUP(addr) (((unsigned long int)(addr) + MEM_ALIGN - 1) \
> +#define MEM_ROUNDUP(addr) (((intptr_t)(addr) + MEM_ALIGN - 1) \
>  				   & ~(MEM_ALIGN - 1))

Likewise, use uintptr_t not intptr_t.

> -long
> +intptr_t
>  r_alloc_size_in_use (void)

ptrdiff_t, not intptr_t, since it's arrived at by subtracting pointers.

> === modified file 'src/regex.c'
> --- src/regex.c	2012-01-19 07:21:25 +0000
> +++ src/regex.c	2012-02-27 23:33:18 +0000

None of the changes to this file are needed for Windows 64.

>  #ifdef REL_ALLOC
> -  extern POINTER (*real_morecore) (long);
> +  extern POINTER (*real_morecore) (__malloc_ptrdiff_t);
>  #endif
> -  extern POINTER (*__morecore) (long);
> +  extern POINTER (*__morecore) (__malloc_ptrdiff_t);

I don't see how this change could work, since __malloc_ptrdiff_t is
not visible in vm-limit.c.  I suggest just using ptrdiff_t.