[LNT] Added reading nm and objdump paths from env variables

[kpdev42]

Summary

• Added reading nm and objdump paths from env variables
• Added binaryCacheRoot

[thopre]

Could you split this patch up a bit?

[kpdev42]

Removed Windows support (will be done in a separate patch)

[kpdev42]

In D110839#3033772, @thopre wrote:

Could you split this patch up a bit?

Yes, please review: windows support were removed (will be done as a separate patch)

[kpdev42]

@thopre Should I rework this patch somehow?

[thopre]

In D110839#3042769, @kpdev42 wrote:

@thopre Should I rework this patch somehow?

I think ideally the bullet points in your patch description should each correspond to a different patch. That said, I don't have sufficient expertise to review everything but I might be able to review some of the resulting patches.

lnt/testing/profile/profile.py

34–36

This will set the parameters to None (and thus override the default values of the parameter) when the environment variables are not set. I don't think this is what you meant.

[slydiman]

lnt/testing/profile/profile.py

34–36

import os

def prn(x):
    print(x)

prn(x = os.getenv('MISSING_VAR', 'abc'))

Output: abc
What am I missing?

[thopre]

lnt/testing/profile/profile.py

34–36

Nothing, I missed something: the second parameter of getenv.

[yakush]

lnt/testing/profile/profile.py

34–36

this patch was actually tested in production setup, so it works at least on some platforms.

[kpdev42]

I've divided previously uploaded patch and minimized all changes. Main new functional of cPerf usage refactoring will follow in a next patch

[kpdev42]

@thopre Kindly ask you to review this version of patch

[kpdev42]

Please review this patch

[thopre]

Please add some tests for the environment variables and possibly for the bug fixes depending on what they are. This should also be split into 4 patches, one for each items mentioned. They are all independent, even if you likely made them in one go to get Linux perf work for you. One important reason for splitting is if an issue is found with one of the items the whole patch might need to be reverted. It also allow for easier cherry-pick for people who maintain their own branch of LNT.

Besides the missing tests, the patch LGTM and I'll be happy to approve it.

[slydiman]

In D110839#3074994, @thopre wrote:

Please add some tests for the environment variables and possibly for the bug fixes depending on what they are. This should also be split into 4 patches, one for each items mentioned. They are all independent, even if you likely made them in one go to get Linux perf work for you. One important reason for splitting is if an issue is found with one of the items the whole patch might need to be reverted. It also allow for easier cherry-pick for people who maintain their own branch of LNT.

Besides the missing tests, the patch LGTM and I'll be happy to approve it.

Most changes in cPerf.cpp are related to the standalone mode which is not used in the production. This mode is designed only for debug purpose. It cannot break anything.

cPerf.cpp was already designed to get nm and objdump path as the parameter (with default value).

The environment variables are used to

• redefine or specify the full path to the tools nm and objdump;
• set the root path to binaries.

What are the options? I see only 2 options - the variable is missing (current default behavior) or the variable contains a path to the tool which is used as is. What may be wrong in the code nm = os.getenv('CMAKE_NM', 'nm')?
I don't see any purpose of such tests.

It is necessary to add some executable binaries (tools) and probably some profile files to the LNT repository for requested tests. I think it is redundant and very bad idea. It is impossible to design such tests to be valid on any target or at least on most targets.

I suggest to accept this patch and commit it as is.

BTW, I see that LNT tests did not execute automatically. Something is totally broken here around LNT tests and all LNT tests are useless for now. Who should we ping to fix this?

[thopre]

In D110839#3075336, @slydiman wrote:

In D110839#3074994, @thopre wrote:

Please add some tests for the environment variables and possibly for the bug fixes depending on what they are. This should also be split into 4 patches, one for each items mentioned. They are all independent, even if you likely made them in one go to get Linux perf work for you. One important reason for splitting is if an issue is found with one of the items the whole patch might need to be reverted. It also allow for easier cherry-pick for people who maintain their own branch of LNT.

Besides the missing tests, the patch LGTM and I'll be happy to approve it.

Most changes in cPerf.cpp are related to the standalone mode which is not used in the production. This mode is designed only for debug purpose. It cannot break anything.

Still, the title does not reflect what is happening in the diff which is a clear sign the patch should be split and this is a completely independent change. If the fixes are also for standalone mode I'm happy for the patch to be split in 2 only: standalone+fixes and envvars+bincacheroot.

BTW, I see that LNT tests did not execute automatically. Something is totally broken here around LNT tests and all LNT tests are useless for now. Who should we ping to fix this?

Whoever broke the tests. There's at least 3 failures:

• plistlib change I introduced seems to only work for Python3, I'll make a patch
• future needs to be installed for mypy to work (I'm guessing some recent Python version don't have it in the standard library anymore because it used to work) I've created https://reviews.llvm.org/D112148
• Something in runtest/test_suite.shtest

I think py27 fails because of plistlib change I introduced which only works for Python 3. I need to revisit that. Python3 fails for me because I think I'm using a too new version which seems to have dropped future.

[thopre]

In D110839#3075469, @thopre wrote:

• plistlib change I introduced seems to only work for Python3, I'll make a patch

https://reviews.llvm.org/D112185

• Something in runtest/test_suite.shtest

https://reviews.llvm.org/D112182

[kpdev42]

Simplify diff

[thopre]

LGTM, thanks!

lnt/testing/profile/cPerf.cpp

struct Symbol {
   bool operator<(const Symbol &Other) const { return Start < Other.Start; }
   bool operator==(const Symbol &Other) const {
     return Start == Other.Start && End == Other.End && Name == Other.Name;
   }
 };
 
 class NmOutput : public std::vector<Symbol> {
 public:
-  std::string Nm;
+  std::string Nm, BinaryCacheRoot;
 
-  NmOutput(std::string Nm) : Nm(Nm) {}
+  NmOutput(std::string Nm, std::string BinaryCacheRoot)
+    : Nm(Nm), BinaryCacheRoot(BinaryCacheRoot) {}
 
   void fetchSymbols(Map *M, bool Dynamic) {
     std::string D = "-D";
     if (!Dynamic)
       // Don't fetch the dynamic symbols - instead fetch static ones.
       D = "";
-    std::string Cmd = Nm + " " + D + " -S --defined-only " + std::string(M->Filename) +
+    std::string Cmd = Nm + " " + D + " -S --defined-only " +
+                      BinaryCacheRoot + std::string(M->Filename) +
                       " 2>/dev/null";
     auto Stream = ForkAndExec(Cmd);
 
     char *Line = nullptr;
     size_t LineLen = 0;
     while (true) {
       ssize_t Len = getline(&Line, &LineLen, Stream);
       if (Len == -1)
while (std::getline(ss, token, delim)) {
         output.push_back(token);
     }
     return output.size();
   }
 };
 
 class ObjdumpOutput {
 public:
-  std::string Objdump;
+  std::string Objdump, BinaryCacheRoot;
   FILE *Stream;
   char *ThisText;
   uint64_t ThisAddress;
   uint64_t EndAddress;
   char *Line;
   size_t LineLen;
 
-  ObjdumpOutput(std::string Objdump)
-    : Objdump(Objdump), Stream(nullptr), Line(NULL), LineLen(0) {}
+  ObjdumpOutput(std::string Objdump, std::string BinaryCacheRoot)
+    : Objdump(Objdump), BinaryCacheRoot(BinaryCacheRoot), Stream(nullptr),
+      Line(NULL), LineLen(0) {}
   ~ObjdumpOutput() {
     if (Stream) {
       fclose(Stream);
       wait(NULL);
     }
     if (Line)
       free(Line);
   }
 
   void reset(Map *M, uint64_t Start, uint64_t Stop) {
     ThisAddress = 0;
     if (Stream) {
       fclose(Stream);
       wait(NULL);
     }
 
     char buf1[32], buf2[32];
     sprintf(buf1, "%#llx", Start);
     sprintf(buf2, "%#llx", Stop + 4);
 
     std::string Cmd = Objdump + " -d --no-show-raw-insn --start-address=" +
                       std::string(buf1) + " --stop-address=" +
-                      std::string(buf2) + " " + std::string(M->Filename) +
+                      std::string(buf2) + " " +
+                      BinaryCacheRoot + std::string(M->Filename) +
                       " 2>/dev/null";
     Stream = ForkAndExec(Cmd);
 
     EndAddress = Stop;
   };
 
   std::string getText() { return ThisText; }
 

 
 //===----------------------------------------------------------------------===//
 // PerfReader
 //===----------------------------------------------------------------------===//
 
 class PerfReader {
 public:
   PerfReader(const std::string &Filename, std::string Nm,
-             std::string Objdump);
+             std::string Objdump, std::string BinaryCacheRoot);
   ~PerfReader();
 
   void readHeader();
   void readAttrs();
   void readDataStream();
   unsigned char *readEvent(unsigned char *);
   perf_event_sample parseEvent(unsigned char *Buf, uint64_t Layout);
   void emitLine(uint64_t PC, std::map<const char *, uint64_t> *Counters,
private:
   std::map<const char *, uint64_t> TotalEvents;
   std::map<uint64_t, std::map<const char *, uint64_t>> TotalEventsPerMap;
   std::vector<Map> Maps;
   std::map<uint64_t, std::map<uint64_t, size_t>> CurrentMaps;
 
   PyObject *Functions, *TopLevelCounters;
   std::vector<PyObject*> Lines;
   
-  std::string Nm, Objdump;
+  std::string Nm, Objdump, BinaryCacheRoot;
 };
 
 PerfReader::PerfReader(const std::string &Filename,
-                       std::string Nm, std::string Objdump)
-    : Nm(Nm), Objdump(Objdump) {
+                       std::string Nm, std::string Objdump,
+                       std::string BinaryCacheRoot)
+    : Nm(Nm), Objdump(Objdump), BinaryCacheRoot(BinaryCacheRoot) {
   int fd = open(Filename.c_str(), O_RDONLY);
   assert(fd > 0);
 
   struct stat sb;
   assert(fstat(fd, &sb) == 0);
   BufferLen = (size_t)sb.st_size;
 
   Buffer = (unsigned char *)mmap(NULL, BufferLen, PROT_READ, MAP_SHARED, fd, 0);
if (AllUnderThreshold)
       continue;
 
     // EXEC ELF objects aren't relocated. DYN ones are,
     // so if it's a DYN object adjust by subtracting the
     // map base.
     bool IsSO = IsSharedObject(Maps[MapID].Filename);
     uint64_t Adjust = IsSO ? Maps[MapID].Start : 0;
 
-    NmOutput Syms(Nm);
+    NmOutput Syms(Nm, BinaryCacheRoot);
     Syms.reset(&Maps[MapID]);
 
     // Accumulate the event totals for each symbol
     auto Sym = Syms.begin();
     auto Event = MapEvents.begin();
     std::map<uint64_t, std::map<const char*, uint64_t>> SymToEventTotals;
     while (Event != MapEvents.end() && Sym != Syms.end()) {
       // Skip events until we find one after the start of Sym
void PerfReader::emitMaps() {
   }
 }
 
 void PerfReader::emitSymbol(
     Symbol &Sym, Map &M,
     std::map<uint64_t, std::map<const char *, uint64_t>>::iterator Event,
     std::map<const char *, uint64_t> &SymEvents,
     uint64_t Adjust) {
-  ObjdumpOutput Dump(Objdump);
+  ObjdumpOutput Dump(Objdump, BinaryCacheRoot);
   Dump.reset(&M, Sym.Start, Sym.End);
   Dump.next();
 
   emitFunctionStart(Sym.Name);
   for (uint64_t I = Sym.Start; I < Sym.End; I = Dump.next()) {
     auto PC = Event->first - Adjust;
 
     auto Text = Dump.getText();
PyObject *PerfReader::complete() {
   return Obj;
 }
 
 #ifndef STANDALONE
 static PyObject *cPerf_importPerf(PyObject *self, PyObject *args) {
   const char *Fname;
   const char *Nm = "nm";
   const char *Objdump = "objdump";
-  if (!PyArg_ParseTuple(args, "s|ss", &Fname, &Nm, &Objdump))
+  const char *BinaryCacheRoot = "";
+  if (!PyArg_ParseTuple(args, "s|sss", &Fname, &Nm, &Objdump, &BinaryCacheRoot))
     return NULL;
 
   try {
-    PerfReader P(Fname, Nm, Objdump);
+    PerfReader P(Fname, Nm, Objdump, BinaryCacheRoot);
     P.readHeader();
     P.readAttrs();
     P.readDataStream();
     P.emitTopLevelCounters();
     P.emitMaps();
     return P.complete();
   } catch (std::logic_error &E) {
     PyErr_SetString(PyExc_AssertionError, E.what());

lnt/testing/profile/perf.py

def __init__(self):
         pass
 
     @staticmethod
     def checkFile(fn):
         with open(fn, 'rb') as f:
             return f.read(8) == b'PERFILE2'
 
     @staticmethod
-    def deserialize(f, nm='nm', objdump='objdump', propagateExceptions=False):
+    def deserialize(f, nm='nm', objdump='objdump', propagateExceptions=False,
+                    binaryCacheRoot=''):
         f = f.name
 
         if os.path.getsize(f) == 0:
             # Empty file - exit early.
             return None
 
         try:
             data = cPerf.importPerf(f, nm, objdump)

lnt/testing/profile/profile.py

class Profile(object):
 
     @staticmethod
     def fromFile(f):
         """
         Load a profile from a file.
         """
         for impl in lnt.testing.profile.IMPLEMENTATIONS.values():
             if impl.checkFile(f):
-                ret = impl.deserialize(open(f, 'rb'))
+                ret = None
+                with open(f, 'rb') as fd:
+                    if impl is lnt.testing.profile.perf.LinuxPerfProfile:
+                        ret = impl.deserialize(fd,
+                                  nm = os.getenv('CMAKE_NM', 'nm'),
+                                  objdump = os.getenv('CMAKE_OBJDUMP', 'objdump'),
+                                  binaryCacheRoot = os.getenv('LNT_BINARY_CACHE_ROOT', ''))
+                    else:
+                        ret = impl.deserialize(fd)
                 if ret:
                     return Profile(ret)
                 else:
                     return None
         raise RuntimeError('No profile implementations could read this file!')
 
     @staticmethod
     def fromRendered(s):