This patch introduces the SubprocessMemory class to llvm-exegesis. This
class contains several utilities that are needed for managing memory to
set up an execution environment for memory annotations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This utility class definitely needs some (unit) testing. I'm hoping to get some tests up soon.
llvm/tools/llvm-exegesis/lib/BenchmarkResult.h | ||
---|---|---|
46 | Please document these. | |
48 | Are these bytes or multiples of Value.getWidth(). Judgin from the code, bytes. But this should be clarified here. | |
llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp | ||
69–71 | Is there a use case for fractions of the value size ? | |
122 | Did you mean to return an error instead ? The function already returns an error, we don;t need to crash here. | |
139 | This is not really needed. | |
llvm/tools/llvm-exegesis/lib/SubprocessMemory.h | ||
34 | Please document these. What do the strings represent ? | |
llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp | ||
32 | Please extract this to a constant: constexpr int kMainProcessPid = 0;, and use it throughout the code for readability. | |
36 | [nit] const std::string& | |
52 | This can be moved to the test fixture (here and below). |
Address reviewer feedback.
llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp | ||
---|---|---|
69–71 | There isn't any especially prominent use case that I had. In the original RFC I mentioned that we had decided on making memory definitions repeat until they filled the size of the definition, getting truncated if the quotient wasn't an integer. There wasn't any strong reason for this, but I figured the flexibility/ease of use improvements might come in handy for someone at some point. |
llvm/tools/llvm-exegesis/lib/BenchmarkResult.h | ||
---|---|---|
50 | SizeBytes then ? | |
52 | Idx ? | |
llvm/tools/llvm-exegesis/lib/SubprocessMemory.h | ||
47–48 | reflow | |
48 | reflow | |
49 | reflow | |
llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp | ||
55 | The typical gtest style for this would be for SM to be a member the test fixture: class SubprocessMemoryTest : public X86TestBase { protected: void setupSuprocessMemory(...) { EXPECT_FALSE(SM.initializeSubprocessMemory(MainProcessPID)); EXPECT_FALSE(SM.addMemoryDefinition(MemoryDefinitions, MainProcessPID)); } SubprocessMemory SM; }; TEST_F(SubprocessMemoryTest, OneDefinition) { // note: SM is available here if you need it, we're in the context of the test fixture. setupSuprocessMemory({{"test1", {APInt(8, 0xff), 4096, 0}}}); checkSharedMemoryDefinition("/0memdef2", 4096, {0xcc}); } | |
84 | If you want to support fractional sizes should add a test for it. |
Address reviewer feedback.
llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp | ||
---|---|---|
84 | This is the test case for fractional sizes. The value is six bytes, so it gets truncated at the end and has to through the fractional size handling path. I can add more test cases though if you feel more coverage is needed. |
llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp | ||
---|---|---|
84 | D'oh. Sorry I misread the 48. |
Rebase, add typedef for pid_t on non-Linux platforms where it isn't included by default.
Address reviewer feedback
llvm/tools/llvm-exegesis/lib/SubprocessMemory.h | ||
---|---|---|
30 | Thanks for the catch! Moved it into the class as it's used in multiple files. |
I'm not sure if it's this commit or one of the other llvm-exegesis commits you pushed, but there's a build failure on AIX now:
/scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/tools/llvm-exegesis/lib/X86/Target.cpp:49:33: error: unused variable 'VAddressSpaceCeiling' [-Werror,-Wunused-const-variable] static constexpr const intptr_t VAddressSpaceCeiling = 0x0000800000000000;
https://lab.llvm.org/buildbot/#/builders/214/builds/8173/steps/5/logs/stdio
Could you take look please?
Looks like since AIX isn't Linux, __linux__ doesn't get defined and none of the new functions that use that variable end up getting compiled. "Should be a simple" #ifdef. I'll get a patch up soon that will hopefully fix the issue.
Looks like 62883d9f9037202628e5454cb935a1d27f6e246d should've fixed it. Let me know if there's something else.
llvm/tools/llvm-exegesis/lib/SubprocessMemory.h | ||
---|---|---|
25 | This broke building on mingw, please use the same definition of pid_t as in PerfHelper.h. |
Sorry for not copying the definition over. Should be fixed in ae4846d8a10c9ca566983fce49e1825a5fac88b6.
llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp | ||
---|---|---|
59 | I think these hard-coded PID:s are creating problems if multiple users are running these tests simultaneously. And it also seems like things aren't cleaned up inside /dev/shm/ when the test is completed, so even if using the correct PID things might get bad in future runs when the PID is reused. Right now I see things like this on our servers: server1> ls -l /dev/shm/auxmem* -rw------- 1 someuser eusers 4096 Jun 28 09:27 /dev/shm/auxmem0 -rw------- 1 someuser eusers 4096 Jun 28 09:27 /dev/shm/auxmem1 -rw------- 1 someuser eusers 4096 Jun 28 09:27 /dev/shm/auxmem2 -rw------- 1 someuser eusers 4096 Jun 28 09:27 /dev/shm/auxmem3 server2> ls -l /dev/shm/auxmem* -rw------- 1 anotheruser eusers 4096 Jun 28 10:27 /dev/shm/auxmem0 -rw------- 1 anotheruser eusers 4096 Jun 28 10:27 /dev/shm/auxmem1 -rw------- 1 anotheruser eusers 4096 Jun 28 10:27 /dev/shm/auxmem2 -rw------- 1 anotheruser eusers 4096 Jun 28 10:27 /dev/shm/auxmem3 This means that if a different user tries to run the llvm-exegesis unittests on those servers the tests will fail: [ RUN ] SubprocessMemoryTest.DefinitionEndTruncation Program aborted due to an unhandled Error: Failed to create shared memory object for auxiliary memory: Permission denied |
I was not considering multi-user setups. I'll look into writing a patch up for that. The process should clean up after its own shared memory. shm_unlink should get called in ~SubprocessMemory, but there's nothing checking that the call actually succeeds. I'll look at doing some more investigation there and probably add an assertion/check within the test.
I was only having a very quick look right now, but afaict there is nothing in that d'tor that handles the auxmem stuff. Those are not added to SharedMemoryNames, so I guess those aren't unlinked.
Any progress on this? It is a bit annoying that the /dev/shm/auxmem* isn't cleaned up automatically. I currently need to manually clean such things up from all our servers where our downstream build bots are running.
Sorry for the late response and lack of appropriate response on my end. 1048b7f8e7c467d1265b3d808ab35ea2e42e79b6 should fix the issue. I'll look into writing a test to ensure this doesn't happen again.
This change also breaks Android builds.
Maybe the linux guard in SubprocessMemoryTest.cpp should instead be:
#if defined(__linux__) && !defined(__ANDROID__)
Without this change we see errors of the following form:
llvm-project/llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp:30:27: error: use of undeclared identifier 'shm_open' int AuxiliaryMemoryFD = shm_open(AuxiliaryMemoryName.c_str(), ^
71cbc62a24df85cf6c6d02d959fccfde231c0698 should fix that. Sorry for the breakage. Please let me know if there are any other breakages caused by this patch stack.
Please document these.