[llvm-profgen] Support perf script without parsing MMap events

[wlei]

Summary

This change supports to run without parsing MMap binary loading events instead it always assumes binary is loaded at the preferred address. This is used when we have assured no binary load address changes or we have pre-processed the addresses resolution. Warn if there's interior mmap event but without leading mmap events.

[lxfind]

Having both "--show-mmap-events --ignore-mmap-events" seems a bit counter-intuitive.
If we already preprocessed the mmap events, could we also assume that we have the right pids and hence don't need --show-mmap-events?

[hoy]

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

[wlei]

In D107097#2914447, @lxfind wrote:

Having both "--show-mmap-events --ignore-mmap-events" seems a bit counter-intuitive.
If we already preprocessed the mmap events, could we also assume that we have the right pids and hence don't need --show-mmap-events?

Thanks for the feedback, the --show-mmap-events here is different from the linux perf script, here is to show the mmap events paring results, only for debugging use I think.

And in this test, see the directive: ; CHECK-IGNORE-NOT: Mmap: Binary /home/a.out loaded at 0x400000 which means --show-mmap-events won't work with --ignore-mmap-events.

I can add a warning here if it's confusing.

[wlei]

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

[hoy]

In D107097#2914528, @wlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

Can this be fixed? Without mmap events we can just use the preferred address as the load address.

It sounds a bit unclear to me by giving ignore-mmap-events when the map events are there. That means we basically do not trust those events.

[wlei]

In D107097#2914552, @hoy wrote:

In D107097#2914528, @wlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

Can this be fixed? Without mmap events we can just use the preferred address as the load address.

It sounds a bit unclear to me by giving ignore-mmap-events when the map events are there. That means we basically do not trust those events.

Understood! My concern is if the scripts has mmap events and only the leading sample missed the load address, they still can be loaded at a different base address which is not the preferred address. it's wrong base address in that case,

How about change it to always-trust-mmap-events, with this, the leading no-matched samples will be dropped. otherwise, it will use preferred address as load address.

[hoy]

In D107097#2914616, @wlei wrote:

In D107097#2914552, @hoy wrote:

In D107097#2914528, @wlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

Can this be fixed? Without mmap events we can just use the preferred address as the load address.

It sounds a bit unclear to me by giving ignore-mmap-events when the map events are there. That means we basically do not trust those events.

Understood! My concern is if the scripts has mmap events and only the leading sample missed the load address, they still can be loaded at a different base address which is not the preferred address. it's wrong base address in that case,

If that's a real case, we'll need to fix that. For now I think we want to cover to cases: 1. with leading mmaps 2. without leading mmaps (assuming the preferred address is the real load address). Have you seen a third case? I'm wondering if the input is invalid.

[wlei]

In D107097#2914634, @hoy wrote:

In D107097#2914616, @wlei wrote:

In D107097#2914552, @hoy wrote:

In D107097#2914528, @wlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

Can this be fixed? Without mmap events we can just use the preferred address as the load address.

It sounds a bit unclear to me by giving ignore-mmap-events when the map events are there. That means we basically do not trust those events.

Understood! My concern is if the scripts has mmap events and only the leading sample missed the load address, they still can be loaded at a different base address which is not the preferred address. it's wrong base address in that case,

If that's a real case, we'll need to fix that. For now I think we want to cover to cases: 1. with leading mmaps 2. without leading mmaps (assuming the preferred address is the real load address). Have you seen a third case? I'm wondering if the input is invalid.

Oh, I see, you mean if turn on --show-mmap-events, it will always have the leading mmap event, otherwise it's the invalid input? I indeed haven't seen the third case before. If so, that makes sense to use preferred address when without leading mmaps.

[hoy]

In D107097#2914645, @wlei wrote:

In D107097#2914634, @hoy wrote:

In D107097#2914616, @wlei wrote:

In D107097#2914552, @hoy wrote:

In D107097#2914528, @wlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

Can this be fixed? Without mmap events we can just use the preferred address as the load address.

It sounds a bit unclear to me by giving ignore-mmap-events when the map events are there. That means we basically do not trust those events.

Understood! My concern is if the scripts has mmap events and only the leading sample missed the load address, they still can be loaded at a different base address which is not the preferred address. it's wrong base address in that case,

If that's a real case, we'll need to fix that. For now I think we want to cover to cases: 1. with leading mmaps 2. without leading mmaps (assuming the preferred address is the real load address). Have you seen a third case? I'm wondering if the input is invalid.

Oh, I see, you mean if turn on --show-mmap-events, it will always have the leading mmap event, otherwise it's the invalid input? I indeed haven't seen the third case before. If so, that makes sense to use preferred address when without leading mmaps.

I think --show-mmap-events should always give leading mmap events or no mmap events at all. We should warn or error out when only interior mmap events are seen.

[wlei]

In D107097#2914659, @hoy wrote:

In D107097#2914645, @wlei wrote:

In D107097#2914634, @hoy wrote:

In D107097#2914616, @wlei wrote:

In D107097#2914552, @hoy wrote:

In D107097#2914528, @wlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

That might have issues with the leading samples before we meet a MMAP, they won't know which binary it belongs to. Currently If not specify ignore-mmap-events, we will always check the Addr2Binary table which is empty without mmap events parsed.

Can this be fixed? Without mmap events we can just use the preferred address as the load address.

It sounds a bit unclear to me by giving ignore-mmap-events when the map events are there. That means we basically do not trust those events.

Understood! My concern is if the scripts has mmap events and only the leading sample missed the load address, they still can be loaded at a different base address which is not the preferred address. it's wrong base address in that case,

If that's a real case, we'll need to fix that. For now I think we want to cover to cases: 1. with leading mmaps 2. without leading mmaps (assuming the preferred address is the real load address). Have you seen a third case? I'm wondering if the input is invalid.

Oh, I see, you mean if turn on --show-mmap-events, it will always have the leading mmap event, otherwise it's the invalid input? I indeed haven't seen the third case before. If so, that makes sense to use preferred address when without leading mmaps.

I think --show-mmap-events should always give leading mmap events or no mmap events at all. We should warn or error out when only interior mmap events are seen.

That makes sense, let me change it, thanks for the feedback!

[wenlei]

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

+1. I was thinking about the same..

We can start with assuming all binaries are loaded at preferred based, then on mmap event we update the base for binary, on other event, we process them using the current base (would be preferred base if no relevant mmap is seen before), and warn if there's no mmap before that event?

The above would be the default behavior without any switch. Is that what we're trying to do? If we want we could add a switch to always require mmap and error out if we don't find mmap for a binary.

[hoy]

In D107097#2914876, @wenlei wrote:

In D107097#2914487, @hoy wrote:

Can the mmap events be treated as optional, so when they present we parse them otherwise just move forward to other events?

+1. I was thinking about the same..

We can start with assuming all binaries are loaded at preferred based,

This is my two cents.

then on mmap event we update the base for binary,

I was thinking about warning or erroring for this. Erroring is probably two strong. An interior mmap event without a corresponding leading event likely means the profiling system doesn't work consistently, i.e., it sometimes grabs mmap event but sometimes not.

[wlei]

addressing reviewers' feedback, default to use preferred adddress as loading address without mmap events, give warning if no leading mmap events

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

320–327

nit: prefered -> preferred

751

Instead of a prescan for leading mmap events based on limited number of lines, can we compute a flag, say EventSeenFirst, the value of which can be sample events or mmap events, during parseAndAggregateTrace and check it in parseMMap2Event?

[wlei]

addressing Hongtao's feedback

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

751

Good idea! Fixed.

[wlei]

lint

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

673

The term "Interior mmap event" could be confusing..

The warning I was thinking is for when LBR/stack sample is seen before a relevant mmap is seen, in which case we warn about assuming preferred based address due to not seeing corresponding mmap. It doesn't matter whether we found other mmap event later or not.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

673

That sounds reasonable. In that case, the warning should be issued in parseSample for the first sample that does not have a leading mmap event.

[wlei]

addressing Wenlei and Hongtao's feedback

[hoy]

LGTM, thanks.

llvm/tools/llvm-profgen/PerfReader.h

657 ↗

(On Diff #363232)

Nit: add a comment to indicate what the flag is used for.

[wlei]

add comments

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

673

Sorry if I wasn't clear. But note the "relevant mmap" in my previous comment - the way EventSeen is set here doesn't guarantee that the mmap seen is relevant for the binary we're interested in. Say if we want to generate profile for foo.so, we'd still want to warn if mmap for foo.so isn't seen (we may have seen mmap for other binaries) before we have a sample that is concluded to belong to foo.so's address space based on the preferred base.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

673

That's more precise. EventSeen should be set after the mmap event for the current binary is analyzed.

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

673

As for the implementation, it's more like a MMapSeen member of ProfiledBinary.

[wlei]

change to check the field in ProfiledBinary

[wenlei]

Thanks for making the changes. It's almost ready, except two more tweaks.

llvm/tools/llvm-profgen/PerfReader.cpp

664–671

Would it be cleaner if we move this to be after line 646 - at that point, we've located the binary using top addresses from stack sample, and we're ready to warn even if we don't get any lbr extracted due to leading outgoing branches.

llvm/tools/llvm-profgen/ProfiledBinary.h

145

Do we need a three-way Optional value here? Would it work if we just always initialize it to false, and set it to true when mmap is seen for the binary?

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

664–671

Good point!

llvm/tools/llvm-profgen/ProfiledBinary.h

145

That's to avoid redundant warnings if there're many sample without matching any leading mmap. see in line 663 isLoadedByMMap.reset(); will mark it as "reported".

[wlei]

addressing Wenlei's feedback

[wenlei]

lgtm with nit on flags. thanks!

llvm/tools/llvm-profgen/ProfiledBinary.h

145

Ok, I see, thanks for clarification. In that case, after resetting, the meaning of the flag no longer abides to its definition described in the comment above? Mmap is still not seen, but we reset it to unknown just to avoid duplicated warning.. May consider a separate flag (like MissingMMapWarned) is cleaner.

Another nit: we use upper case initial for member field - IsLoadedByMMap

[wlei]

addressing Wenlei's feedback

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

325

With IsLoadedByMMap initialized to false, this is no longer needed? (We don't setMissingMMapWarned here either)

[wlei]

remove a redundant assignment of IsLoadedByMMap

llvm/tools/llvm-profgen/PerfReader.cpp

325

Good catch, fixed!

llvm/test/tools/llvm-profgen/Inputs/recursion-compression-pseudoprobe-nommap.perfscript

+            2017db
+            2017ba
+            2017e5
+            2017ba
+            2017e5
+            2017d9
+            2017ba
+            2017b0
+            2017b0
+            2017b0
+            2017b0
+            2017b0
+            2017b0
+            2017b0
+            2017b0
+            2017e5
+            2017d9
+            201847
+      7fcb072a67c3
+  5541f689495641d7
+ 0x2017cd/0x2017db/P/-/-/0  0x2017b5/0x2017c0/P/-/-/0  0x2017a7/0x2017b2/P/-/-/0  0x2017e0/0x2017a0/P/-/-/0  0x2017cd/0x2017db/P/-/-/0  0x2017b5/0x2017c0/P/-/-/0  0x2017a7/0x2017b2/P/-/-/0  0x2017e0/0x2017a0/P/-/-/0  0x2017cd/0x2017db/P/-/-/0  0x2017d4/0x2017c0/P/-/-/0  0x2017b5/0x2017c0/P/-/-/0  0x2017a7/0x2017b2/P/-/-/0  0x2017ab/0x2017a0/P/-/-/0  0x2017ab/0x2017a0/P/-/-/0  0x2017ab/0x2017a0/P/-/-/0  0x2017ab/0x2017a0/P/-/-/0

llvm/test/tools/llvm-profgen/recursion-compression-pseudoprobe.test

 ; Firstly test uncompression(--compress-recursion=0)
 ; RUN: llvm-profgen --format=text --perfscript=%S/Inputs/recursion-compression-pseudoprobe.perfscript --binary=%S/Inputs/recursion-compression-pseudoprobe.perfbin --output=%t --compress-recursion=0 --profile-summary-cold-count=0
 ; RUN: FileCheck %s --input-file %t -check-prefix=CHECK-UNCOMPRESS
 ; RUN: llvm-profgen --format=text --perfscript=%S/Inputs/recursion-compression-pseudoprobe.perfscript --binary=%S/Inputs/recursion-compression-pseudoprobe.perfbin --output=%t --show-unwinder-output --profile-summary-cold-count=0 | FileCheck %s --check-prefix=CHECK-UNWINDER
 ; RUN: FileCheck %s --input-file %t
+; RUN: llvm-profgen --format=text --perfscript=%S/Inputs/recursion-compression-pseudoprobe-nommap.perfscript --binary=%S/Inputs/recursion-compression-pseudoprobe.perfbin --output=%t --show-unwinder-output --profile-summary-cold-count=0 | FileCheck %s --check-prefix=CHECK-UNWINDER
+; RUN: FileCheck %s --input-file %t
+
 
 ; CHECK-UNCOMPRESS: [main:2 @ foo:5 @ fa:8 @ fa:7 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:6 @ fa:8 @ fa:7 @ fb:6 @ fa]:4:1
 ; CHECK-UNCOMPRESS:  1: 1
 ; CHECK-UNCOMPRESS:  3: 1
 ; CHECK-UNCOMPRESS:  4: 1
 ; CHECK-UNCOMPRESS:  7: 1 fb:1
 ; CHECK-UNCOMPRESS:  !CFGChecksum: 563070469352221
 ; CHECK-UNCOMPRESS: [main:2 @ foo:5 @ fa:8 @ fa:7 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:6 @ fa:8 @ fa]:4:1

llvm/tools/llvm-profgen/PerfReader.cpp

ProfiledBinary &PerfReaderBase::loadBinary(const StringRef BinaryPath,
   auto Ret = BinaryTable.insert({BinaryName, ProfiledBinary(BinaryPath)});
 
   if (!Ret.second && !AllowNameConflict) {
     std::string ErrorMsg = "Binary name conflict: " + BinaryPath.str() +
                            " and " + Ret.first->second.getPath().str() + " \n";
     exitWithError(ErrorMsg);
   }
 
-  return Ret.first->second;
+  // Initialize the base address to preferred address.
+  ProfiledBinary &B = Ret.first->second;
+  uint64_t PreferredAddr = B.getPreferredBaseAddress();
+  AddrToBinaryMap[PreferredAddr] = &B;
+  B.setBaseAddress(PreferredAddr);
+  B.setIsLoadedByMMap(false);
+
+  return B;
 }
 
 void PerfReaderBase::updateBinaryAddress(const MMapEvent &Event) {
   // Load the binary.
   StringRef BinaryPath = Event.BinaryPath;
   StringRef BinaryName = llvm::sys::path::filename(BinaryPath);
 
   auto I = BinaryTable.find(BinaryName);
   // Drop the event which doesn't belong to user-provided binaries
-  // or if its image is loaded at the same address
-  if (I == BinaryTable.end() || Event.Address == I->second.getBaseAddress())
+  if (I == BinaryTable.end())
     return;
 
   ProfiledBinary &Binary = I->second;
+  // Drop the event if its image is loaded at the same address
+  if (Event.Address == Binary.getBaseAddress()) {
+    Binary.setIsLoadedByMMap(true);
+    return;
+  }
 
   if (Event.Offset == Binary.getTextSegmentOffset()) {
     // A binary image could be unloaded and then reloaded at different
     // place, so update the address map here.
     // Only update for the first executable segment and assume all other
     // segments are loaded at consecutive memory addresses, which is the case on
     // X64.
     AddrToBinaryMap.erase(Binary.getBaseAddress());
     AddrToBinaryMap[Event.Address] = &Binary;
 
     // Update binary load address.
     Binary.setBaseAddress(Event.Address);
+
+    Binary.setIsLoadedByMMap(true);
   } else {
     // Verify segments are loaded consecutively.
     const auto &Offsets = Binary.getTextSegmentOffsets();
     auto It = std::lower_bound(Offsets.begin(), Offsets.end(), Event.Offset);
     if (It != Offsets.end() && *It == Event.Offset) {
       // The event is for loading a separate executable segment.
       auto I = std::distance(Offsets.begin(), It);
       const auto &PreferredAddrs = Binary.getPreferredTextSegmentAddresses();
void HybridPerfReader::parseSample(TraceStream &TraceIt) {
   // Parsing call stack and populate into HybridSample.CallStack
   if (!extractCallstack(TraceIt, Sample->CallStack)) {
     // Skip the next LBR line matched current call stack
     if (!TraceIt.isAtEoF() && TraceIt.getCurrentLine().startswith(" 0x"))
       TraceIt.advance();
     return;
   }
   // Set the binary current sample belongs to
-  Sample->Binary = getBinary(Sample->CallStack.front());
+  ProfiledBinary *PB = getBinary(Sample->CallStack.front());
+  Sample->Binary = PB;
+  if (!PB->getMissingMMapWarned() && !PB->getIsLoadedByMMap()) {
+    WithColor::warning() << "No relevant mmap event is matched, will use "
+                            "preferred address as the base loading address!\n";
+    // Avoid redundant warning, only warn at the first unmatched sample.
+    PB->setMissingMMapWarned(true);
+  }
 
   if (!TraceIt.isAtEoF() && TraceIt.getCurrentLine().startswith(" 0x")) {
     // Parsing LBR stack and populate into HybridSample.LBRStack
     if (extractLBRStack(TraceIt, Sample->LBRStack, Sample->Binary)) {
       // Canonicalize stack leaf to avoid 'random' IP from leaf frame skew LBR
       // ranges
       Sample->CallStack.front() = Sample->LBRStack[0].Target;
       // Record samples by aggregation
       Sample->genHashCode();
       AggregatedSamples[Hashable<PerfSample>(Sample)]++;
     }
   } else {
     // LBR sample is encoded in single line after stack sample
     exitWithError("'Hybrid perf sample is corrupted, No LBR sample line");
   }
 }
 
 void PerfReaderBase::parseMMap2Event(TraceStream &TraceIt) {
   // Parse a line like:
   //  PERF_RECORD_MMAP2 2113428/2113428: [0x7fd4efb57000(0x204000) @ 0
   //  08:04 19532229 3585508847]: r-xp /usr/lib64/libdl-2.17.so
   constexpr static const char *const Pattern =
       "PERF_RECORD_MMAP2 ([0-9]+)/[0-9]+: "
       "\\[(0x[a-f0-9]+)\\((0x[a-f0-9]+)\\) @ "
       "(0x[a-f0-9]+|0) .*\\]: [-a-z]+ (.*)";
   // Field 0 - whole line
   // Field 1 - PID
   // Field 2 - base address

 }
 
 void HybridPerfReader::generateRawProfile() { unwindSamples(); }
 
 void PerfReaderBase::parsePerfTraces(
     cl::list<std::string> &PerfTraceFilenames) {
   // Parse perf traces and do aggregation.
   for (auto Filename : PerfTraceFilenames)
     parseAndAggregateTrace(Filename);
 
   generateRawProfile();
 }
 
 } // end namespace sampleprof
 } // end namespace llvm

llvm/tools/llvm-profgen/ProfiledBinary.h

class ProfiledBinary {
   // The symbolizer used to get inline context for an instruction.
   std::unique_ptr<symbolize::LLVMSymbolizer> Symbolizer;
 
   // Pseudo probe decoder
   PseudoProbeDecoder ProbeDecoder;
 
   bool UsePseudoProbes = false;
 
+  // Indicate if the base loading address is parsed from the mmap event or uses
+  // the preferred address
+  bool IsLoadedByMMap = false;
+  // Use to avoid redundant warning.
+  bool MissingMMapWarned = false;
+
   void setPreferredTextSegmentAddresses(const ELFObjectFileBase *O);
 
   template <class ELFT>
   void setPreferredTextSegmentAddresses(const ELFFile<ELFT> &Obj, StringRef FileName);
 
   void decodePseudoProbe(const ELFObjectFileBase *Obj);
 
   // Set up disassembler and related components.
const AddressProbesMap &getAddress2ProbesMap() const {
     return ProbeDecoder.getAddress2ProbesMap();
   }
   const PseudoProbeFuncDesc *getFuncDescForGUID(uint64_t GUID) {
     return ProbeDecoder.getFuncDescForGUID(GUID);
   }
   const PseudoProbeFuncDesc *getInlinerDescForProbe(const PseudoProbe *Probe) {
     return ProbeDecoder.getInlinerDescForProbe(Probe);
   }
+
+  bool getIsLoadedByMMap() { return IsLoadedByMMap; }
+
+  void setIsLoadedByMMap(bool Value) { IsLoadedByMMap = Value; }
+
+  bool getMissingMMapWarned() { return MissingMMapWarned; }
+
+  void setMissingMMapWarned(bool Value) { MissingMMapWarned = Value; }
 };
 
 } // end namespace sampleprof
 } // end namespace llvm
 
 #endif