From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel Subject: Re: Windows 64 port Date: Wed, 29 Feb 2012 19:23:46 -0800 Organization: UCLA Computer Science Department Message-ID: <4F4EEBC2.5070704@cs.ucla.edu> References: <20120219211800.0000558f@unknown> <834numv7js.fsf@gnu.org> <83ty2ltep0.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1330572249 29155 80.91.229.3 (1 Mar 2012 03:24:09 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 1 Mar 2012 03:24:09 +0000 (UTC) Cc: Eli Zaretskii , ajmr@ilovetortilladepatatas.com, emacs-devel@gnu.org To: Fabrice Popineau Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Mar 01 04:24:08 2012 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1S2wch-00039l-T4 for ged-emacs-devel@m.gmane.org; Thu, 01 Mar 2012 04:24:08 +0100 Original-Received: from localhost ([::1]:46414 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2wch-00013P-9E for ged-emacs-devel@m.gmane.org; Wed, 29 Feb 2012 22:24:07 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:54716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2wcd-00013F-WD for emacs-devel@gnu.org; Wed, 29 Feb 2012 22:24:06 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2wcb-00061L-Kq for emacs-devel@gnu.org; Wed, 29 Feb 2012 22:24:03 -0500 Original-Received: from smtp.cs.ucla.edu ([131.179.128.62]:39860) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2wcX-0005zb-OL; Wed, 29 Feb 2012 22:23:58 -0500 Original-Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id BEEB5A60008; Wed, 29 Feb 2012 19:23:46 -0800 (PST) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Original-Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id b2f7FOa1a1dj; Wed, 29 Feb 2012 19:23:45 -0800 (PST) Original-Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 4DDF4A60004; Wed, 29 Feb 2012 19:23:45 -0800 (PST) User-Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 131.179.128.62 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:148845 Archived-At: 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 before defining tzname, to avoid DLL clash. */ #include #define tzname _tzname This should avoid the diagnostic, since tzname isn't defined until after 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 ", 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 . */ > > +#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 . */ > > -#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.