Skip to content
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

Fix issues identified by the cudf clang-tidy run #662

Open
paleolimbot opened this issue Oct 15, 2024 · 7 comments
Open

Fix issues identified by the cudf clang-tidy run #662

paleolimbot opened this issue Oct 15, 2024 · 7 comments

Comments

@paleolimbot
Copy link
Member

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
Copy link
Contributor

vyasr commented Oct 28, 2024

@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
Copy link
Member Author

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
Copy link
Contributor

vyasr commented Oct 30, 2024

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

@paleolimbot
Copy link
Member Author

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
Copy link
Contributor

vyasr commented Nov 2, 2024

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
Copy link
Member Author

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.

@vyasr
Copy link
Contributor

vyasr commented Dec 2, 2024

@paleolimbot Sorry, lost track of this for a bit! Unfortunately it looks like setting NANOARROW_DEBUG doesn't seem to help. Am I missing something in rapidsai/cudf#17476?

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

No branches or pull requests

2 participants