Skip to content

[CDRIVER-5859] Enable sign-compare warnings globally, and fix them all #1856

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

Merged

Conversation

vector-of-bool
Copy link
Contributor

Refer: CDRIVER-5859 This changeset adds sign-compare warnings at the top-level, and fixes every occurrence. Most occurrences just swap for the new mlib_cmp macro, but a few more changes have been made to clear up sign-handling.

Summary

  • Lots of use of mlib_cmp, from PR CDRIVER-5859 Integer comparison macro #1845
  • c763a73: A statement-macro that tests whether a given (compound) statement raises SIGABRT. This is a no-op on Windows. This uses fork without exec, so it isn't super fast, usually incurring a few milliseconds for each use.
  • e830cf5: A macro for looping over a half-open range of integers.
  • 93efe4d: A macro for looping over arrays.
  • d01e7f6: A new header: mlib/intencode.h, which adds functions dedicated to decoding/encoding integer values into raw memory.
  • b689d49: This commit actually fixes all instances of sign-comparison warnings in the codebase.

The `mlib_assert_aborts` macro checks that the following statement
terminates the process with SIGABRT. This relies on `fork()`, so it only
works on Unix systems. On Win32 it is a no-op.
`mlib_foreach_irange` and `mlib_foreach_urange` provide
concise looping over an integral range.
This change swaps any `for (...)` loops that trigger
`-Wsign-compare` with `mlib/loop` macros. This also simplifies
redundant code around looping over array elements.
This adds function for reading little-endian i32, u32, i64, and u64 from
pointers to memory. This allows us to decode integers in a single line
instead of doing a declare+memcpy+byteswap that clutters the code and
prevents us from using `const` and correct signedness. Instead, we can
declare and initialize integers of the exact size and sign that we want
in a single line.
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice changes so far.

Some additional suggestions:

  • Also replace or remove use of BSON_MIN and BSON_MAX from <bson/bson-macros.h> to address the following warning(s) due to the internal comparison expression:
src/libbson/src/bson/bson-macros.h:121:30: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
  121 | #define BSON_MIN(a, b) (((a) < (b)) ? (a) : (b))
      |                              ^
src/libbson/src/bson/bson-decimal128.c:303:38: note: in expansion of macro ‘BSON_MIN’
  303 |          const unsigned n_to_write = BSON_MIN (n_trailing_digits, available_bytes);
      |                                      ^~~~~~~~
src/libbson/src/bson/bson-macros.h:121:45: warning: operand of ‘?:’ changes signedness from ‘int’ to ‘unsigned int’ due to unsignedness of other operand [-Wsign-compare]
  121 | #define BSON_MIN(a, b) (((a) < (b)) ? (a) : (b))
      |                                             ^~~
src/libbson/src/bson/bson-decimal128.c:303:38: note: in expansion of macro ‘BSON_MIN’
  303 |          const unsigned n_to_write = BSON_MIN (n_trailing_digits, available_bytes);
      |                                      ^~~~~~~~
  • Also update ASSERT_CMPINT_HELPER in TestSuite.h to use mlib_cmp for its internal equality comparison. Note the non-integer ASSERT_CMP* macros currently implemented in terms of ASSERT_CMPINT_HELPER (for double and void*) will need to be special-cased to avoid using incorrect use with the new integer comparison utilities.

@vector-of-bool
Copy link
Contributor Author

@eramongodb

  • I considered how to modify or replace uses of BSON_MIN/BSON_MAX, but it's difficult in that one cannot statically determine whether the caller intends to have a signed/unsigned result in the case that the signs mismatch. For now, I just let -Wsign-compare find the issues and then update the call sites so that the signs agree with what the caller actually intends to happen.
  • I can update ASSERT_CMP* functions to use this compare, but I could also replace all uses of ASSERT_CMP<int-type> with mlib_check, which will do the correct assertion with a diagnostic. This will touch hundreds of lines, but significantly simplifies integer assertions. Do you (or anyone else) have any opinions on whether to do this swap?

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consistent use of helpers to read/write integers is much appreciated.

Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you (or anyone else) have any opinions on whether to do this swap?

I am fine with deferring those changes to a followup PR.

@eramongodb eramongodb self-requested a review February 19, 2025 15:39
@eramongodb eramongodb self-requested a review February 19, 2025 18:14
@vector-of-bool vector-of-bool requested a review from kevinAlbs March 4, 2025 19:26
@@ -328,7 +324,7 @@ BSON_STATIC_ASSERT2 (max_alloc_grow_fits_min_sizet, (uint64_t) BSON_MAX_SIZE * 2
if (BSON_UNLIKELY ((_length) > BSON_MAX_SIZE - (_list).n_bytes)) { \
goto append_failure; \
} else if ((_length) > 0) { \
*(_list).current++ = (_bson_append_bytes_arg){ \
*(_list).current++ = (_bson_append_bytes_arg) { \
Copy link
Collaborator

@kevinAlbs kevinAlbs Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*(_list).current++ = (_bson_append_bytes_arg) { \
*(_list).current++ = (_bson_append_bytes_arg){ \

To fix clang-format task. Can be run locally with:

./tools/poetry.sh run .evergreen/scripts/clang-format-all.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uggh. My local clang-format is updated past the version installed by Poetry, and it now adds a space after compound-initializers every time I save a file. I need to figure out what .clang-format setting does this, because it is a chore to remember not to commit them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like clang-format 19 now treats compound initializers as C-style casts, so our SpaceAfterCStyleCast option now inserts a space. Earlier Clang versions don't seem to have a way to control the whitespace here, and always deleted it.

@vector-of-bool vector-of-bool merged commit 9152773 into mongodb:master Mar 7, 2025
40 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants