[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
lldb-commits at lists.llvm.org
Fri May 7 14:52:41 PDT 2021
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();
+ 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?
Note, m_interrupt_timeout is not an ivar of GDBRemoteClientBase, it's an ivar of GDBRemoteClientBase::Lock.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the lldb-commits