wallace requested changes to this revision. wallace added a comment. This revision now requires changes to proceed.
much closer! I'm glad you are starting to understand the patterns we use for this kind of code ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:13 #include <memory> +#include <unordered_map> +#include <utility> ---------------- delete this ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:14 +#include <unordered_map> +#include <utility> ---------------- you don't need to import it. Maybe your c++ vscode extension has been autoimporting this one, in which case it's better to disable that feature. ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:134-137 + m_errors.insert(std::pair<uint64_t, std::unique_ptr<ErrorInfoBase>>( + m_instructions.size(), + std::move(info) + )); ---------------- emplace will prevent unnecessary copies and also doesn't need you to pass a pair ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:142-144 + return m_raw_trace_size + + IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size() + + sizeof(DecodedThread); ---------------- in order to have correct formatting all along, you need to use git clang-format: https://llvm.org/docs/Contributing.html#format-patches follow that guide. Whenever you are going to submit a patch, first run git clang-format and it will format your code correctly, so that you never again have to lose time doing that. It can even format comments ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:143-144 + return m_raw_trace_size + + IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size() + + sizeof(DecodedThread); } ---------------- here you also need to ask for the size of the DenseMap ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:42-45 + // /// \param[in] string_error + // /// StringError to copy from + // IntelPTError(StringError string_error); + ---------------- delete commented code ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:76 /// libipt errors should use the underlying \a IntelPTError class. - IntelPTInstruction(llvm::Error err); + IntelPTInstruction(llvm::Error &err); ---------------- Pass the error by const reference, because we don't modify it ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:141 - /// Get the instructions from the decoded trace. Some of them might indicate - /// errors (i.e. gaps) in the trace. + /// Get the instructions from the decoded trace. /// ---------------- ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:150 + /// \return + /// The error or \a llvm::Error::success if the given index + /// points to a valid instruction. ---------------- ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:155 + /// Append a successfully decoded instruction. + void AppendInstruction(IntelPTInstruction ins); + ---------------- remove this one. We need the version that accepts a parameter pack, as we discussed offline ================ Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:157-158 + + /// Append a decoding error (i.e. an instruction that failed to be decoded). + void AppendError(llvm::Error err); + ---------------- same here ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:111-112 + DecodedThreadSP decoded_thread = + std::make_shared<DecodedThread>(DecodedThread( + thread_sp, std::vector<IntelPTInstruction>(), raw_trace_size)); ---------------- the magic of make_shared is that it uses parameter packs, so that it only constructs the object right where it'll store it, thus preventing unnecessary copies ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:184 +static Expected<DecodedThreadSP> +DecodeInMemoryTrace(const ThreadSP &thread_sp, Process &process, + TraceIntelPT &trace_intel_pt, ---------------- don't pass the process, because we can get it from the thread_sp object ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:207 int errcode = pt_image_set_callback(image, ReadProcessMemory, &process); assert(errcode == 0); ---------------- if the compiler complains mentioning that the Process pointer you are passing is const, you can do an unsafe cast that removes the const qualifier and write a comment here. I hope you don't need it anyway ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:235 if (!buffer) - return buffer.takeError(); - raw_trace_size = buffer->size(); - if (Expected<pt_cpu> cpu_info = trace.GetCPUInfo()) - return DecodeInMemoryTrace(*thread.GetProcess(), trace, - MutableArrayRef<uint8_t>(*buffer)); + return DecodedThread(m_thread_sp, buffer.takeError()).shared_from_this(); + ---------------- use make_shared instead of shared_from_this. That will be much more performant ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:239 + if (Expected<DecodedThreadSP> dtsp = + DecodeInMemoryTrace(m_thread_sp, *m_thread_sp->GetProcess(), + m_trace, MutableArrayRef<uint8_t>(*buffer))) ---------------- don't pass the process, it's not necessary anymore ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:243-247 + return DecodedThread(m_thread_sp, cpu_info.takeError()) + .shared_from_this(); else - return cpu_info.takeError(); + return DecodedThread(m_thread_sp, cpu_info.takeError()) + .shared_from_this(); ---------------- same here ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:256 -DecodedThreadSP PostMortemThreadDecoder::DoDecode() { - size_t raw_trace_size = 0; - if (Expected<std::vector<IntelPTInstruction>> instructions = - DecodeTraceFile(*m_trace_thread->GetProcess(), m_trace, - m_trace_thread->GetTraceFile(), raw_trace_size)) - return std::make_shared<DecodedThread>(m_trace_thread->shared_from_this(), - std::move(*instructions), - raw_trace_size); - else - return std::make_shared<DecodedThread>(m_trace_thread->shared_from_this(), - instructions.takeError()); -} +static Expected<DecodedThreadSP> DecodeTraceFile(const ThreadSP &thread_sp, + Process &process, ---------------- just return a direct DecodedThreadSP that holds any errors you might find here, same like LiveThreadDecoder::DoDecode() ================ Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:275-280 + if (Expected<DecodedThreadSP> dtsp = DecodeTraceFile(m_trace_thread, *m_trace_thread->GetProcess(), + m_trace, m_trace_thread->GetTraceFile())) + return *dtsp; + // else + // return DecodedThread(m_trace_thread, cpu_info.takeError()) + // .shared_from_this(); ---------------- this will become simpler once DecodeTraceFile returns directly a DecodedThreadSP Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122293/new/ https://reviews.llvm.org/D122293 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
