[CSSPGO][llvm-profgen] Fix bug with parsing hybrid sample trace line

[wlei]

Summary

when we skip the call stack starting with an external address, we should also skip the bottom LBR entry, otherwise it will cause a truncated context issue.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

445

A truncated context might still be helpful to the sample loader inliner. We saw this happened to some SPEC2006 benchmarks. What issue did it cause to you?

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

445

Sorry for the confusion. This line is for the non-address line like empty line.

This patch is for the issue like below

external address1
..
address2
address3
LBR entry

The first one is an external address1, so we need also skip the address2 and address3. otherwise, the missing part is the top frame, it will make the unwinder incorrect.

For the right truncated context, it's like below

address1
address2
external address3
..
LBR entry

Here the truncated part is at the bottom, then the compiler can use this for the inliner. The tool can handle this case.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

445

Oh I see. Thanks for explaining. So with your change, we now restart when seeing an empty line. And we continue scanning even with an empty call stack to avoid breaking the samples into two parts, with the second part being a truncated call stack?

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

445

Yeah, exactly. Our internal prototype didn't have this issue since the whole trace file is split by the '\n\n', so it will always ensure a whole sample and avoid breaking into two parts. But for llvm-profgen, it's processed line by line and previously it will start parsing new sample when the call stack is empty which will cause this bug when the first address is an external one.

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

445

Sounds good. Is it possible to add a test based on the current test binaries?

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

445

Yeah, Good idea! I can manually create a perfscript for this.

[wlei]

add test for this

[wlei]

fix indentation

[wlei]

fix indentation..

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

445

Just copy another sample and added a fake top address on it. with out this fix, it will generate wrong profile count.

[hoy]

LGTM, thanks for the fix!

llvm/tools/llvm-profgen/PerfReader.cpp

486–487

Nit: comment may be updated to reflect the truncated callstack case.

[wlei]

update comments

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

444

line 413 and line 416 can be hoisted to line 409?

445

This change means we no longer handle a comment line mixed in stack sample? Although I'm not sure if we ever need a comment line..

[wlei]

llvm/tools/llvm-profgen/PerfReader.cpp

444

This is tricky, because the underlying TraceStream read line by line .. you see in line 408 StringRef FrameStr which use StringRef, if we hoist TraceIt.advance();, the internal string instance will be freed. That means we must ensure to use the StringRef before call advance. have the comments in TraceStream.

445

yeah, I think so. For the perf generated perfscript, I saw the only case is a log line at the head of the file, shouldn't have comments in other place. Here the comments are most for the regression test, but as you see we have to make sure it's not mixed in the sample.

[wenlei]

llvm/tools/llvm-profgen/PerfReader.cpp

444

I see.. thanks for clarification.

445

Ok, update the comment then, and we can also assert that if FrameStr.getAsInteger(16, FrameAddr) is true, it must be empty line.

[wenlei]

and btw.. bug fix isn't really NFC

[hoy]

llvm/tools/llvm-profgen/PerfReader.cpp

445

A comment line at the beginning of a callstack sample should still be OK, but not for a comment in the middle of a callstack sample. Previously we would just skip the frames following a comment. Will a continue work here if we need comments?

[wlei]

In D95480#2524512, @wenlei wrote:

and btw.. bug fix isn't really NFC

Ah,I misuse it.. let me change the title.

llvm/tools/llvm-profgen/PerfReader.cpp

445

we can also assert that if FrameStr.getAsInteger(16, FrameAddr) is true, it must be empty line.

Sorry I just make mistakes to explain, it could have comments between different sample but not in the middle of sample(as Hongtao reminded), like:

...

address1
address2
LBR

;legal comment

address3
;illegal comment
address4
LBR

Will a continue work here if we need comments?

continue might not work here. because we could have MMAP event, like:

; comment1
MMAP_Event_..
address1
...

So it will also skip the MMAP line, that's why we return false to defer parsing the next line to parseEventOrSample.

llvm/test/tools/llvm-profgen/Inputs/inline-cs-noprobe.perfscript

 PERF_RECORD_MMAP2 2854748/2854748: [0x400000(0x1000) @ 0 00:1d 123291722 526021]: r-xp /home/inline-cs-noprobe.perfbin
 
+; test for an external or invalid top address, should skip the whole sample
+
+          ffffffff
+            40067e
+  5541f689495641d7
+ 0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x40069b/0x400670/M/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0
 
             40067e
   5541f689495641d7
  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x40069b/0x400670/M/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0  0x4006c8/0x40067e/P/-/-/0

llvm/tools/llvm-profgen/PerfReader.cpp

bool PerfReader::extractCallstack(TraceStream &TraceIt,
   //            400634
   //            400684      # root frame
   // It's in bottom-up order with each frame in one line.
 
   // Extract stack frames from sample
   ProfiledBinary *Binary = nullptr;
   while (!TraceIt.isAtEoF() && !TraceIt.getCurrentLine().startswith(" 0x")) {
     StringRef FrameStr = TraceIt.getCurrentLine().ltrim();
-    // We might get an empty line at the beginning or comments, skip it
     uint64_t FrameAddr = 0;
     if (FrameStr.getAsInteger(16, FrameAddr)) {
+      // We might parse a non-perf sample line like empty line and comments,
+      // skip it
       TraceIt.advance();
-      break;
+      return false;
     }
     TraceIt.advance();
     if (!Binary) {
       Binary = getBinary(FrameAddr);
       // we might have addr not match the MMAP, skip it
       if (!Binary) {
         if (AddrToBinaryMap.size() == 0)
           WithColor::warning() << "No MMAP event in the perfscript, create it "
while (!TraceIt.isAtEoF() && !TraceIt.getCurrentLine().startswith(" 0x")) {
     // for non-leaf frames
     if (!CallStack.empty()) {
       FrameAddr = Binary->getCallAddrFromFrameAddr(FrameAddr);
     }
 
     CallStack.emplace_back(FrameAddr);
   }
 
-  if (CallStack.empty())
-    return false;
   // Skip other unrelated line, find the next valid LBR line
+  // Note that even for empty call stack, we should skip the address at the
+  // bottom, otherwise the following pass may generate a truncated callstack
   while (!TraceIt.isAtEoF() && !TraceIt.getCurrentLine().startswith(" 0x")) {
     TraceIt.advance();
   }
   // Filter out broken stack sample. We may not have complete frame info
   // if sample end up in prolog/epilog, the result is dangling context not
   // connected to entry point. This should be relatively rare thus not much
   // impact on overall profile quality. However we do want to filter them
   // out to reduce the number of different calling contexts. One instance
   // of such case - when sample landed in prolog/epilog, somehow stack
   // walking will be broken in an unexpected way that higher frames will be
   // missing.
-  return !Binary->addressInPrologEpilog(CallStack.front());
+  return !CallStack.empty() &&
+         !Binary->addressInPrologEpilog(CallStack.front());
 }
 
 void PerfReader::parseHybridSample(TraceStream &TraceIt) {
   // The raw hybird sample started with call stack in FILO order and followed
   // intermediately by LBR sample
   // e.g.
   //            4005dc    # call stack leaf
   //            400634