-
Notifications
You must be signed in to change notification settings - Fork 38
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
test: Fix meson build and clean up some warnings #595
Conversation
98a2575
to
1f34f09
Compare
Not really familiar with the IPC code or Run End Encoding, but looks like the UBSAN running in the meson build picks up that we are calling memcpy with a NULL dst argument somewhere in the stack: ../src/nanoarrow/common/inline_buffer.h:294:3: runtime error: null pointer passed as argument 1, which is declared to never be null
#0 0x7abd950d3ff9 in ArrowBufferAppendFill ../src/nanoarrow/common/inline_buffer.h:294
#1 0x7abd950d9a2b in ArrowIpcEncoderBuildContiguousBodyBufferCallback ../src/nanoarrow/ipc/encoder.c:511
#2 0x7abd950da36b in ArrowIpcEncoderEncodeRecordBatchImpl ../src/nanoarrow/ipc/encoder.c:546
#3 0x7abd950dac34 in ArrowIpcEncoderEncodeRecordBatch ../src/nanoarrow/ipc/encoder.c:590
#4 0x7abd950db1a5 in ArrowIpcEncoderEncodeSimpleRecordBatch ../src/nanoarrow/ipc/encoder.c:622
#5 0x6531f7d04f6d in ArrowTypeParameterizedTestFixture_NanoarrowIpcNanoarrowArrayRoundtrip_Test::TestBody() ../src/nanoarrow/ipc/decoder_test.cc:815
#6 0x6531f7f1e93b in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2612
#7 0x6531f7ef698a in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2648
#8 0x6531f7e3f9b5 in testing::Test::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2687
#9 0x6531f7e43a3e in testing::TestInfo::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2836
#10 0x6531f7e4877b in testing::TestSuite::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:3015
#11 0x6531f7e93e22 in testing::internal::UnitTestImpl::RunAllTests() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:5920
#12 0x6531f7f24673 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2612
#13 0x6531f7efd21a in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2648
#14 0x6531f7e8676b in testing::UnitTest::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:5484
#15 0x6531f7f756ca in RUN_ALL_TESTS() ../subprojects/googletest-1.14.0/googletest/include/gtest/gtest.h:2317
#16 0x6531f7f75497 in main ../subprojects/googletest-1.14.0/googletest/src/gtest_main.cc:64
#17 0x7abd91c29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#18 0x7abd91c29e3f in __libc_start_main_impl ../csu/libc-start.c:392
#19 0x6531f7ccf924 in _start (/home/willayd/clones/arrow-nanoarrow/builddir/nanoarrow-ipc-decoder-test+0x42f924)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/nanoarrow/common/inline_buffer.h:294:3 in |
if (size_bytes == 0) { | ||
return NANOARROW_OK; | ||
} | ||
|
||
memset(buffer->data + buffer->size_bytes, value, size_bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UB was triggered by the fact that we were making a call of memset(NULL, ..., 0)
- even though size_bytes
was 0 I don't think prevents UB from occurring when the dst
is NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a test for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test suite is what hits this in the first place. Are you asking to split that off into its own dedicated test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just a few lines here to hit that branch explicitly?
arrow-nanoarrow/src/nanoarrow/common/buffer_test.cc
Lines 143 to 147 in cf38896
EXPECT_EQ(ArrowBufferAppendFill(&buffer, 0xff, 10), NANOARROW_OK); | |
EXPECT_EQ(buffer.size_bytes, 10); | |
for (int i = 0; i < 10; i++) { | |
EXPECT_EQ(buffer.data[i], 0xff); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks - makes a lot of sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
if (size_bytes == 0) { | ||
return NANOARROW_OK; | ||
} | ||
|
||
memset(buffer->data + buffer->size_bytes, value, size_bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a test for this case?
af0b045
to
5373a14
Compare
The meson build is broken without this dependency. Also fixed up a few warnings around unused variables and the use of uninitialized struct members