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

[Lei Wang via Phabricator via llvm-commits]

wlei added a comment.

In D95480#2524512 <https://reviews.llvm.org/D95480#2524512>, @wenlei wrote:

> and btw.. bug fix isn't really NFC

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



================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:414
       TraceIt.advance();
-      break;
+      return false;
     }
----------------
hoy wrote:
> wenlei wrote:
> > 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. 
> 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?
> 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`.







Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95480