You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2021/01/09 19:40:05 UTC
[GitHub] [thrift] EnigmaTriton opened a new pull request #2305: Do not rely on compiler and check boundaries
EnigmaTriton opened a new pull request #2305:
URL: https://github.com/apache/thrift/pull/2305
C++ consider the overflow of a signed integer to be an undefined behavior (even with the "Signed Integers are Two’s Complement" update: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r1.html ).
Instead of relying on tests(1) to discover if a compiler does not handle the signed integer overflow as we expect it to, we should add an explicit check before incrementing.
(1) See https://github.com/apache/thrift/blob/c4e899a6d64aa97430ec9f7608d38db2095f6159/lib/cpp/src/thrift/async/TConcurrentClientSyncInfo.cpp#L33
Quick check on https://godbolt.org/z/d7W8ds shows that Clang is able to optimize and just do the +1 so modern compilers should be able to keep the same preformances.
In addition, some company rules might require code to do just that:
https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] EnigmaTriton commented on pull request #2305: Do not rely on compiler and check boundaries
Posted by GitBox <gi...@apache.org>.
EnigmaTriton commented on pull request #2305:
URL: https://github.com/apache/thrift/pull/2305#issuecomment-757359214
Regarding the compilers behavior, some of them do have flags to enforce some behavior or another.
In particular GCC has -fwrapv and -ftrapv (which seem to be valid for Clang as well but I did not check the documentation).
-fwrapv explicitly requires the behavior we expect
-ftrapv explicitly forbids overflow (and if company policy enforce this flag, we're doomed at the 10th or 11th command sent).
Some compilers (MSVC) don't have any flag and just let the processor do what it wants (undefined = I don't care what happens, which is a perfectly acceptable behavior for the compiler).
That's why I think it's better to do the check ourselves.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] EnigmaTriton commented on pull request #2305: Do not rely on compiler and check boundaries
Posted by GitBox <gi...@apache.org>.
EnigmaTriton commented on pull request #2305:
URL: https://github.com/apache/thrift/pull/2305#issuecomment-757359214
Regarding the compilers behavior, some of them do have flags to enforce some behavior or another.
In particular GCC has -fwrapv and -ftrapv (which seem to be valid for Clang as well but I did not check the documentation).
-fwrapv explicitly requires the behavior we expect
-ftrapv explicitly forbids overflow (and if company policy enforce this flag, we're doomed at the 10th or 11th command sent).
Some compilers (MSVC) don't have any flag and just let the processor do what it wants (undefined = I don't care what happens, which is a perfectly acceptable behavior for the compiler).
That's why I think it's better to do the check ourselves.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G closed pull request #2305: THRIFT-5355 Do not rely on compiler and check boundaries
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2305:
URL: https://github.com/apache/thrift/pull/2305
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org