Let GCC use the add/mul overflow intrinsics - #4929
Conversation
| #if (__has_builtin(__builtin_add_overflow) || \ | ||
| (defined(__GNUC__) && (__GNUC__ >= 5))) | ||
|
|
||
| # if (ULONG_MAX == ULLONG_MAX) && defined(_WIN64) |
There was a problem hiding this comment.
I don't think that I understand this - if ULONG_MAX == ULLONG_MAX, wouldn't uaddl be equivalent to uaddll?
There was a problem hiding this comment.
that's the intention. this means the block definitely needs some comments to explain what's going on and which cases it's addressing, will do that later today.
| __builtin_uaddll_overflow(one, two, out) | ||
| # define git__multiply_sizet_overflow(out, one, two) \ | ||
| __builtin_umulll_overflow(one, two, out) | ||
| # elif (ULONG_MAX == ULLONG_MAX) |
There was a problem hiding this comment.
I don't think that I understand this - if ULONG_MAX == ULLONG_MAX, wouldn't uaddl be equivalent to uaddll by definition?
There was a problem hiding this comment.
not by definition, since unsigned long and unsigned long long are still different types to the compiler, even though they have the same size. so passing in a size_t to the wrong method results in a warning, which goes against what i'm trying to achieve.
There was a problem hiding this comment.
But you're not doing anything here to validate that size_t is actually either of those types. Again, not likely but theoretically possible.
There was a problem hiding this comment.
In other words, couldn't we end up in a (yes, I admit, almost certainly theoretical) situation where unsigned long and unsigned long long are both 64 bits but size_t is something shorter?
Thanks for explaining the rationale behind the uaddll vs uaddl, that makes more sense now. Some comments would indeed be helpful because this is super not obvious to me.
There was a problem hiding this comment.
err... i totally missed that case. will add that explicit check together with the comments.
There was a problem hiding this comment.
I think it's exceeeeeeeedingly unlikely to ever occur in practice. If you wanted to dust off your Cray and port libgit2 to it then it would still not have a modern compiler or 64 bit unsigned longs. But I like being a little extra paranoid when it comes to this little bit of security.
Thanks for accommodating my paranoia.
| __builtin_uaddl_overflow(one, two, out) | ||
| # define git__multiply_sizet_overflow(out, one, two) \ | ||
| __builtin_umull_overflow(one, two, out) | ||
| # else |
There was a problem hiding this comment.
We're dropping the SIZE_MAX == UINT_MAX test. I'm not sure that there's a guarantee that they're identical in the C standard. size_t is:
the unsigned integer type of the result of the sizeof operator
and is a minimum of a 16-bits. But it doesn't necessarily hold that size_t is equivalent to unsigned int that I can see in the spec. I realize that we're unlikely to encounter a system that has 16 bit size_ts and 32 bit unsigned ints, but without a strict assurance of that from the spec, since this macro is explicitly for security sensitive code, I would like to keep the explicit test.
There was a problem hiding this comment.
what happens in the final case is that we call the __builtin_add_overflow intrinsic (as opposed to __builtin_uadd_overflow), which takes any type you give it. i just added the two other cases explicitly to make clang prefer the other two and avoid a dependency to compiler-rt.
There was a problem hiding this comment.
Ah, thanks - I had indeed read this as __builtin_uadd_overflow.
This change tweaks the macros for git__{add,multiply}_sizet_overflow so
that GCC can use them.
It also stops using the uadd,umul versions since the add,mul can handle
way more cases.
25c9ca2 to
2848923
Compare
|
All right, a nice, healthy comment explaining the whole thing has been added. |
|
/rebuild |
|
Okay, @ethomson, I started to rebuild this pull request as build #1194. |
|
/rebuild |
|
Okay, @ethomson, I started to rebuild this pull request as build #1225. |
This change tweaks the macros for git__{add,multiply}_sizet_overflow so
that GCC can use them.
It also stops using the uadd,umul versions since the add,mul can handle
way more cases.