[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcfd96f057ba4: [trace][intel-pt] Implement the basic decoding functionality (authored by Walter Erquinigo wall...@fb.com, committed by wallace).

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. I am good too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added a comment. Thanks. I'm thinking about doing the early decoding, as doing "trace load" right now doesn't do anything very useful unless you do "dump instructions". However, I'll try to do it once I finish implementing decoding for live processes, because I want to have a unified

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

labath added a comment. I'd say this looks ok now. Another advantage of doing the decoding early is that we could then simplify the IntelPTInstruction class. If we handle the early decoding errors (file not found, etc.) early, then the only kind of errors this class could contain are errors

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 302354. wallace added a comment. Followed the suggestion by @labath. The code definitely looks better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

labath added a subscriber: lhames. labath added a comment. A more light-weight approach to this would be to store the `ErrorInfo` object from the `Error` and copy that when needed. Something like: class IntelPTInstruction { std::unique_ptr m_error; // Technically, this should be a

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 302000. wallace added a comment. Address changes. - Made IntelPTError and IntelPTInstruction non-copyable, which simplified the code. - I still keep IntelPTError as a lightweight way to create Error objects on demand when traversing the trace. Notice that

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:116 + pt_insn m_pt_insn; + std::shared_ptr m_error; +}; wallace wrote: > vsk wrote: > > Still doesn't feel like this is aligning with the design of llvm::Error - > >

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:26 +public: + IntelPTError() = default; + vsk wrote: > Do we need a default constructor for this pure-virtual class? The compiler forces me to do it because I declare

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:26 +public: + IntelPTError() = default; + Do we need a default constructor for this pure-virtual class? Comment at:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added a comment. friendly ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 301130. wallace added a comment. nits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files: lldb/include/lldb/Core/Disassembler.h

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 301128. wallace added a comment. Followed @vsk's suggestion. I didn't do exactly what he said, but I ended up creating my own IntelPTError to represent better the different kinds of errors and make sure we can create a correct llvm::Error when traversing

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added a comment. @wallace thanks for winnowing the test objects. I left an inline suggestion about simplifying the error-handling in IntelPTInstruction. Other than that, mechanically this is looking good. Thanks! Comment at:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 300864. wallace marked 2 inline comments as done. wallace added a comment. Address issues. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files: lldb/include/lldb/Core/Disassembler.h

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace marked 7 inline comments as done. wallace added a comment. > That makes me sad, but I am not going to hold this patch over that. I would > encourage you to find and implement the missing bits in yaml2obj though... I'll do that later as a way to learn yaml2obj.

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

labath added a comment. I'm not sure if I'll have time to go through this again, but it seems ok. One question inline, though. In D89283#2348815 , @wallace wrote: > The diff is the now ready for review. There are a few updates, including some > design

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 300134. wallace added a comment. The diff is the now ready for review. There are a few updates, including some design decisions after some chats with Greg. - Now the dump command includes disassembly information and symbol context information whenever

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 299524. wallace added a comment. Herald added a subscriber: dang. Some updates, especially regarding the indexing and dump ordering, which I discussed at lenght with some coworkers: - I've addressed most of the issues, except for the disassembly and the

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace marked 10 inline comments as done. wallace added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:109 + else { +consumeError(decoded_thread.takeError()); +return 0; labath wrote: > wallace wrote: > > labath wrote:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

labath added a comment. In D89283#2340586 , @wallace wrote: > What's left to discuss: > > - I tried to use obj2yaml, but it creates files much larger than the > binaries, so in terms of space, I'd rather keep the binaries. Besides, I > removed ld.so,

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added a comment. > Do we need to know the instruction count? Again, won't this be really > expensive to calculate? It is expensive, but today I realized that it might be necessary to decode the entire trace if you want things to work nicely. Let's take, for example, a very common

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

clayborg added a comment. In D89283#2340586 , @wallace wrote: > Addressed almost all changes. > > What's left to discuss: > > - I tried to use obj2yaml, but it creates files much larger than the > binaries, so in terms of space, I'd rather keep the

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 299236. wallace marked 3 inline comments as done. wallace added a comment. Addressed almost all changes. What's left to discuss: - I tried to use obj2yaml, but it creates files much larger than the binaries, so in terms of space, I'd rather keep the

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace marked 16 inline comments as done. wallace added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:188 + if (int error = pt_image_set_callback(image, ReadProcessMemory, )) +return CreateLibiptError(error); + labath

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace marked 3 inline comments as done. wallace added a comment. @labath is right regarding the need of pre-baked binaries to test specific conditions. I'll remove the ld binary, as it really tests nothing useful, and i'll try to use yaml to represent the binaries in a more concise format.

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

labath added a comment. In D89283#2336280 , @vsk wrote: > In D89283#2336120 , @wallace wrote: > >> @vsk, I agree with you regarding the files. At the moment our implementation >> of intel-pt tracing doesn't

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added a comment. In D89283#2336120 , @wallace wrote: > @vsk, I agree with you regarding the files. At the moment our implementation > of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. > Then, we'll be able to generate

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

clayborg added a comment. As I stated before we should be printing the disassembly for each instruction on the output lines. We should probably also normalize the address hex value to it doesn't change widths. Something like: [ 0] 0x0040065f: pushq %rbp [ 1] 0x0040065a:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Instead of just seeing the address, we should disassemble the instruction at the address in this patch for clarity if it is available. If we can't read the opcode from the

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298771. wallace added a comment. - Remove the llvm::consumeExpected function, as @vsk suggested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added inline comments. Comment at: llvm/include/llvm/Support/Error.h:1016 +/// expected-producer might be more clearly refactored to return an Optional. +template inline void consumeExpected(Expected E) { + if (!E) vsk wrote: > Probably best to not

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added a comment. @vsk, I agree with you regarding the files. At the moment our implementation of intel-pt tracing doesn't support collecting a trace, but soon we'll do so. Then, we'll be able to generate these trace files on the fly as the tests run, so I imagine I'll be deleting these

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298768. wallace added a comment. removing unwanted changes in lldb/tools/intel-features/intel-pt/Decoder.cpp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added inline comments. Comment at: lldb/tools/intel-features/intel-pt/Decoder.cpp:301 + std::ofstream of("/tmp/multi-file.trace", std::ios::binary | std::ios::out); + for (auto bait : pt_buffer) vsk wrote: > Not sure what this is in aid of? I

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

vsk added a comment. I'm not sure if the current revision of the patch reflects the long-term testing strategy. But if so, I'm quite concerned about the proliferation of large binary files in the repo (like ld.so, or the raw trace binary itself). These are opaque blobs that are hard to

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace marked 2 inline comments as done. wallace added inline comments. Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:142-143 +substrs=['''thread #1: tid = 3842849, total instructions = 2 + [0] no memory mapped at this address + [1] no

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298762. wallace added a comment. Figured out how to show the addressed that failed to decode. Now the output in those cases is something like [ 4] no memory mapped at this address: 0x77df1950 This should address all of Greg's comments Repository:

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace added a comment. nvm, libipt does report the addresses that failed to read, I'll quickly fix it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 ___

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace marked 8 inline comments as done. wallace added a comment. > When you are dumping instructions we are only showing one hex value. Is this > the instruction address or the opcode itself? That's the instruction load address. In a later diff I'll implement pretty-printing similar to the

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Great start to this. Many comments inlined. When you are dumping instructions we are only showing one hex value. Is this the instruction address or the opcode itself?

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298673. wallace added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files: lldb/include/lldb/Target/ProcessTrace.h

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298495. wallace added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files: lldb/include/lldb/Target/Trace.h

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298311. wallace added a comment. cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files: lldb/include/lldb/Target/Trace.h

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298253. wallace added a comment. - Moved the non intel-pt changes to D89408 - Added some more complex tests: - A multifile case in which even the dynamic linker code is decoded - A variation of that case that doesn't

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 298052. wallace marked 11 inline comments as done. wallace added a comment. Changes: - Changed the callback signature of ForEachInstruction to receive an Expected - Use Expected more ubiquitously in IntelPTDecoder - Use MemoryBuffer to read the trace file -

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

labath added a comment. I'm still digesting this, but here's my first set of comments. From a high-level perspective. it's not clear to me how to test the exceptional states that can occur when decoding a trace. The decoding function is pretty complex, but the current tests only seem to cover

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace updated this revision to Diff 297736. wallace added a comment. nits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89283/new/ https://reviews.llvm.org/D89283 Files: lldb/include/lldb/Target/Trace.h

[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

wallace created this revision. Herald added subscribers: lldb-commits, mgorny. Herald added a reviewer: JDevlieghere. Herald added a project: LLDB. wallace requested review of this revision. This diff finally implements trace decoding! The current interface is $ trace load