Respect pgoff and rely on it for all types of binaries

[kpdev42]

Summary

This fixes incorrect attribution of events to symbols for ET_DYN objects (shared libraries and PIEs), and also simplifies the code.

Generally, the program linker assigns virtual addresses to segments, and virtual addresses of symbols stored in ELF are also in terms of that.
Upon mmapping a segment, its first byte conicides with the start of the memory mapping.
So to get the value of the program counter in terms of the DSO's virtual addresses, one has to add the virtual address of the segment to the raw PC's offset from the start of the mapping.
Like it is done in perf (see linux/tools/perf/util/map.h):

static inline u64 map__map_ip(struct map *map, u64 ip)
{
        return ip - map->start + map->pgoff;
}

(The addresses are translated in thread__find_map() via invoking the map_ip callback that has a comment saying /* ip -> dso rip */.)

OS Laboratory. Huawei Russian Research Institute. Saint-Petersburg

[thopre]

lnt/testing/profile/cPerf.cpp

611

Is PgOff 0 for ET_EXEC objects?

[kpdev42]

Simpleperf generates mmap2 records for all loadable blocks of the same module (not only executable) which may overlap. It may cause an incorrect address calculation because (Start - PgOff) is not a constant for such blocks.

[kpdev42]

lnt/testing/profile/cPerf.cpp

611

Thank you for the question, I've updated a patch, please review

[thopre]

LGTM

[DavidSpickett]

lnt/testing/profile/cPerf.cpp

565

This has broken our armv7 LNT bot:
https://lab.llvm.org/buildbot/#/builders/174/builds/3548

lnt/testing/profile/cPerf.cpp: In member function ‘unsigned char* PerfReader::readEvent(unsigned char*)’:
lnt/testing/profile/cPerf.cpp:565:7: error: break statement not within loop or switch
  565 |       break;
      |       ^~~~~

Perhaps you wanted to return?

[DavidSpickett]

lnt/testing/profile/cPerf.cpp

565

Just saw the fix come in, thanks!

[kpdev42]

lnt/testing/profile/cPerf.cpp

565

Yes, something went wrong during rebase, fixed now. Thank you

lnt/testing/profile/cPerf.cpp

if (Expr)
     return;
   char Str[512];
   sprintf(Str, "%s (%s:%d)", ExprStr, File, Line);
   throw std::logic_error(Str);
 }
 
 // Returns true if the ELF file given by filename
 // is a shared object (DYN).
-bool IsSharedObject(std::string Fname) {
+bool IsSharedObject(char *Fname) {
   // We replicate the first part of an ELF header here
   // so as not to rely on <elf.h>.
   struct PartialElfHeader {
     unsigned char e_ident[16];
     uint16_t e_type;
   };
   const int ET_DYN = 3;
 
-  FILE *stream = fopen(Fname.c_str(), "r");
+  FILE *stream = fopen(Fname, "r");
   if (stream == NULL)
     return false;
 
   PartialElfHeader H;
   auto NumRead = fread(&H, 1, sizeof(H), stream);
   assert(NumRead == sizeof(H));
 
   fclose(stream);

 struct perf_event_mmap {
   struct perf_event_header header;
 
   uint32_t pid, tid;
   uint64_t start, extent, pgoff;
   char filename[1];
 };
 
+#define PROT_EXEC 4
+
 struct perf_event_mmap2 {
   struct perf_event_header header;
 
   uint32_t pid, tid;
   uint64_t start, extent, pgoff;
   uint32_t maj, min;
   uint64_t ino, ino_generation;
   uint32_t prot, flags;
struct perf_sample_id {
   uint64_t id;
 };
 
 //===----------------------------------------------------------------------===//
 // Readers for nm and objdump output
 //===----------------------------------------------------------------------===//
 
 struct Map {
-  uint64_t Start, End;
+  uint64_t Start, End, Adjust;
   const char *Filename;
 };
 
 struct Symbol {
   uint64_t Start;
   uint64_t End;
   std::string Name;
 

 }
 
 unsigned char *PerfReader::readEvent(unsigned char *Buf) {
   perf_event_sample *E = (perf_event_sample *)Buf;
   
   if (E->header.type == PERF_RECORD_MMAP) {
     perf_event_mmap *E = (perf_event_mmap *)Buf;
     auto MapID = Maps.size();
-    Maps.push_back({E->start, E->start + E->extent, E->filename});
+    // 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(E->filename);
+    Maps.push_back({E->start, E->start + E->extent,
+                    IsSO ? E->start - E->pgoff : 0, E->filename});
 
     // FIXME: use EventLayouts.begin()->second!
     perf_sample_id *ID =
         (perf_sample_id *)(Buf + E->header.size - sizeof(perf_sample_id));
     auto &CurrentMap = CurrentMaps[ID->time];
     CurrentMap.insert({E->start, MapID});
   }
   if (E->header.type == PERF_RECORD_MMAP2) {
     perf_event_mmap2 *E = (perf_event_mmap2 *)Buf;
+    if (!(E->prot & PROT_EXEC))
+      break;
     auto MapID = Maps.size();
-    Maps.push_back({E->start, E->start + E->extent, E->filename});
+    // 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(E->filename);
+    Maps.push_back({E->start, E->start + E->extent,
+                    IsSO ? E->start - E->pgoff : 0, E->filename});
 
     // FIXME: use EventLayouts.begin()->second!
     perf_sample_id *ID =
         (perf_sample_id *)(Buf + E->header.size - sizeof(perf_sample_id));
     auto &CurrentMap = CurrentMaps[ID->time];
     CurrentMap.insert({E->start, MapID});
   }
 
if (NewI->first > PC)
       continue;
     MapID = NewI->second;
     break;
   }
   if (MapID == ~0UL)
     return &Buf[E->header.size];
   assert(MapID != ~0UL);
   
   assert(EventIDs.count(EventID));
   Events[MapID][PC][EventIDs[EventID]] += NewE.period;
 
   TotalEvents[EventIDs[EventID]] += NewE.period;
   TotalEventsPerMap[MapID][EventIDs[EventID]] += NewE.period;
 
   return &Buf[E->header.size];
 }
 
for (auto &I : TotalEventsPerMap[MapID]) {
       if ((float)I.second / (float)Total > 0.01f) {
         AllUnderThreshold = false;
         break;
       }
     }
     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;
+    uint64_t Adjust = Maps[MapID].Adjust;
 
     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;