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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:413
+      // skip it
       TraceIt.advance();
+      return false;
----------------
wlei wrote:
> wenlei wrote:
> > line 413 and line 416 can be hoisted to line 409?
> 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`.
I see.. thanks for clarification. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:414
       TraceIt.advance();
-      break;
+      return false;
     }
----------------
wlei wrote:
> wenlei wrote:
> > 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.. 
> > 
> > 
> 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.
Ok, update the comment then, and we can also assert that if `FrameStr.getAsInteger(16, FrameAddr)` is true, it must be empty line. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95480