[llvm-exegesis] Introduce SubprocessMemory Utility Class

[aidengrossman]

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.

[aidengrossman]

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

[aidengrossman]

Rebase

[aidengrossman]

Add unit tests

[aidengrossman]

Format + rebase

[aidengrossman]

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

[courbet]

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

31 ↗

(On Diff #526393)

Please extract this to a constant: constexpr int kMainProcessPid = 0;, and use it throughout the code for readability.

35 ↗

(On Diff #526393)

[nit] const std::string&

51 ↗

(On Diff #526393)

This can be moved to the test fixture (here and below).

[aidengrossman]

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]

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

54 ↗

(On Diff #526919)

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 ↗

(On Diff #526919)

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

[aidengrossman]

Address reviewer feedback.

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

83 ↗

(On Diff #526919)

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]

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

83 ↗

(On Diff #526919)

D'oh. Sorry I misread the 48.

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

Rebase

[aidengrossman]

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

[aidengrossman]

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

[aidengrossman]

Address reviewer feedback.

[courbet]

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 ?

[aidengrossman]

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.

[Jake-Egan]

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?

[aidengrossman]

In D151022#4450888, @Jake-Egan wrote:

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.

[aidengrossman]

Looks like 62883d9f9037202628e5454cb935a1d27f6e246d should've fixed it. Let me know if there's something else.

[mstorsjo]

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.

[aidengrossman]

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

[mstorsjo]

In D151022#4453763, @aidengrossman wrote:

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

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

[bjope]

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

58 ↗

(On Diff #534666)

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

[aidengrossman]

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.

[bjope]

In D151022#4456689, @aidengrossman wrote:

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]

In D151022#4456689, @aidengrossman wrote:

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.

[aidengrossman]

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]

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(),
                            ^

[aidengrossman]

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

[dstuttard]

In D151022#4479345, @aidengrossman wrote:

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

Thanks!

llvm/tools/llvm-exegesis/lib/BenchmarkResult.h

 
 enum class BenchmarkFilter { All, RegOnly, WithMem };
 
+struct MemoryValue {
+  APInt Value;
+  size_t Size;
+  size_t Number;
+};
+
+struct MemoryMapping {
+  intptr_t Address;
+  std::string MemoryValueName;
+};
+
 struct BenchmarkKey {
   // The LLVM opcode name.
   std::vector<MCInst> Instructions;

llvm/tools/llvm-exegesis/lib/CMakeLists.txt

   SnippetFile.cpp
   SnippetGenerator.cpp
   SnippetRepetitor.cpp
+  SubprocessMemory.cpp
   Target.cpp
   UopsBenchmarkRunner.cpp
 

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

+//===-- SubprocessMemory.h --------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Defines a class that automatically handles auxiliary memory and the
+/// underlying shared memory backings for memory definitions
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TOOLS_LLVM_EXEGESIS_SUBPROCESSMEMORY_H
+#define LLVM_TOOLS_LLVM_EXEGESIS_SUBPROCESSMEMORY_H
+
+#include "BenchmarkResult.h"
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+namespace llvm {
+namespace exegesis {
+
+static constexpr const size_t AuxiliaryMemoryOffset = 1;
+static constexpr const size_t AuxiliaryMemorySize = 4096;
+
+class SubprocessMemory {
+public:
+  Error initializeSubprocessMemory(pid_t ProcessPID);
+  Error addMemoryDefinition(
+      std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+      pid_t ProcessPID);
+  static Expected<int> setupAuxiliaryMemoryInSubprocess(
+      std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+      pid_t ParentPID, int CounterFileDescriptor);
+  ~SubprocessMemory();
+
+private:
+  std::vector<std::string> SharedMemoryNames;
+};
+
+} // namespace exegesis
+} //namespace llvm
+
+#endif

llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp

+//===-- SubprocessMemory.cpp ------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "SubprocessMemory.h"
+#include "Error.h"
+
+#ifdef __linux__
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#endif
+
+// TODO(boomanaiden154): Add unit tests for this file
+
+namespace llvm {
+namespace exegesis {
+
+#ifdef __linux__
+Error SubprocessMemory::initializeSubprocessMemory(pid_t ProcessPID) {
+  // Add the PID to the shared memory name so that if we're running multiple
+  // processes at the same time, they won't interfere with each other.
+  // This comes up particularly often when running the exegesis tests with
+  // llvm-lit
+  std::string AuxiliaryMemoryName = "/auxmem" + std::to_string(ProcessPID);
+  int AuxiliaryMemoryFD = shm_open(AuxiliaryMemoryName.c_str(),
+                                   O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
+  if (AuxiliaryMemoryFD == -1)
+    return make_error<Failure>(
+        "Failed to create shared memory object for auxiliary memory: " +
+        Twine(strerror(errno)));
+  if (ftruncate(AuxiliaryMemoryFD, AuxiliaryMemorySize) != 0) {
+    return make_error<Failure>("Truncating the auxiliary memory failed: " +
+                               Twine(strerror(errno)));
+  }
+  return Error::success();
+}
+
+Error SubprocessMemory::addMemoryDefinition(
+    std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+    pid_t ProcessPID) {
+  SharedMemoryNames.reserve(MemoryDefinitions.size());
+  for (auto &[Name, MemVal] : MemoryDefinitions) {
+    std::string SharedMemoryName = "/" + std::to_string(ProcessPID) + "memdef" +
+                                   std::to_string(MemVal.Number);
+    SharedMemoryNames.push_back(SharedMemoryName);
+    int SharedMemoryFD =
+        shm_open(SharedMemoryName.c_str(), O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
+    if (ftruncate(SharedMemoryFD, MemVal.Size) != 0) {
+      return make_error<Failure>("Truncating a memory definiton failed: " +
+                                 Twine(strerror(errno)));
+    }
+
+    char *SharedMemoryMapping =
+        (char *)mmap(NULL, MemVal.Size, PROT_READ | PROT_WRITE, MAP_SHARED,
+                     SharedMemoryFD, 0);
+    // fill the buffer with the specified value
+    size_t CurrentByte = 0;
+    const size_t ValueWidthBytes = MemVal.Value.getBitWidth() / 8;
+    while (CurrentByte < MemVal.Size - ValueWidthBytes) {
+      memcpy(SharedMemoryMapping + CurrentByte, MemVal.Value.getRawData(),
+             ValueWidthBytes);
+      CurrentByte += ValueWidthBytes;
+    }
+    // fill the last section
+    memcpy(SharedMemoryMapping + CurrentByte, MemVal.Value.getRawData(),
+           MemVal.Size - CurrentByte);
+    if (munmap(SharedMemoryMapping, MemVal.Size) != 0) {
+      return make_error<Failure>(
+          "Unmappng a memory definition in the parent failed: " +
+          Twine(strerror(errno)));
+    }
+  }
+  return Error::success();
+}
+
+Expected<int> SubprocessMemory::setupAuxiliaryMemoryInSubprocess(
+    std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+    pid_t ParentPID, int CounterFileDescriptor) {
+  std::string AuxiliaryMemoryName = "/auxmem" + std::to_string(ParentPID);
+  int AuxiliaryMemoryFileDescriptor =
+      shm_open(AuxiliaryMemoryName.c_str(), O_RDWR, S_IRUSR | S_IWUSR);
+  if (AuxiliaryMemoryFileDescriptor == -1)
+    return make_error<Failure>(
+        "Getting file descriptor for auxiliary memory failed");
+  // set up memory value file descriptors
+  int *AuxiliaryMemoryMapping =
+      (int *)mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_SHARED,
+                  AuxiliaryMemoryFileDescriptor, 0);
+  if ((intptr_t)AuxiliaryMemoryMapping == -1)
+    return make_error<Failure>("Mapping auxiliary memory failed");
+  AuxiliaryMemoryMapping[0] = CounterFileDescriptor;
+  for (auto &[Name, MemVal] : MemoryDefinitions) {
+    std::string MemoryValueName = "/" + std::to_string(ParentPID) + "memdef" +
+                                  std::to_string(MemVal.Number);
+    AuxiliaryMemoryMapping[AuxiliaryMemoryOffset + MemVal.Number] =
+        shm_open(MemoryValueName.c_str(), O_RDWR, S_IRUSR | S_IWUSR);
+    if (AuxiliaryMemoryMapping[AuxiliaryMemoryOffset + MemVal.Number] == -1)
+      return make_error<Failure>("Mapping shared memory failed");
+  }
+  if (munmap(AuxiliaryMemoryMapping, 4096) == -1)
+    return make_error<Failure>("Unmapping auxiliary memory failed");
+  return AuxiliaryMemoryFileDescriptor;
+}
+
+SubprocessMemory::~SubprocessMemory() {
+  for (std::string SharedMemoryName : SharedMemoryNames) {
+    if (shm_unlink(SharedMemoryName.c_str()) != 0) {
+      errs() << "Failed to unlink shared memory section: " << strerror(errno)
+             << "\n";
+    }
+  }
+}
+
+#else
+Error SubprocessMemory::initializeSubprocessMemory(pid_t ProcessPID) {
+  report_fatal_error("initializeSubprocessMemory is only supported on Linux");
+}
+
+Error SubprocessMemory::addMemoryDefinition(
+    std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+    pid_t ProcessPID) {
+  report_fatal_error("addMemoryDefinitions is only supported on Linux");
+}
+
+Expected<int> SubprocessMemory::setupAuxiliaryMemoryInSubprocess(
+    std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+    pid_t ParentPID, int CounterFileDescriptor) {
+  report_fatal_error(
+      "setupAuxiliaryMemoryInSubprocess is only supported on Linux");
+}
+#endif
+
+} // namespace exegesis
+} // namespace llvm