-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use macros from limits.h
to prevent signed integer wrap-around warnigns
#13083
Conversation
limits.h
to prevent signed integer wrap-around warnigns
runtime/caml/config.h
Outdated
@@ -140,16 +140,19 @@ typedef unsigned char uint8_t; | |||
typedef long intnat; | |||
typedef unsigned long uintnat; | |||
#define ARCH_INTNAT_PRINTF_FORMAT "l" | |||
#define INTNAT_MIN LONG_MIN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried moving these defines to runtime/caml/misc.h
? runtime/caml/config.h
doesn't include <limits.h>
but these new macros depend on it, so it would make more sense to define them in a place where <limits.h>
is included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, it's true that config.h
is missing limits.h
, but adding the following to misc.h
also seems like wasted duplication.
#if SIZEOF_PTR == SIZEOF_LONG
/* Standard models: ILP32 or I32LP64 */
#define INTNAT_MIN LONG_MIN
#elif SIZEOF_PTR == SIZEOF_INT
/* Hypothetical IP32L64 model */
#define INTNAT_MIN INT_MIN
#elif SIZEOF_PTR == 8
/* Win64 model: IL32P64 */
#define INTNAT_MIN INT64_MIN
#endif
config.h
could include limits.h
instead, we've switched to C11, and most of the compatibility code around C99 integer types seems to have been added for old MSVC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preprocessor logic duplication is unfortunate but probably acceptable (with a comment telling it must match what's in config.h
) if adding <limits.h>
to config.h
is considered too large a change.
I think a Changes
entry will be required if config.h
now includes <limits.h>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opting to add limits.h
to config.h
. I think a follow-up PR could switch entirely to C99 fixed-width integers all the macros and defines of config.h
.
68289f9
to
d36498c
Compare
I'll review this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all good, a clear improvement.
d36498c
to
3b61291
Compare
Thanks, I've rebased on trunk and added you as a reviewer. |
What about using |
This makes sense to me, and could remove the test for While we're on the subject, it's surprising to me that we don't seem to have, or use, |
Right. <stdint.h> is standard since C99, and OCaml 5 requires C11, so we should use <stdint.h> unconditionally and remove the configure test for it. |
3b61291
to
1b08350
Compare
Two good suggestions. I've changed the definitions to use the |
What I meant about |
1b08350
to
3ba9f9b
Compare
I've rebased this PR.
I've introduced |
On reflection we shouldn't change |
My thoughts also, I'll remove that commit.
but on 64-bits arches, only |
3ba9f9b
to
b350290
Compare
2b71514
to
cc99c94
Compare
Rebased to fix conflicts, let me know if something can be improved. |
dcd6d26
to
c6ce70a
Compare
Gentle ping to the reviewers, how can we move forward with this PR? |
runtime/ints.c
Outdated
} else { | ||
if (res > (uint64_t)1 << 63) caml_failwith(INT64_ERRMSG); | ||
if (res > (uint64_t)INT64_MAX + UINT64_C(1)) caml_failwith(INT64_ERRMSG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two changes necessary? They make the code harder to read for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just to clarify: the github UI shows only one changed line above my comment, but there is a similar change right above in the diff.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two changes necessary?
No.
They make the code harder to read for me.
I hoped to convey more meaning with a name, and show that there's an error if res
is outside the range of int64_t
.
The second one can be written as (uint64_t)INT64_MAX + 1
if that's nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a comment above that says that we want to accept the range from -2^63 to 2^63-1. It is easy for me to understand why we want that, and to see the connection with the checks sign >= 0 && res >= (uint64_t)1 << 63
and sign < 0 && res > (uint64_t)1 << 63)
. On the other hand, to my untrained eyes the checks res > (uint64_t)INT64_MAX
and res > (uint64_t)INT64_MAX + 1
look confusing and hard to relate to the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted this change, and found this one-liner (which I've not included) for the whole block in the meantime:
/* Signed representation expected, allow -2^63 to 2^63 - 1 only */
if (res > (uint64_t)INT64_MAX + (sign < 0))
caml_failwith(INT64_ERRMSG);
Introduce the macro INTNAT_MIN.
This fixes the warning from MSVC raised on -0x80000000. > warning C4146: unary minus operator applied to unsigned type, result > still unsigned The other replacements are made for consistency and, hopefully, legibility.
c6ce70a
to
7168368
Compare
The code is currently correct since we use wrap-around semantics for signed integers (
-fwrapv
), but:Using constants from
<limits.h>
instead allows for self-documenting code and silences these warnings.Computing the minimum signed integer
From the standard (which I recall doesn't consider wrap-around semantics for signed integers):
The problem being that the result of
1 << CHAR_BIT * sizeof(int) - 1
to compute the minimumint
can't be represented in the result type (it's 2^63, but the maximum is 2^63-1); without wrap-around.Introduce the
INTNAT_MIN
macro to avoid independent re-definitions of this value.Is a change entry needed?
This also prevents warnings raised under Windows by clang-cl and improves code quality with MSVC.
(I might have confused undefined behavior with unspecified behavior, oh well)