[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 {
----------------
wallace wrote:
> 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.
It should be possible. I wonder about memory consumption.
llvm::Expected has these members:

```
union {
    AlignedCharArrayUnion<storage_type> TStorage;
    AlignedCharArrayUnion<error_type> ErrorStorage;
  };
  bool HasError : 1;
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
  bool Unchecked : 1;
#endif
```

that means that it would end up using 3 * 8 bytes in total, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588