[libc] Fix incorrect unsigned comparison

[yingcong-wu]

There is a problem with such unsigned comparison pattern:

if(unsigned_a - unsigned_b > 0) { /* only NOT go here when unsigned_a==unsigned_b */ }

When unsigned_a < unsigned_b, the result will still be >0 due to underflow.
This patch fixes two of the occurrences I found.
Also remove two redundant if where its condition is guaranteed by outer if.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Wu Yingcong (yingcong-wu)

Changes

There is a problem with such unsigned comparison pattern:

if(unsigned_a - unsigned_b > 0) { /* only NOT go here when unsigned_a==unsigned_b */ }

When unsigned_a < unsigned_b, the result will still be >0 due to underflow.
This patch fixes two of the occurrences I found.

---

Full diff: https://github.com/llvm/llvm-project/pull/135595.diff

1 Files Affected:

• (modified) libc/src/stdio/printf_core/float_dec_converter.h (+2-2)

diff --git a/libc/src/stdio/printf_core/float_dec_converter.h b/libc/src/stdio/printf_core/float_dec_converter.h
index ee5549825a6f2..178f228527f68 100644
--- a/libc/src/stdio/printf_core/float_dec_converter.h
+++ b/libc/src/stdio/printf_core/float_dec_converter.h
@@ -192,7 +192,7 @@ template <WriteMode write_mode> class FloatWriter {
         RET_IF_RESULT_NEGATIVE(writer->write({block_buffer, digits_to_write}));
       }
       RET_IF_RESULT_NEGATIVE(writer->write(DECIMAL_POINT));
-      if (buffered_digits - digits_to_write > 0) {
+      if (buffered_digits > digits_to_write) {
         // Write the digits after the decimal point.
         RET_IF_RESULT_NEGATIVE(
             writer->write({block_buffer + digits_to_write,
@@ -222,7 +222,7 @@ template <WriteMode write_mode> class FloatWriter {
         RET_IF_RESULT_NEGATIVE(writer->write(MAX_BLOCK_DIGIT, digits_to_write));
       }
       RET_IF_RESULT_NEGATIVE(writer->write(DECIMAL_POINT));
-      if ((BLOCK_SIZE * max_block_count) - digits_to_write > 0) {
+      if ((BLOCK_SIZE * max_block_count) > digits_to_write) {
         RET_IF_RESULT_NEGATIVE(writer->write(
             MAX_BLOCK_DIGIT, (BLOCK_SIZE * max_block_count) - digits_to_write));
       }

[yingcong-wu]

The build error looks not related to my change.

/home/runner/work/llvm-project/llvm-project/build/compiler-rt/lib/fuzzer/libcxx_fuzzer_aarch64/build/include/c++/v1/__bit/popcount.h:30:10: error: use of undeclared identifier '__builtin_popcountg'
   30 |   return __builtin_popcountg(__t);

[s-barannikov]

The build error looks not related to my change.

FYI @winner245 @mordante

[winner245]

The outer if condition guarantees that buffered_digits - digits_to_write >= 0. So underflow does not occur here. However, the proposed change enhances clarity and readability.

[winner245]

Similarly, the if (digits_to_write > 0) statement on line 190 is redundant, as the outer if already guarantees the condition to be true.

[yingcong-wu]

I agree that the if in line 190 is redundant and that is quite clear, but I don't think it is straightforward that the outer if also guarantees buffered_digits - digits_to_write >= 0, not to me anyway. I will remove the redundant if in line 190.

[michaelrj-google]

I agree that the underflow shouldn't be possible, but the proposed change improves clarity.

[michaelrj-google]

Thanks for this cleanup, this change seems correct and passes the tests + some local fuzzing.

If you're interested in creating a followup, simplifying the ifs on 186 and 215 would be great. The intended behavior is checking if the number of digits that will be written is between 0 and buffered_digits/BLOCK_SIZE * max_block_count respectively. Making that explicit would make this change more clear.

[michaelrj-google]

I agree that the underflow shouldn't be possible, but the proposed change improves clarity.

[yingcong-wu]

Hi @michaelrj-google, are those CI failures expected? Can we merge this PR in this state?

[michaelrj-google]

The CI failures appear to be from libc++ and unrelated. This patch is fine to merge. Do you have commit access or should I merge this for you?

[yingcong-wu]

Merged, thank you all for reviewing.