4 ms·

While wrapping parts of OpenSubdiv for Rust [1] I noticed the C++ API was littered with uses of int for indexing/lengths (both are always positive) of geometry

While wrapping parts of OpenSubdiv for Rust [1] I noticed the C++ API was littered with uses of int for indexing/lengths (both are always positive) of geometry buffers. So I opened a ticket with Pixar [2]. Some of the same strange arguments were brought forward (incl. the Google style guide BS). I wrote a longish reply [3] that mentions two comments [4,5] from an ill-guided article promoting use of signed in C++. I closed the ticket in the end because I felt that if people really believe there are good reasons for signed in the cases at hand they haven't grokked the problem sufficiently – trying to convince them otherwise would not be a good use of my time. [1] https://crates.io/crates/opensubdiv-petite https://crates.io/crates/opensubdiv-petite [2] https://github.com/PixarAnimationStudios/OpenSubdiv/issues/1222 https://github.com/PixarAnimationStudios/OpenSubdiv/issues/1... [3] https://github.com/PixarAnimationStudios/OpenSubdiv/issues/1222#issuecomment-814548396 https://github.com/PixarAnimationStudios/OpenSubdiv/issues/1... [4] https://www.learncpp.com/cpp-tutorial/unsigned-integers-and-why-to-avoid-them/#comment-487024 https://www.learncpp.com/cpp-tutorial/unsigned-integers-and-... [5] https://www.learncpp.com/cpp-tutorial/unsigned-integers-and-why-to-avoid-them/comment-page-2/#comment-474548 https://www.learncpp.com/cpp-tutorial/unsigned-integers-and-...

jcelerier 5y ago
Quoting one of these comments: > The argument with substracting 5 from 3 is not an argument against using unsigned. When you use signed you can run into a similar problem when the result underflows. This happens much less frequent and causes UB, at INT_MIN, which is the worst kind of bug. The bug is often not triggered durring testing and you will only notice it when it is too late. The "When you use signed you can run into a similar problem" part is just not true in our world. 3 - 5 happens much more often than anything that may cause signed .*flow. "signed underflow" "bug report" yields 8 results on google FFS
- leni536 5y ago
  No such thing as "underflow" for integers. Signed integers overflow in both directions. Underflow is a floating point concept. Edit: https://en.m.wikipedia.org/wiki/Arithmetic_underflow https://en.m.wikipedia.org/wiki/Arithmetic_underflow
  - cies 5y ago
    So what about an unsigned int going below zero? I think most in the comments here mean that by "underflow", but i could be mistaken (and it should correctly be called "overflow" as well). I agree with the WP article saying integers more correctly "wrap around" (than overflow).
    - leni536 5y ago
      The C standard only calls certain integer arithmetic to "overflow" if it results in undefined behavior. In that sense undefined integer arithmetic never overflows, but they "wrap around", as the behavior is defined there. I can't actually find a normative definition for "overflow" in the C or C++ standards, but there are sections and notes that describe this interpretation: C++: https://eel.is/c++draft/full#basic.fundamental-note-2 https://eel.is/c++draft/full#basic.fundamental-note-2 C: http://web.archive.org/web/20211231011139/https://cigix.me/c17#6.2.5.p9 http://web.archive.org/web/20211231011139/https://cigix.me/c... There can be subtle differences between various definitions of "overflow", and I imagine there could be other subtly different definitions outside C and C++. But "underflow" is exclusively a floating point concept, as far as I know.
von_lohengramm 5y ago
API contracts aside, signed indices do have a major benefit: https://godbolt.org/z/q4eTdPdcd https://godbolt.org/z/q4eTdPdcd
- strictfp 5y ago
  Thanks! If you don't mind explaining, what are we seeing here?
  - masklinn 5y ago
    After the initial setup block, c1 = block[i1]; c2 = block[i2]; if (c1 != c2) return (c1 > c2); i1++; i2++; becomes mov cl, byte ptr [rdx + rcx] cmp byte ptr [rdx + rax], cl jne .LBB0_6 lea eax, [rdi + 2] lea ecx, [rsi + 2] if the indices are unsigned, but mov al, byte ptr [rcx + rdx + 1] cmp byte ptr [rdi + rdx + 1], al jne .LBB1_6 if the indices are signed, that is the compiler just removes the indices and traverses the block directly (starting at the input offsets). That's because in C (and C++) overflow is UB, so the compiler can assume it doesn't happen. Clang is apparently unable to make such a determination or work around it for unsigned, so for every increment of the indices it actually goes and computes the indices to fetch the items from the array. Incidentally, GCC does not care and generates the exact same (rather different) code for both signednesses.
    - sparkie 5y ago
      I see no reason why the same addressing mode could not be used in both versions. This just seems like an optimization that has been made under the assumption that in the addressing mode [reg1 + reg2 + disp] reg2 refers to a signed integer (Which may result in the effective address being < reg1 if reg2 is negative). Obviously you would not want to make the same assumption if reg2 refers to an unsigned integer, because if the most significant bit is set, it would result in an effective address below reg1, which is definitely not what we would expect from adding an unsigned integer. But in this case, 64-bit addressing is being used, and the value of reg2 is a 32-bit integer which has been specifically zero-extended. We can make the assumption that [reg1 + reg2 + disp] can never result in an effective address below reg1 (assuming positive disp). If you take the signed version, and replace the lines movsxd rdi, edi movsxd rcx, esi with movzx rdi, edi movzx rcx, esi I believe you will have something functionally equivalent to the unsigned version. Of course, this same optimization could not apply if we were using uint64, but it could in the case of int64. Either way, buggy code with no bounds checking is not a reliable method of determining what optimizations can be done in production.

jcelerier 5y ago

Quoting one of these comments: > The argument with substracting 5 from 3 is not an argument against using unsigned. When you use signed you can run into a similar problem when the result underflows. This happens much less frequent and causes UB, at INT_MIN, which is the worst kind of bug. The bug is often not triggered durring testing and you will only notice it when it is too late. The "When you use signed you can run into a similar problem" part is just not true in our world. 3 - 5 happens much more often than anything that may cause signed .*flow. "signed underflow" "bug report" yields 8 results on google FFS

leni536 5y ago
No such thing as "underflow" for integers. Signed integers overflow in both directions. Underflow is a floating point concept. Edit: https://en.m.wikipedia.org/wiki/Arithmetic_underflow https://en.m.wikipedia.org/wiki/Arithmetic_underflow
- cies 5y ago
  So what about an unsigned int going below zero? I think most in the comments here mean that by "underflow", but i could be mistaken (and it should correctly be called "overflow" as well). I agree with the WP article saying integers more correctly "wrap around" (than overflow).
  - leni536 5y ago
    The C standard only calls certain integer arithmetic to "overflow" if it results in undefined behavior. In that sense undefined integer arithmetic never overflows, but they "wrap around", as the behavior is defined there. I can't actually find a normative definition for "overflow" in the C or C++ standards, but there are sections and notes that describe this interpretation: C++: https://eel.is/c++draft/full#basic.fundamental-note-2 https://eel.is/c++draft/full#basic.fundamental-note-2 C: http://web.archive.org/web/20211231011139/https://cigix.me/c17#6.2.5.p9 http://web.archive.org/web/20211231011139/https://cigix.me/c... There can be subtle differences between various definitions of "overflow", and I imagine there could be other subtly different definitions outside C and C++. But "underflow" is exclusively a floating point concept, as far as I know.

von_lohengramm 5y ago

API contracts aside, signed indices do have a major benefit: https://godbolt.org/z/q4eTdPdcd https://godbolt.org/z/q4eTdPdcd

strictfp 5y ago
Thanks! If you don't mind explaining, what are we seeing here?
- masklinn 5y ago
  After the initial setup block, c1 = block[i1]; c2 = block[i2]; if (c1 != c2) return (c1 > c2); i1++; i2++; becomes mov cl, byte ptr [rdx + rcx] cmp byte ptr [rdx + rax], cl jne .LBB0_6 lea eax, [rdi + 2] lea ecx, [rsi + 2] if the indices are unsigned, but mov al, byte ptr [rcx + rdx + 1] cmp byte ptr [rdi + rdx + 1], al jne .LBB1_6 if the indices are signed, that is the compiler just removes the indices and traverses the block directly (starting at the input offsets). That's because in C (and C++) overflow is UB, so the compiler can assume it doesn't happen. Clang is apparently unable to make such a determination or work around it for unsigned, so for every increment of the indices it actually goes and computes the indices to fetch the items from the array. Incidentally, GCC does not care and generates the exact same (rather different) code for both signednesses.
  - sparkie 5y ago
    I see no reason why the same addressing mode could not be used in both versions. This just seems like an optimization that has been made under the assumption that in the addressing mode [reg1 + reg2 + disp] reg2 refers to a signed integer (Which may result in the effective address being < reg1 if reg2 is negative). Obviously you would not want to make the same assumption if reg2 refers to an unsigned integer, because if the most significant bit is set, it would result in an effective address below reg1, which is definitely not what we would expect from adding an unsigned integer. But in this case, 64-bit addressing is being used, and the value of reg2 is a 32-bit integer which has been specifically zero-extended. We can make the assumption that [reg1 + reg2 + disp] can never result in an effective address below reg1 (assuming positive disp). If you take the signed version, and replace the lines movsxd rdi, edi movsxd rcx, esi with movzx rdi, edi movzx rcx, esi I believe you will have something functionally equivalent to the unsigned version. Of course, this same optimization could not apply if we were using uint64, but it could in the case of int64. Either way, buggy code with no bounds checking is not a reliable method of determining what optimizations can be done in production.