clayborg added inline comments.
================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:370 + m_should_interrupt(true), m_did_interrupt(false) { + SyncWithContinueThread(); + if (m_acquired) ---------------- if "interrupt_timeout" is optional, it can just be passed along to SyncWithContinueThread(interrupt_timeout) and avoid the need for m_should_interrupt. ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:387 std::unique_lock<std::mutex> lock(m_comm.m_mutex); - if (m_comm.m_is_running && !interrupt) + if (m_comm.m_is_running && !m_should_interrupt) return; // We were asked to avoid interrupting the sender. Lock is not ---------------- It we pass take a "llvm::Optional<std::chrono::seconds> interrupt_timeout = llvm::None" as a parameter to this, we can avoid the need for m_should_interrupt to exist and can test if it has no value ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:36 - bool SendAsyncSignal(int signo); + bool SendAsyncSignal(int signo, std::chrono::seconds interrupt_timeout); ---------------- There are a lot of interrupt_timeout parameters in many methods here. Should we pass the interrupt timeout to the constructor and keep it as a member variable? ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:55 + StringExtractorGDBRemote &response, + std::chrono::seconds interrupt_timeout); ---------------- This function and the one above could be made into one function if we change the interrupt timeout to be defined as: ``` llvm::Optional<std::chrono::seconds> interrupt_timeout = llvm::None ``` ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:57-60 PacketResult SendPacketAndReceiveResponseWithOutputSupport( llvm::StringRef payload, StringExtractorGDBRemote &response, - bool send_async, + std::chrono::seconds interrupt_timeout, llvm::function_ref<void(llvm::StringRef)> output_callback); ---------------- Is this function only used when sending async now? ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:73 + // succeed. + Lock(GDBRemoteClientBase &comm, std::chrono::seconds interrupt_timeout); ~Lock(); ---------------- Combine with above function and change to optional?: ``` llvm::Optional<std::chrono::seconds> interrupt_timeout = llvm::None ``` ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:85 GDBRemoteClientBase &m_comm; + std::chrono::seconds m_interrupt_timeout; bool m_acquired; ---------------- Can't we just set this and update it if the user changes the setting and avoid passing "std::chrono::seconds interrupt_timeout" to many of the member functions in this class and switch any such parameters to a bool? ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:325 + uint32_t length, // Byte Size of breakpoint or watchpoint + std::chrono::seconds interrupt_timeout); // Time to wait for an interrupt ---------------- We have a ton of new "std::chrono::seconds interrupt_timeout" parameters. Since GDBRemoteClientBase already has an ivar for this, can't we just use this and pass a bool or no param instead? ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:514-515 - llvm::Expected<TraceSupportedResponse> SendTraceSupported(); + llvm::Expected<TraceSupportedResponse> + SendTraceSupported(std::chrono::seconds interrupt_timeout); ---------------- We have a ton of new "std::chrono::seconds interrupt_timeout" parameters. Since GDBRemoteClientBase already has an ivar for this, can't we just use this and pass a bool or no param instead? ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:517-518 - llvm::Error SendTraceStart(const llvm::json::Value &request); + llvm::Error SendTraceStart(const llvm::json::Value &request, + std::chrono::seconds interrupt_timeout); ---------------- Ditto ================ Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1213 llvm::Expected<TraceSupportedResponse> ProcessGDBRemote::TraceSupported() { - return m_gdb_comm.SendTraceSupported(); + return m_gdb_comm.SendTraceSupported(GetInterruptTimeout()); } ---------------- We should just set the interrupt timeout when we create the m_gdb_comm and then we won't have to pass this in to each call? We just need to make sure we update the timeout if it gets changed during a debug session, but that isn't hard. ================ Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1217 llvm::Error ProcessGDBRemote::TraceStop(const TraceStopRequest &request) { - return m_gdb_comm.SendTraceStop(request); + return m_gdb_comm.SendTraceStop(request, GetInterruptTimeout()); } ---------------- ditto and for many calls below Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102085/new/ https://reviews.llvm.org/D102085 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits