[PATCH] D95480: [NFC] Fix bug with parsing hybrid sample trace line
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 26 23:28:59 PST 2021
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
More information about the llvm-commits
mailing list