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 the straight-line path which
does not encounter any errors or missing data or whatnot. Have you given that
any thought?
================
Comment at: lldb/include/lldb/Target/Trace.h:169
+ const Thread &thread,
+ std::function<bool(size_t index, const Instruction &insn)> callback,
+ size_t from = 0) = 0;
----------------
It sounds like this could just be `llvm::function_ref<bool(size_t index,
Expected<addr_t> load_addr>` and the Instruction class does not even need to
exist. At least not here -- it may still be useful for the PT plugin to store
the instructions in some sort of an error-or-load-addr union, but there's no
need to impose that organization on anyone else.
================
Comment at: lldb/include/lldb/Target/Trace.h:193
+ /// or an actual Error in case of failures.
+ virtual llvm::Error IsTraceFailed(const Thread &thread) = 0;
};
----------------
I'd expect a method called `IsXXX` to return bool.
================
Comment at: lldb/include/lldb/Target/TraceThread.h:23
+/// files.
+class TraceThread : public Thread {
+public:
----------------
I'm very confused by TraceThread residing in the `Target` library and
ProcessTrace in `Plugin/Process/Trace`. I think both of them should be in the
same place (I could make a case for either location).
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:77
+ DecodedThread(std::vector<IntelPTInstruction> &&instructions)
+ : m_instructions(instructions) {}
+
----------------
std::move(instructions)
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:84
+ /// The instructions of the trace.
+ const std::vector<IntelPTInstruction> &GetInstructions() const;
+
----------------
`ArrayRef<IntelPTInstruction>` maybe?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:24-31
+static std::vector<uint8_t> ReadTraceFile(const FileSpec &trace_file) {
+ char file_path[PATH_MAX];
+ trace_file.GetPath(file_path, sizeof(file_path));
+ std::ifstream src(file_path, std::ios::in | std::ios_base::binary);
+ std::vector<uint8_t> trace((std::istreambuf_iterator<char>(src)),
+ std::istreambuf_iterator<char>());
+ return trace;
----------------
`llvm::MemoryBuffer::getFile(filename)`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:33
+
+/// Get a list of the sections of the given process that are Read or Exec,
+/// which are the only sections that could correspond to instructions traced.
----------------
and? That's what the code seems to be doing.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:35
+/// which are the only sections that could correspond to instructions traced.
+std::vector<lldb::SectionSP> static GetReadExecSections(Process &process) {
+ Target &target = process.GetTarget();
----------------
Please put static first (I'm surprised this even compiles).
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:68
+/// The instructions decoded.
+void DecodeInstructions(pt_insn_decoder &decoder,
+ std::vector<IntelPTInstruction> &instructions) {
----------------
`static std::vector<IntelPTInstruction> DecodeInstructions`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:177
+/// A libipt error code in case of failure or 0 otherwise.
+int CreateDecoderAndDecode(Process &process, const pt_cpu &pt_cpu,
+ std::vector<uint8_t> &trace,
----------------
Expected<vector<Insns>>
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:197
+ char path[PATH_MAX];
+ section_sp->GetModule()->GetPlatformFileSpec().GetPath(path, sizeof(path));
+ if (int errcode = pt_image_add_file(
----------------
Use the GetPath overload returing a std::string
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:199
+ if (int errcode = pt_image_add_file(
+ image, path, section_sp->GetFileOffset(),
section_sp->GetByteSize(),
+ nullptr, section_sp->GetLoadBaseAddress(&process.GetTarget()))) {
----------------
So will this make the library load the file into memory once again? Is there no
way to make it use the copy already loaded by lldb?
================
Comment at: lldb/source/Target/TraceSessionFileParser.cpp:125
+ ProcessSP process_sp(target_sp->CreateProcess(
+ /*listener*/ nullptr, "trace",
+ /*crash_file*/ nullptr));
----------------
This is not a dependency in the strictest sense but it still means that this
code would explode if the ProcessTrace "plugin" is plugged "out". It sounds
like that, in this design, the ProcessTrace class should just be a part of lldb
core.
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
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
