-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
32-bit builds with warnings as errors are broken #1353
Comments
#987 left too little space for object allocation I think, after that we decided to leave 32-bit support aside for the moment. The initial change of 2<<32` was also to make it consistent with the comment, but it turned out that a real solution would be more complex |
I see. That probably means that #1330 is too simplistic then, if it keeps the |
not necessarily, but the comment should be consistent with the code, which wasn't the case. Also |
Not sure if it's solved. It should be possible to do a cross-compile with CMake (which might be a good nightly build candidate...), but I haven't done that before so it will take me some time to work out how to do it. I'll give it a go if I have time. |
The following completes without any problems. Hopefully it's enough to resolve this issue.
|
@reuk Thank you very much for undertaking this experiment! |
Currently it is not possible to produce a 32-bit build of CBMC with
-Wall -Wpedantic -Werror
set. This is because there is a cast from2<<32
tosize_t
which gets truncated to0
on 32-bit platforms (ininterpreter.cpp
).There have been several attempts to fix this problem.
The first attempt, #987, solves the problem by reducing the shift amount to
1<<32
. It is not clear why this PR was not accepted.The second attempt, #1330, keeps the shift amount at
2<<32
and attempts to solve the problem by usingmp_integer
wherever possible in the interpreter in order to avoid truncation altogether. However, to facilitate minimal changes in this patch, the binary operators inmp_integer
had to be changed to allow conversions on both sides of operators (it's surprising to haveBigInt + size_t
work, butsize_t + BigInt
result in a compile error - associative operators should be really associative). As a result, #1330 depends on #1341.There were concerns about the performance implications of the new
mp_integer
operators in #1341, which may introduce conversions which weren't required before. It is not practical for me to build a benchmarking suite (this would be an enormous undertaking and I do not have the necessary expertise in either benchmarking or the customer requirements), and so #1341 will not be merge-able in the near future.The text was updated successfully, but these errors were encountered: