Skip to content

Conversation

@rdmark
Copy link
Member

@rdmark rdmark commented Nov 14, 2025

Adds a suite of meson scripts for building the C++ project. Example commands:

meson setup build
meson compile -C build
meson test -C build
meson install -C build

Running the unit tests after being built with meson revealed a number of test isolation / atomicity bugs, leading to pointer dereference and buffer overflow bugs in piscsi proper, for which fixes are included.

@nucleogenic
Copy link
Member

Hey @rdmark

Do we need to update the GHA workflow as well?

It wasn't obvious to me (no experience with Meson Build, fwiw) why some dirs were suffixed with _src?

@rdmark rdmark force-pushed the rdmark-meson branch 2 times, most recently from 68e4baa to f92e6c4 Compare November 14, 2025 22:55
@rdmark
Copy link
Member Author

rdmark commented Nov 14, 2025

@nucleogenic good questions! I am still experimenting here, not all functionality replicated in meson yet. I also want to make sure that it's not slower or more unstable when building natively on old RPis. Once I get cross compiling working I will set up the GitHub CI workflows.

Note for future reference: On the Zeros it's best to force clang++ and single-threaded builds, which requires you to run ninja directly and not through the meson wrapper (AFAICT).

Example commands for building only piscsi and scsictl, skipping all the other binaries to save some time:

CXX=clang++ meson setup build -Dto_build=piscsi,scsictl
ninja -C build -j 1 -v
sudo meson install -C build

Regarding the _src dir suffixes. It's kind of a long story.

The build system is set up so that cpp/ has to be the active build directory, given the relative header include paths and so on. Therefore the meson executable() targets have to be called from cpp/meson.build, with the resulting binaries put under build/cpp/. The problem is that build/cpp/ also has dirs with the same names as the binaries, so linking fails.

It seems the meson project has finally realized that this is a problem and are adding a build_subdir argument to executable() in v1.10.0, the upcoming bleeding edge version. But needless to say, we need to support older meson versions shipped with stable Raspbian releases for instance.

But! As I was typing out this answer to you I had an epiphany. The reason f.e. a build/cpp/piscsi dir gets created is because there is a corresponding cpp/piscsi/meson.build file. If I just move all of the logic into the big build file in cpp/meson.build the dirs won't get created, and the problem goes away. :)

Only drawback is that we lose a bit of modularity.

@rdmark
Copy link
Member Author

rdmark commented Nov 15, 2025

Right now I'm stuck at the unit tests. The suite crashes in PiscsiExecutorTest.ProcessDeviceCmd when built with meson.

@rdmark rdmark force-pushed the rdmark-meson branch 4 times, most recently from 248dd20 to ed9b274 Compare November 15, 2025 20:53
@rdmark rdmark marked this pull request as ready for review November 15, 2025 21:01
@rdmark rdmark requested a review from akuker as a code owner November 15, 2025 21:01
@rdmark rdmark requested a review from nucleogenic November 15, 2025 21:08
@rdmark
Copy link
Member Author

rdmark commented Nov 15, 2025

Actually, I found out now there are more tests that are failing when you randomize the execution order. The gtest library has built-in functionality for randomization: f.e. GTEST_SHUFFLE=1 ./build/cpp/piscsi_test

What I can consistently reproduce are both in StorageDeviceTest

[ RUN      ] StorageDeviceTest.GetSetReservedFiles
../cpp/test/storage_device_test.cpp:137: Failure
Expected equality of these values:
  1
  reserved_files.size()
    Which is: 2

../cpp/test/storage_device_test.cpp:141: Failure
Expected equality of these values:
  1
  reserved_files.size()
    Which is: 2

[  FAILED  ] StorageDeviceTest.GetSetReservedFiles (0 ms)

and, a sigabort when executing:

[----------] 8 tests from StorageDeviceTest
[ RUN      ] StorageDeviceTest.MediumChanged
[       OK ] StorageDeviceTest.MediumChanged (0 ms)
[ RUN      ] StorageDeviceTest.GetIdsForReservedFile
[       OK ] StorageDeviceTest.GetIdsForReservedFile (0 ms)
[ RUN      ] StorageDeviceTest.SetGetFilename
[       OK ] StorageDeviceTest.SetGetFilename (0 ms)
[ RUN      ] StorageDeviceTest.GetSetReservedFiles
[       OK ] StorageDeviceTest.GetSetReservedFiles (0 ms)
[ RUN      ] StorageDeviceTest.UnreserveAll

Program received signal SIGABRT, Aborted.
__pthread_kill_implementation (threadid=281474842215520, signo=signo@entry=6, no_tid=no_tid@entry=0)
    at ./nptl/pthread_kill.c:44
warning: 44	./nptl/pthread_kill.c: No such file or directory
(gdb) bt
#0  __pthread_kill_implementation (threadid=281474842215520, signo=signo@entry=6, no_tid=no_tid@entry=0)
    at ./nptl/pthread_kill.c:44
#1  0x0000fffff77b7e64 [PAC] in __pthread_kill_internal (threadid=<optimized out>, signo=6)
    at ./nptl/pthread_kill.c:89
#2  0x0000fffff7766980 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x0000fffff7751ac4 [PAC] in __GI_abort () at ./stdlib/abort.c:73
#4  0x0000fffff775f9bc [PAC] in __assert_fail_base (fmt=<optimized out>, assertion=<optimized out>, 
    file=<optimized out>, line=86, function=<optimized out>) at ./assert/assert.c:118
#5  0x0000aaaaaad0d378 [PAC] in StorageDevice::ReserveFile (this=0xaaaaab237ec0)
    at ../cpp/devices/storage_device.cpp:86
#6  0x0000aaaaaaf020d8 in StorageDeviceTest_UnreserveAll_Test::TestBody (this=0xaaaaab228b60)
    at ../cpp/test/storage_device_test.cpp:115
#7  0x0000aaaaaaf4bd90 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#8  0x0000aaaaaaf39120 [PAC] in testing::Test::Run() ()
#9  0x0000aaaaaaf392a8 [PAC] in testing::TestInfo::Run() ()
#10 0x0000aaaaaaf39944 [PAC] in testing::TestSuite::Run() ()
#11 0x0000aaaaaaf41de4 [PAC] in testing::internal::UnitTestImpl::RunAllTests() ()
#12 0x0000aaaaaaf423dc [PAC] in testing::UnitTest::Run() ()
#13 0x0000aaaaaaf0470c [PAC] in RUN_ALL_TESTS () at /usr/include/gtest/gtest.h:2334
#14 0x0000aaaaaaf045a8 in main (argc=1) at ../cpp/test/test_setup.cpp:29

@rdmark
Copy link
Member Author

rdmark commented Nov 15, 2025

Pushed a fix that does thorough cleanup between each StorageDeviceTest test. Now I don't get any more test failures when shuffling. Added the GTEST_SHUFFLE flag to the workflow now to catch future test isolation bugs earlier.

I'm still quite suspicious that there are other tests that don't clean up after themselves and that the global state is inconsistent.

@sonarqubecloud
Copy link

@rdmark rdmark marked this pull request as draft November 22, 2025 17:23
Copy link
Member

@nucleogenic nucleogenic left a comment

Choose a reason for hiding this comment

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

Basically just unblocking for merge :)

@rdmark
Copy link
Member Author

rdmark commented Nov 24, 2025

Thanks for the review! For the record, I'm holding off on this to do more testing with actual SCSI hosts. I'm a little paranoid since I don't fully understand some of those failures and why my fixes fixed them. ^^;

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