[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Guile 64-bit Windows support, redux
From: |
Maxime Devos |
Subject: |
Re: Guile 64-bit Windows support, redux |
Date: |
Thu, 8 Jun 2023 21:50:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 |
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.
OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature