[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–326

nit: prefered -> preferred

746

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

746

Good idea! Fixed.

[wlei]

lint

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

662

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

662

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

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

[wlei]

add comments

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

662

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

662

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

662

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

650–657

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 ↗

(On Diff #363506)

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

650–657

Good point!

llvm/tools/llvm-profgen/ProfiledBinary.h

145 ↗

(On Diff #363506)

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 ↗

(On Diff #363506)

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

protected:
 
   BinaryMap BinaryTable;
   AddressBinaryMap AddrToBinaryMap; // Used by address-based lookup.
 
   BinarySampleCounterMap BinarySampleCounters;
   // Samples with the repeating time generated by the perf reader
   AggregatedCounter AggregatedSamples;
   PerfScriptType PerfType = PERF_UNKNOWN;
+
+  Optional<bool> EventSeenFirst;
 };
 
 /*
   Hybrid perf script includes a group of hybrid samples(LBRs + call stack),
   which is used to generate CS profile. An example of hybrid sample:
     4005dc    # call stack leaf
     400634
     400684    # call stack root

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);
+
+  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);
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)]++;
+      if (!EventSeenFirst.hasValue()) {
+        EventSeenFirst = false;
+      }
     }
   } 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) {
+  if (EventSeenFirst.hasValue() && !EventSeenFirst.getValue()) {
+    WithColor::warning() << "Interior mmap event but no leading mmap event!\n";
+  } else if (!EventSeenFirst.hasValue()) {
+    EventSeenFirst = true;
+  }
+
   // 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

 }
 
 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