[Lldb-commits] [PATCH] D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in

[Greg Clayton via Phabricator via lldb-commits]

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