Re: Guile 64-bit Windows support, redux

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9604 bytes --]



Op 06-06-2023 om 22:50 schreef Mike Gran:
> Hello Guile,
> 
> There have been a few times where I made Guile work on
> MinGW -- usually the 32-bit, unthreaded variety -- because I wanted it
> for my own entertainment.  Often around the time of the Lisp Game Jam.
> The Game Jam just happened, so I was poking it again.  Although
> I never actually delivered my game this year.
> 
> Janneke has also poked at Windows support a few times as well, and
> he actually got it to run on 64-bit Windows.
> 
> I'd be willing [1] to update the patches for MinGW support for
> review and inclusion into the main branch
> for 3.0.10 or 3.2 but, only if maintainers are open, in principle,
> to committing the most disruptive patch: the one where all
> long integers become intptr_t to deal with Window's stupid
> 4-byte longs on 64-bit OSs.  Without agreeing on that as a
> possibility, 64-bit Windows support is DOA.

(The following response should be read with the following proverb in mind:

<https://en.wiktionary.org/wiki/de_beste_stuurlui_staan_aan_wal>
Proverb
de beste stuurlui staan aan wal

    1. It's easy to criticize other people's work when you're not the 
one doing it. Compare also a backseat driver.

I haven't tried any alternative approached myself.)

(The following response is a bit muddled -- it initially says I don't 
follow but later I will.  Still, there is some stuff of which I really 
disagree with the implementation.)

I don't quite follow why the change from 'long' to 'intptr_t'.  I mean,
reading the commit message you linked to:

 > As long is used for SCM_FIXNUM_BIT, that would mean incompatible
 > .go files, [...]

... how is this a problem?  .go files are already incompatible between 
architectures and Guile already supports cross-compilation; adding 
incompatibility between OS doesn't really make a difference.

(For other readers: SCM_FIXNUM_BIT has been renamed to SCM_I_FIXNUM_BIT.)

Also:

 > [...] and a waste of cell space.

IIUC, Guile currently uses a 'long' (minus a few bits for typing) 
embedded in a SCM/scm_t_bits/uintptr_t for fixnums.  If 
sizeof(long)<sizeof(uintptr_t), that's indeed a waste (I assume that's 
what you are referring to).

In that case, why not change the fixnum implementation to use 
'uintptr_t' or 'scm_t_bits' instead of 'long'?  Reading a bit further,
such a thing appears to already have thought of:

 > So we would like to use long long, but the GMP interface
 > uses long.

and the new problem is already resolved.  Though I don't get why a new 
SCM_INTPTR_T_BIT is defined instead of changing SCM_FIXNUM_BIT to use 
(64-bit) uintptr_t instead of (32-bit) long -- I mean, wasn't 
compatibility and space-savings (i.e. avoiding bignums) a concern?

Also, why not patch GMP to also have 'long long' variants?  Sure, it 
will take a while to reach distributions ... but Windows doesn't really 
do distributions much anyway, so that doesn't seem a problem to me.
That would avoid a lot of GMP-related changes and avoid the need for the 
non-standard minigmp on Windows.

(There are some downsides, like having to pick different functions 
depending on whether 'long' or 'long long' is used, though to a degree 
that can be resolved with some #define.  Maybe that was one of the 
reasons why not.)

Looking at the changelog, I'm thinking it changes too much: changing 
configure.ac and {numbers,integers}.{c,h}, sure, but why why hash.c, 
symbols.c, array-map.c?

It also changes too little: it forgets to change scm_t_inum from 'long' 
to 'uintptr_t' -- if you want to have uintptr_t (minus type bits) 
fixnums, then you need to change scm_t_inum, otherwise the comment 
becomes invalid:

/* Immediate Numbers, also known as fixnums
  *
  * Inums are exact integers that fit within an SCM word
  * (along with two tagging bits).
  *
  * In the current implementation, Inums must also fit within a long
  * because that's what GMP's mpz_*_si functions accept.  */
typedef long scm_t_inum;

(It also becomes invalid because with the new minigmp, minigmp accepts 
uintptr_t instead of long.)

Looking closer at some of them, the changes to array_compare makes sense 
to me, even if Windows didn't have 32-bit longs, but some of the other 
changes don't.

For example, the first change is to scm_array_in_bounds_p.  Now you can 
have INTPTR_T_MAX dimensions instead of LONG_MAX dimensions!  Seems 
useless to me; 2**32 is already a stupidly high limit.

The change to array_to_list makes sense to me, even if long were 64-bit.

Now consider the change to scm_array_contents:

 > -                  SCM_I_ARRAY_BASE (ra) % SCM_LONG_BIT ||
 > -                  len % SCM_LONG_BIT))
 > +                  SCM_I_ARRAY_BASE (ra) % SCM_INTPTR_T_BIT ||
 > +                  len % SCM_INTPTR_T_BIT))

Neither version makes much sense to me -- SCM_I_ARRAY_BASE nominally 
returns a size_t, not an intptr_t:

#define SCM_I_ARRAY_BASE(a) ((size_t) SCM_CELL_WORD_2 (a))

so it should use SCM_SIZE_T_BIT instead of SCM_LONG_BIT/SCM_INTPTR_T_BIT.

Sure, Guile assumes that intptr=size_t elsewhere, but it's an assumption 
that can trivially be avoided here.

Now, on to libguile/bytevectors.c:

+#if !(__MINGW32__ && __x86_64__)
  #define is_signed_int32(_x)     (((_x) >= -2147483648L) && ((_x) <= 
2147483647L))
  #define is_unsigned_int32(_x)   ((_x) <= 4294967295UL)
+#else /* (__MINGW32__ && __x86_64__) */
+#define is_signed_int32(_x)     (((_x) >= -2147483648LL) && ((_x) <= 
2147483647LL))
+#define is_unsigned_int32(_x)   ((_x) <= 4294967295ULL)
+#endif /* (__MINGW32__ && __x86_64__) */

These #if / #else suck.  What if there exists some other system where 
long is 32-bit?

They are also unnecessary, it is possible to just use LL/ULL 
unconditionally (Guile already assumes that 'long long' exists, in 
test-num2integral.c):

#define is_signed_int32(_x)     (((_x) >= -2147483648LL) && ((_x) <= 
2147483647LL))
#define is_unsigned_int32(_x)   ((_x) <= 4294967295ULL)
#define is_unsigned_int32(_x)   ((_x) <= 4294967295ULL)

Also, in twos_complement, the implementation is confusingly written -- 
the MPZ API accepts longs, not uintptr, going by the official 
documentation. There is a reason for this, but it needs comments for 
people not aware that Guile replaces the GMP implementation on Windows.

About libguile/deprecated.c: this change is unnecessary if the 
definition of scm_t_inum is fixed.

About libguile/hash.c / libguile/hash.h: I don't get the type changes -- 
if 32-bit hashes have too high risk of collisions, why not also have 
64-bit hashes on 32-bit architectures instead of only when uintptr_t is 
64-bit?  It looks like an overly general search+replace long->intptr_t 
to me.

About libguile/integers.c:

+#if !(__MINGW32__ && __x86_64__)
+#define L1 1L
+#else /* (__MINGW32__ && __x86_64__) */
+#define L1 1LL
+#endif /* (__MINGW32__ && __x86_64__) */

Again, these #if/#else suck. Looking at the uses of L1, L1 should be an 
uintptr_t -- this code doesn't need to know whether L1 is long or long 
long, it just needs to be an intptr_t. It can be replaced by INTPTR_C(1) 
instead -- ok, bah, INTPTR_C doesn't actually exist, but you can define 
it in terms of INT<N>_C and some (cross-platform!!) #if/#else that 
compare UINTN_MAX with UINTPTR_MAX.

(Likewise, the configure.ac test is bad.)

Also, because integers.c deals with fixnums, it should be using 
scm_t_inum instead of intptr_t.

About scmsigs.c: unless signal numbers have more than 32-bits on 
Windows, I don't think that change accomplishes anything.

About srfi-60.c: unless the goal was to improve performance by using 
larger integers, these changes don't seem to accomplish anything.  And 
if the goal is performance, then this needs a benchmark.

About strings.h and symbols.c: see my comments about hashes.

Overall, I think there are some places were Guile really needs to stop 
using 'long' and instead use a more specific type like 'size_t' / 
'uintptr_t', for example when representing sizes and offsets of 
array-like stuff, whether or not Guile is ported to Windows.

> To get around this, the x86-64-MinGW port now requires the use of
> mini-GMP.  Mini-GMP has been changed to use intptr_t and uintptr_t.

Then the local copy should state that, otherwise people updating Guile's 
minigmp will introduce bugs.

> Let me know what you think.

More cross-platform support = good and out-of-tree = inconvenient, but 
the current patch makes some questionable choices and doesn't document 
stuff properly; it's really inconvenient for a newcomer from, say, 2050, 
to have to dig through multiple decades of commits and e-mails to avoid 
pitfalls like ‘this comment used to be true but now is a lie’ and 
‘hidden assumptions’ when doing some refactoring or porting.

Also, regardless of whether the uintptr stuff is accepted, there seem to 
be some Windows-independent patches that seem fit for the main branch:

  * Avoid mysterious "error no error" message in pipe
  * fixes for chmodat test

(Also, not actually a maintainer myself.)

> -Mike Gran
> 
> [1] Last two times I volunteered myself for something on Guile/Guix,
> I got quite ill. Let's not hope for a repeat.

Ill, like in, bad social interactions, or like in independent medical 
issues that happened at the same time as working on Guile/Guix stuff?
I don't need to know per se but this footnote is ambiguous.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]