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

wallace updated this revision to Diff 351329. wallace added a comment. Herald added a subscriber: mgorny. Following @vsk 's feedback, I made the following changes: - Create a ThreadTrace class, that contains the trace for a specific thread and offers the TraverseInstructions method and other get

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

wallace added a comment. Btw, thanks for the conversation. This is being really helpful. > I'm not convinced that retaining only 1 decoded instruction in memory at a > time (under a TraverseInstructions callback) will be sufficient for the use > cases you've outlined. Let's say the user sets a

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

vsk added a comment. In D103588#2806894 , @wallace wrote: > I've been thinking about what you said and I'm having second thoughts on my > implementation. I'll share more context: > > - I want to work in the short term on reverse debugging and reconstruct

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

wallace added a comment. I've been thinking about what you said and I'm having second thoughts on my implementation. I'll share more context: - I want to work in the short term on reverse debugging and reconstruction of stack traces, for that i'll need to know the instruction type of each inst

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

vsk added a comment. In D103588#2806611 , @wallace wrote: >> This approach - of prototyping trace analyses on a vector >> representation and then rewriting them later when scaling issues arise - >> doesn't sound good to me. Even for something simple, li

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

wallace added a comment. > This approach - of prototyping trace analyses on a vector > representation and then rewriting them later when scaling issues arise - > doesn't sound good to me. Even for something simple, like finding a > backtrace, the window of instructions needed for analysis can e

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

vsk added a comment. In D103588#2806512 , @wallace wrote: > Right now we haven't implemented lazy decoding; we are decoding the entire > trace. But that's just because so far we have been working with small traces. > As we progress in this work, we'll s

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

wallace requested review of this revision. wallace added a comment. > At Apple, we use lldb to navigate instruction traces that contain billions of > instructions. Allocating 16 bytes per instruction simply will not scale for > our workflows. We require the in-memory overhead to be approximately

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

vsk requested changes to this revision. vsk added a comment. This revision now requires changes to proceed. I'm quite concerned about the design decision to represent a trace as a vector of TraceInstruction. This bakes considerations that appear to be specific to Facebook's usage of IntelPT way

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

wallace updated this revision to Diff 350652. wallace added a comment. I tried to use a ConstString instead of a const char *, but that's not trivial because the compilers wants a safe destructor all the way down, and it seems that this is non-standard across compilers, except for C++17 and onwa

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

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. One quick either documentation update for the error string, or switch to ConstString... Probably best to the the error string docs as if we use fancy C++ union stuff it might not

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

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 {

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

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 {

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

wallace updated this revision to Diff 350495. wallace added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103588/new/ https://reviews.llvm.org/D103588 Files: lldb/include/lldb/Target/Trace.h lldb/include/lldb/lldb-e

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

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 {

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

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So it would be nice to try and not encode errors into the TraceInstruction class and deal with any errors at decode time. Comment at: lldb/include/lldb/Target/