This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] Introduce SubprocessMemory Utility Class
ClosedPublic

Authored by aidengrossman on May 20 2023, 3:39 AM.

Details

Summary

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.

Diff Detail

Event Timeline

aidengrossman created this revision.May 20 2023, 3:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 3:39 AM
aidengrossman requested review of this revision.May 20 2023, 3:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 3:39 AM

This utility class definitely needs some (unit) testing. I'm hoping to get some tests up soon.

Add unit tests

Format + rebase

Rebase, minor formatting changes, fix build on non-Linux platforms.

courbet added inline comments.May 30 2023, 2:45 AM
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).

aidengrossman marked 8 inline comments as done.

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.

courbet added inline comments.May 30 2023, 11:50 PM
llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
50

SizeBytes then ?

52

Idx ?

llvm/tools/llvm-exegesis/lib/SubprocessMemory.h
46–47

reflow

47

reflow

48

reflow

llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp
54

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});
}
83

If you want to support fractional sizes should add a test for it.

aidengrossman marked 4 inline comments as done.

Address reviewer feedback.

llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp
83

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.

courbet added inline comments.May 31 2023, 12:01 AM
llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp
83

D'oh. Sorry I misread the 48.

aidengrossman marked 4 inline comments as done.

Address reviewer feedback.

Rebase, add typedef for pid_t on non-Linux platforms where it isn't included by default.

@courbet This should be ready for another look when you have a chance.

Address reviewer feedback.

courbet accepted this revision.Jun 16 2023, 12:44 AM
courbet added inline comments.
llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp
75

typo

llvm/tools/llvm-exegesis/lib/SubprocessMemory.h
31

we're in a header, the static here is weird. Did you mean to put this in a class definition, or in the cpp file ?

This revision is now accepted and ready to land.Jun 16 2023, 12:44 AM
aidengrossman marked 2 inline comments as done.

Address reviewer feedback

llvm/tools/llvm-exegesis/lib/SubprocessMemory.h
31

Thanks for the catch! Moved it into the class as it's used in multiple files.

This revision was landed with ongoing or failed builds.Jun 21 2023, 1:15 AM
This revision was automatically updated to reflect the committed changes.
aidengrossman reopened this revision.Jun 25 2023, 4:10 PM
This revision is now accepted and ready to land.Jun 25 2023, 4:10 PM
aidengrossman reopened this revision.Jun 25 2023, 8:17 PM
This revision is now accepted and ready to land.Jun 25 2023, 8:17 PM

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?

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.

mstorsjo added inline comments.
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.

Sorry for not copying the definition over. Should be fixed in ae4846d8a10c9ca566983fce49e1825a5fac88b6.

Thanks, that does seem to have fixed my builds again!

bjope added a subscriber: bjope.Jun 28 2023, 3:55 AM
bjope added inline comments.
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
bjope added a subscriber: uabelho.Jun 28 2023, 3:56 AM

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 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.

bjope added a comment.Jun 30 2023, 8:22 AM

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.

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.

dstuttard added a subscriber: dstuttard.EditedJul 6 2023, 2:32 AM

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.

71cbc62a24df85cf6c6d02d959fccfde231c0698 should fix that. Sorry for the breakage. Please let me know if there are any other breakages caused by this patch stack.

Thanks!