[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/Trace.h:31-32
+///
+/// This class assumes that errors will be extremely rare compared to the number
+/// of correct instructions and storing them as \a ConstString should be fine.
+class TraceInstruction {
----------------
clayborg wrote:
> Can't we just avoid including any errors in this TraceInstruction class? And have the function that generates these return a:
> 
> ```
> llvm::Expected<TraceInstruction>
> ```
> ?
not really, because these errors don't mean that the entire decoding failed, they instead mean that some specific chunks of the trace were not able to be decoded. 
For example, you can have this decoded trace:

```
a.out`(none)
    [ 4] 0x0000000000400510    pushq  0x200af2(%rip)            ; _GLOBAL_OFFSET_TABLE_ + 8
    [ 5] 0x0000000000400516    jmpq   *0x200af4(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 16
    [ 6] 0x00007ffff7df1950    error: no memory mapped at this address
    ...missing instructions
  a.out`main + 20 at main.cpp:10
    [ 7] 0x0000000000400674    movl   %eax, -0xc(%rbp)
```

Instruction 6 is an error. We have an associated address but the decoder couldn't decode it and then it skipped the trace until the next synchronization point, which is instruction 7 in this case.

Not all decoder errors have an associated address, though, only some. So I prefer just to have any errors be represented as a strings for now. 

Another option is to create a new class that contains a sequential list of valid instructions terminated by 0 or more contiguous errors. But that seems a little bit too much, as there's not a lot that we can do when we see errors, we either ignore them or we stop the current iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588