[PATCH] D95480: [NFC] Fix bug with parsing hybrid sample trace line

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:414
       TraceIt.advance();
-      break;
+      return false;
     }
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wlei wrote:
> > > > hoy wrote:
> > > > > 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?
> > > > 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.
> > > > 
> > > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > 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?
> > > 
> > 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.
> Sounds good. Is it possible to add a test based on the current test binaries?
Yeah, Good idea! I can manually create a perfscript for this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95480/new/

https://reviews.llvm.org/D95480