Remove unneessary and possibly dangerious overshadowing of mp_bitcnt_t #557
Conversation
|
Thanks. |
|
Thank you! |
|
Could you be more specific? Just blithely overshadowing typedefs with macros is generally not a workable solution.
Yes, of course, or else I wouldn't have noticed ;) I'm working in Cygwin which depending on what you mean by "Windows" is not necessarily the same thing. The question is why anyone thinks this is needed on Windows because chances are it's an X-Y problem. |
|
The problem as I understand it is, GMP defines this type to be 32 bits on Windows, whereas MPIR defines it to be 64 bits. Flint was originally written for use with MPIR and later we decided to support GMP. But this type is used literally thousands of times throughout Flint, with the assumption (as per MPIR) that the size is the same as an mp_limb_t (or ulong in Flint). I see that just redefining it as we were is a pain in the neck for you, as any code that #includes gmp.h and flint.h will probably end up with the Flint definition of this type which might not be intended. But it's a tricky job to fix the problem, not the least source of which is that this type is used in the prototype of functions defined in .h files in Flint which are included from outside Flint. I guess the only proper solution is to replace every instance of mp_bitcnt_t in Flint (including code_conventions.txt, all .c and .h files and the docs) with flint_mp_bitcnt_t and hope that doesn't break anyone's code outside of Flint. |
|
"GMP defines this type to be 32 bits on Windows" => "GMP defines this type to be 32 bits on 64 bit Windows" |
|
@embray I presume you are doing a 64 bit Cygwin port? As that seems to be the only way you could hit this problem. |
|
I use mp_bitcnt_t with the assumption that it is unsigned and can hold the bit count of any fmpz_t. no other assumptions (I hope). |
|
If you don't use very large mpn's that is a safe assumption. |
|
Are you saying that the bit count of an fmpz could overflow a mp_bitcnt_t? Because of mpir? I based my assumptions on https://gmplib.org/manual/Nomenclature-and-Types.html |
|
No, I'm saying the bit count of an mpn could overflow an mp_bitcnt_t on Windows 64 bit. That is not anything to do with MPIR. |
|
Surely that is gmp's fault then, right? |
|
It's a design decision I think. |
|
This design decision is a real head-scratcher. I hope that I have used mp_bitcnt_t with only the following assumptions: The crux is point (4). If one squares 2^(2^32-2) on win64 with gmp, what does it in principal do? If it throws, then all is well. If it doesn't then I would consider this a bug in gmp. |
|
All of this is presupposing that it makes sense to |
|
Flint originally used only MPIR not GMP. We therefore could always assume an mp_bitcnt_t was the same size as an mp_limb_t. Not so when we decided to also support GMP. GMP uses two types to represent counts, an mp_size_t and an mp_bitcnt_t. The first is for counting a number of limbs and is always an int. On most machines, that's 32 bits. So you might think GMP supports integers up to 2^31 limbs. But it doesn't. Because of the limitations of mp_bitcnt_t, for representing bit counts, it can't represent integers that big on a 32 bit machine. On a 64 bit machine, it is supposed to support much larger integers. But because of its historical poor support for Windows, this is not true on a 64 bit Windows box. Hence MPIR exists, to fix some of these problems. And that is why Flint always used MPIR, and why we have these historical problems. The shadowing was a historical patch designed to save effort in fixing something that should never have been a problem in the first place. |
|
@embray Perhaps you are not aware of the history of this, but I was involved in the fork of GMP (called MPIR), way back. I was for many years the maintainer of MPIR (and still am some kind of defacto maintainer, though certainly not an active one nowadays). |
|
Note that the GMP manual states that GMP will fix this problem in the future, i.e. an mp_bitcnt_t will be 64 bits on a 64 bit Windows box. There are still other significant issues on 64 bit Windows that they haven't fixed yet, which MPIR fixes, but that will be progress. |
|
I'm aware of some of the history (I have submitted fixes to MPIR you know) though some of these details were helpful. But that still doesn't explain, just on a nitty-gritty level, why that define of mp_bitcnt_t was needed. I mean even if using MPIR doesn't it still define that in its headers? And why would removing it break the windows build, as @thofma believes? |
|
@embray, see #557 (comment). FLINT assumes that mp_bitcnt_t and mp_limb_t have the same size. Removing this define leads to overflow issues as you can see at https://ci.appveyor.com/project/isuruf/flint2/builds/24127096/job/rexkrtnuu73ybsgx |
|
After a quick search of the docs, I could only find one function that uses |
|
Ah, maybe you missed the discussion on the other PR @embray . There are pointer casts between |
I can confirm. |
|
@isuruf I meant the actual solution of getting your hands dirty and fixing functions that use mp_bitcnt_t incorrectly (i.e. with the assumption that it is the same as ulong). If you just replace all |
|
This should be fixed now. Thanks to Daniel Schultz @tthsqe12 all the Windows tests now also pass. |
AFAICT this line can be traced back to f988d3e where it wasn't added for any immediately obvious reason (perhaps it was a temporary workaround to some other problem).
It was later updated to read
#define mp_limb_t ulongwhich is dangerous sinceulongis defined in flint.h as#define ulong mp_limb_t, where it's not necessarily the case thatmp_limb_tis the same underlying type asmp_bitcnt_tdepending on how GMP was configured.