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: 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) 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_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.