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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I don't see the change as "adding a bunch of timeouts", since what it mostly did was remove a bunch of send_async = false's.  For all the clients that don't care about a timeout, I removed the send_async parameter, and for those that did I replaced it with a timeout.  There were a few cases at the GDBRemoteClientBase layer where routines were calling the SendPacket routines with send_async = true w/o requesting that info from ProcessGDBRemote.  To those I added a timeout parameter.  That I consider a plus since it made explicit which packet request functions were interrupting and which were not.

Given the packet requests that called with send_async = false made up  ~95% of all uses, it seemed like marking the few uses that did want to interrupt by the fact that they explicitly receive a timeout parameter was cleaner than switching everybody over to a Timeout, and then having 95% of the clients still have to make up an empty Timeout.

Instead, the rule is now "client functions that are going to interrupt the target get passed a timeout, and otherwise you don't have to worry about it".  That removed a bunch of  boiler-plate and seems clear to me.

We could go further, as you suggest, and add an m_interrupt_timeout to GDBRemoteClientBase (*).  That would avoid ever having to pass a timeout into the SendPacket client functions in GDBRemoteClientBase.  We already have ValueChanged callbacks for Properties.  We'd have to add a virtual "InterruptTimeoutChanged" to Process to do the right thing for ProcessGDBRemote.  None of that would be hard to do.

In fact, I actually thought about doing that, but I didn't because I like the fact that from ProcessGDBRemote, you are able to tell which methods in GDBRemoteClientBase interrupt a running process, and which don't by whether they take a timeout or not.  If the timeout were in the GDBRemoteClientBase class you wouldn't see that.  But that does seems a thing worth knowing.  We could preserve that knowledge by adding some naming convention to distinguish between interrupting & non-interrupting methods.  But I don't see what that really gains us.

OTOH, if the general consensus is that is isn't important to know whether an information requesting function in GDBRemoteClientBase is interrupting or not, I can certainly add the variable and get it to track the setting changes in Process & ProcessGDBRemote.

(*) We would have to add it, the one that's already there belongs to the GDBRemoteClientBase::Lock class, which is short-lived, not to the GDBRemoteClientBase per se.



================
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);
 
----------------
clayborg wrote:
> 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?
Note, m_interrupt_timeout is not an ivar of GDBRemoteClientBase, it's an ivar of GDBRemoteClientBase::Lock.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102085/new/

https://reviews.llvm.org/D102085