[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:
> 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?
disregard this comment, I understood your remark incorrectly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588