Fix issues identified by the cudf clang-tidy run

[paleolimbot]

Kindly reproducible thanks to @vyasr (see #639 (comment) ). PR demonstrating the issue: rapidsai/cudf#17083

The traceback is similar to one that was "fixed" in our CI by adding NANOARROW_DCHECK()s to help clang-tidy (at least on our local run) see through some of the invariants we know about with respect to what happens on the "reserve" step (or maybe I'm reading this too quickly and clang-tidy has identified an issue).

One traceback:

/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:259:5: warning: Null pointer passed to 1st parameter expecting 'nonnull' [clang-analyzer-core.NonNullParamChecker]
  259 |     std::memset(out.buffer.data, 0, out.buffer.size_bytes);
      |     ^
/__w/cudf/cudf/cpp/tests/interop/from_arrow_host_test.cpp:104:29: note: Calling 'get_nanoarrow_host_tables'
  104 |   auto [tbl, schema, arr] = get_nanoarrow_host_tables(0);
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/cudf/cudf/cpp/tests/interop/from_arrow_host_test.cpp:54:21: note: Calling 'get_nanoarrow_array<bool>'
   54 |   auto boolarray  = get_nanoarrow_array<bool>(test_data.bool_data, test_data.bool_validity);
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:253:3: note: Assuming 'errno_status_92' is 0
  253 |   NANOARROW_THROW_NOT_OK(ArrowArrayInitFromType(tmp.get(), NANOARROW_TYPE_BOOL));
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:32: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:137:36: note: expanded from macro '_NANOARROW_MAKE_NAME'
  137 | #define _NANOARROW_MAKE_NAME(x, y) _NANOARROW_CONCAT(x, y)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:136:33: note: expanded from macro '_NANOARROW_CONCAT'
  136 | #define _NANOARROW_CONCAT(x, y) x##y
      |                                 ^~~~
note: expanded from here
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:71:9: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
   71 |     if (NAME) {                                                       \
      |         ^~~~
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:253:3: note: Taking false branch
  253 |   NANOARROW_THROW_NOT_OK(ArrowArrayInitFromType(tmp.get(), NANOARROW_TYPE_BOOL));
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:3: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:71:5: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
   71 |     if (NAME) {                                                       \
      |     ^
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:253:3: note: Loop condition is false.  Exiting loop
  253 |   NANOARROW_THROW_NOT_OK(ArrowArrayInitFromType(tmp.get(), NANOARROW_TYPE_BOOL));
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:3: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:69:3: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:269:3: note: expanded from macro 'NANOARROW_RETURN_NOT_OK'
  269 |   _NANOARROW_RETURN_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR)
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:142:5: note: expanded from macro '_NANOARROW_RETURN_NOT_OK_IMPL'
  142 |     if (NAME) return NAME;                        \
      |     ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_buffer.h:582:3: note: Loop condition is false.  Exiting loop
  582 |   NANOARROW_RETURN_NOT_OK(
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:269:3: note: expanded from macro 'NANOARROW_RETURN_NOT_OK'
  269 |   _NANOARROW_RETURN_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR)
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:140:3: note: expanded from macro '_NANOARROW_RETURN_NOT_OK_IMPL'
  140 |   do {                                            \
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_buffer.h:586:3: note: Returning without writing to 'bitmap->buffer.data'
  586 |   return NANOARROW_OK;
      |   ^
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:258:28: note: Returning from 'ArrowBitmapResize'
  258 |     NANOARROW_THROW_NOT_OK(ArrowBitmapResize(&out, b.size(), 1));
      |                            ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:82: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |                                                                                  ^~~~
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:70:23: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
   70 |     const int NAME = (EXPR);                                          \
      |                       ^~~~
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:258:5: note: 'errno_status_93' is 0
  258 |     NANOARROW_THROW_NOT_OK(ArrowBitmapResize(&out, b.size(), 1));
      |     ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:32: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:137:36: note: expanded from macro '_NANOARROW_MAKE_NAME'
  137 | #define _NANOARROW_MAKE_NAME(x, y) _NANOARROW_CONCAT(x, y)
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/common/inline_types.h:136:33: note: expanded from macro '_NANOARROW_CONCAT'
  136 | #define _NANOARROW_CONCAT(x, y) x##y
      |                                 ^~~~
note: expanded from here
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:71:9: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
   71 |     if (NAME) {                                                       \
      |         ^~~~
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:258:5: note: Taking false branch
  258 |     NANOARROW_THROW_NOT_OK(ArrowBitmapResize(&out, b.size(), 1));
      |     ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:3: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:71:5: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
   71 |     if (NAME) {                                                       \
      |     ^
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:258:5: note: Loop condition is false.  Exiting loop
  258 |     NANOARROW_THROW_NOT_OK(ArrowBitmapResize(&out, b.size(), 1));
      |     ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:80:3: note: expanded from macro 'NANOARROW_THROW_NOT_OK'
   80 |   _NANOARROW_THROW_NOT_OK_IMPL(_NANOARROW_MAKE_NAME(errno_status_, __COUNTER__), EXPR, \
      |   ^
/__w/cudf/cudf/cpp/build/_deps/nanoarrow-src/src/nanoarrow/nanoarrow.hpp:69:3: note: expanded from macro '_NANOARROW_THROW_NOT_OK_IMPL'
   69 |   do {                                                                \
      |   ^
/__w/cudf/cudf/cpp/tests/interop/nanoarrow_utils.hpp:259:5: note: Null pointer passed to 1st parameter expecting 'nonnull'
  259 |     std::memset(out.buffer.data, 0, out.buffer.size_bytes);
      |     ^           ~~~~~~~~~~~~~~~

[vyasr]

@paleolimbot do you want me to keep rapidsai/cudf#17083 open, or should I go ahead and close it? I can keep the branch alive for as long as you want. I don't mean to rush you, just cleaning house a bit on my end.

[paleolimbot]

Sorry for being slow! I really am going to look at this 😬

Feel free to close the PR but if you don't mind keeping the branch open that would be helpful. (For my own brain: rapidsai/cudf@branch-24.12...vyasr:cudf:testing/nanoarrow_clang_tidy ).

[vyasr]

No rush at all! I was just cleaning up my open PRs list. I'll close the PR but leave the branch.

[paleolimbot]

I was going to try to reproduce this but have discovered that a driver update seems to have borked any ability to do GPU things today 🙁

I see that you're running clang-tidy in release mode, which is probably why none of my updates have helped:

cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja

This doesn't have to be the permanent fix, but my working hypothesis is that adding -DNANOARROW_DEBUG=ON will silence the errors that you're seeing.

In general, I think the issue is that we maintain certain invariants in the C runtime that aren't visible in the headers. We communicate that using DCHECK, but if you don't compile it with the debug checks, there's no way for clang-tidy to know about those invariants.

In particular, the invariant I'm talking about is that if you reserve with additional_bits/bytes > 0, then the buffer's data pointer will never be null. We confirm that internally using debug checks and unit testing. Another invariant that I think that clang-tidy doesn't know about in your internal code is the relationship between size() and empty() (which controls if you do or don't reserve).

[vyasr]

Ah I see. Is NANOARROW_DEBUG intended to be part of the public build interface? If so, we can certainly try that for these jobs. We can't switch to a full debug build because that will take eons for cudf (especially in CI, we're likely to OOM during compilation), but you should be able to just set the NANOARROW_DEBUG and see if that fixes it.

[paleolimbot]

Let me know if that works and/or is an acceptable solution! We could also make the public define something like NANOARROW_CLANG_TIDY in case we want to separate the things that are hints that benefit clang-tidy from other debug things.