From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Fabrice Popineau Newsgroups: gmane.emacs.devel Subject: Re: Windows 64 port Date: Thu, 1 Mar 2012 08:24:40 +0100 Message-ID: References: <20120219211800.0000558f@unknown> <834numv7js.fsf@gnu.org> <83ty2ltep0.fsf@gnu.org> <4F4EEBC2.5070704@cs.ucla.edu> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=0015175cf8b89d12af04ba295b37 X-Trace: dough.gmane.org 1330586740 16371 80.91.229.3 (1 Mar 2012 07:25:40 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 1 Mar 2012 07:25:40 +0000 (UTC) Cc: Eli Zaretskii , ajmr@ilovetortilladepatatas.com, emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Mar 01 08:25:38 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 1S30OQ-0001vv-55 for ged-emacs-devel@m.gmane.org; Thu, 01 Mar 2012 08:25:38 +0100 Original-Received: from localhost ([::1]:37114 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S30OP-0007tF-GT for ged-emacs-devel@m.gmane.org; Thu, 01 Mar 2012 02:25:37 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:47044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S30OL-0007t8-HA for emacs-devel@gnu.org; Thu, 01 Mar 2012 02:25:35 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S30OE-0001Iy-KM for emacs-devel@gnu.org; Thu, 01 Mar 2012 02:25:33 -0500 Original-Received: from mail-bk0-f41.google.com ([209.85.214.41]:34890) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S30Nr-00013q-6f; Thu, 01 Mar 2012 02:25:03 -0500 Original-Received: by bkwq16 with SMTP id q16so236217bkw.0 for ; Wed, 29 Feb 2012 23:25:00 -0800 (PST) Received-SPF: pass (google.com: domain of fabrice.popineau@gmail.com designates 10.204.157.5 as permitted sender) client-ip=10.204.157.5; Authentication-Results: mr.google.com; spf=pass (google.com: domain of fabrice.popineau@gmail.com designates 10.204.157.5 as permitted sender) smtp.mail=fabrice.popineau@gmail.com; dkim=pass header.i=fabrice.popineau@gmail.com Original-Received: from mr.google.com ([10.204.157.5]) by 10.204.157.5 with SMTP id z5mr2004863bkw.95.1330586700450 (num_hops = 1); Wed, 29 Feb 2012 23:25:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:from:date :x-google-sender-auth:message-id:subject:to:cc:content-type; bh=kbTmhOgblZ2o0UM3cJ/g/1bQx07q1ladnZJOsaPfO5g=; b=csLr9b1lAlgXPZ9iJks/dfkMddmbJ6KXGPClrIoYWGVS7zLo+g0u1Bs/J80TshgdS/ pKQF1ArUv93Usetoy/XWt7dNVr/B6Z3VDMHCYvyBv89E4q8EIZZHC2reC/LXm9YJTxOD CWh2BU6AgbSPX+C9t1URqqYHyQjyOZLO4tWEk= Original-Received: by 10.204.157.5 with SMTP id z5mr1614503bkw.95.1330586700288; Wed, 29 Feb 2012 23:25:00 -0800 (PST) Original-Received: by 10.204.60.3 with HTTP; Wed, 29 Feb 2012 23:24:40 -0800 (PST) In-Reply-To: <4F4EEBC2.5070704@cs.ucla.edu> X-Google-Sender-Auth: 2UdGIX5Fm6irSItdvzdqf8ZRu7w X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.214.41 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:148850 Archived-At: --0015175cf8b89d12af04ba295b37 Content-Type: text/plain; charset=ISO-8859-1 2012/3/1 Paul Eggert > 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. > > The problem is trickier than this and probably need a careful rewrite of ms-w32.h. In this file, names like tzname or fopen get rewired. Some of them need to be rewired to sys_fopen or _fopen (example) depending on whether it is for emacs or not. So a more robust way of doing things would be to ensure that all system headers are read once for all, then doing the rewiring for emacs and for (not emacs). That should avoid the problem of tzname for example. In the mean time, this is the smallest change I could come with. What you propose didn't work (resulting in either : some function returns an array, that is tzname, or _tzname being undefined at link time). > > > #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. > Sure but the definition needs to be put somewhere because an int is not the same size as a pointer in windows 64. > > - const problems in regex.c (large patch, but harmless) > > None of the regex.c changes are needed for Windows 64. The code works > > I anwsered in a separate message. I never claimed it is needed by Windows 64 either. (My only claim is that the whole thing I sent is what I needed to get a clean compile.) > - turn off compiler warnings of 3 kinds (harmless, but frequent in emacs > code) > > Not needed for Windows 64, since these warnings are harmless. > Sure, but unless the source code is fixed to avois the warnings, it gets me a cleaner compilation listing. > 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 > Prove it. I bet your compiler is much more clever than you think. twenty years ago, I would have agree, not today. > it might not work on unusual hosts that have filler bits in their > pointers. The it is because the compiler is buggy, because this is a perfectly legal and much more right than the double cascading cast (one explicit, one implicit). > -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. > Who knows that ? Certainly not the compiler. > > - 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. > Same remark. > > - make_gap (-(nextb->text->gap_size - size)); > > + make_gap ((size - nextb->text->gap_size)); > > Not needed for Windows 64, since the expressions are equivalent. Depends on signed/unsigned. > > - unsigned long int adj; > > + intptr_t adj; > > Not needed for Windows 64; the value is always in unsigned long range. > > I thank you for all those intptr_t -> uintptr_t catches. > > +#ifdef min > > +#undef min > > +#endif > > Just because it is redefined when it is a commonly defined macro in system headers. > > - 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. > > If it is a 32bits integer, then better make it this way everywhere. > > > -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'. > > Again, the compiler does not know it. > > +#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. > > Not needed, that was a redifinition of a system header macro. > > -long > > +intptr_t > > r_alloc_size_in_use (void) > > ptrdiff_t, not intptr_t, since it's arrived at by subtracting pointers. > > OK. > > > #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. > > It should be else I won't have been able to compile it. Anyway no problem with that. I'm not even sure why there is a __malloc_ptrdiff_t at all. Fabrice --0015175cf8b89d12af04ba295b37 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

2012/3/1 Paul Eggert &= lt;eggert@cs.ucla.edu><= br>
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. =A0For example, src/s/ms-w32.h can do something like= this:

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

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


The problem is= trickier than this and probably need a careful rewrite of ms-w32.h.
<= div>In this file, names like tzname or fopen get rewired. Some of them need= to
be rewired to sys_fopen or _fopen (example) depending on whether it is= for emacs or not.
So a more robust way of doing things would be = to ensure that all system headers are read once
for all, then doi= ng the rewiring for emacs and for (not emacs). That should avoid the proble= m
of tzname for example. In the mean time, this is the smallest change I= could come with.
What you propose didn't work (resulting in = either : some function returns an array, that is tzname,=A0
or _t= zname being undefined at link time).
=A0
> =A0 =A0 > =A0#ifdef WINDOWSNT
> =A0 =A0 > =A0extern Lisp_Object w32_get_internal_run_time (void); > =A0 =A0 > +
> =A0 =A0 > +extern struct tm *localtime (const time_t *t);
> =A0 =A0 > =A0#endif
>
> =A0 =A0 Why is this needed? =A0It seems also unrelated to 64-bit Windo= ws.

Sure but the definition needs to be = put somewhere because an int is not the same
size as a pointer in= windows 64.

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

None of the regex.c changes are needed for Windows 64. =A0The code wo= rks

I anwsered in a separa= te message. I never claimed it is needed by Windows 64 either.
(M= y only claim is that the whole thing I sent is what I needed to get a clean= compile.)

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

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

Sure, but unless the source code is fixed = to avois the warnings, it gets me a cleaner compilation listing.
=
=A0
Here is a more-detailed review of the latest patch you sent.

> - =A0 =A0 =A0int i =3D 0, aligned =3D (intptr_t) ABLOCKS_BUSY (abase);=
> + =A0 =A0 =A0int i =3D 0, aligned =3D (ABLOCKS_BUSY (abase) !=3D= NULL);

Not needed for Windows 64. =A0The change slows the code down a bit, and
=

Prove it. I bet your compiler is much more= clever than you think. twenty years ago, I would=A0
have agree, = not today.
=A0
it might not work on unusual hosts that have filler bits in their
pointers.

The it is because the compiler is= buggy, because this is a perfectly legal and much more
right tha= n the double cascading cast (one explicit, =A0one implicit).

> -allocate_pseudovector (int memle= n, int lisplen, EMACS_INT tag)
> +allocate_pseudovector (size_t memlen, size_t lisplen, EMACS_INT tag)<= br>
Not needed for Windows 64, since the arguments are all in int range.

Who knows that ? Certainly not the compiler.<= /div>
=A0
=A0
> - =A0 =A0 =A0 =A0 int size =3D min (2000, max (20, (nextb->text->= ;z_byte / 10)));
> + =A0 =A0 =A0 =A0 size_t size =3D min (2000, max (20, (nextb->text-= >z_byte / 10)));

Not needed for Windows 64, since 'size' is in int range.
=A0
Same remark.
=A0
> - =A0 =A0 =A0 =A0 =A0 =A0 make_gap (-(nextb->text->gap_size - si= ze));
> + =A0 =A0 =A0 =A0 =A0 =A0 make_gap ((size - nextb->text->gap_siz= e));

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

Depends on signed/unsigned.=A0

=A0
> - =A0unsigned long int adj;
> + =A0intptr_t adj;

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


I thank you for all those intptr_t -&g= t; uintptr_t catches.
=A0
>= ; +#ifdef min
> +#undef min
> +#endif


Just because it is redefined when it i= s a commonly defined macro in system headers.
=A0
> - =A0 =A0unsigned long int magic; /* Magic number to check header inte= grity. =A0*/
> + =A0 =A0uint64_t magic; =A0/* Magic number to check header integrity.= =A0*/

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

If it is a 32bits integer, then better make it this w= ay everywhere.
=A0

> -extern struct Lisp_Vector *allocate_pseudovector (int memlen, int lis= plen, 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'.


Again, the compiler does not know it.<= /div>
> +#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.=


Not needed, that was a redifinition of= a system header macro.

=A0
> -long
> +intptr_t
> =A0r_alloc_size_in_use (void)

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

OK.
=A0

> =A0#ifdef REL_ALLOC
> - =A0extern POINTER (*real_morecore) (long);
> + =A0extern POINTER (*real_morecore) (__malloc_ptrdiff_t);
> =A0#endif
> - =A0extern POINTER (*__morecore) (long);
> + =A0extern 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. =A0I suggest just using ptrdiff_t.


It should be else I won't have been a= ble to compile it. Anyway no problem with that.
I'm not even sure w= hy there is a __malloc_ptrdiff_t at all.

Fabrice
--0015175cf8b89d12af04ba295b37--