[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 a comment.

In D102085#2745450 <https://reviews.llvm.org/D102085#2745450>, @jingham wrote:

> 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.

I just note when I see a ton of changes to things that can easily be fixed by adding an ivar I tend to do it. It is fine that the "bool async" parameter went away.

We should probably combine the two functions that only differ by one taking a timeout and the other not. They are very similar and can easily use a Optional<> on the timeout parameter.

> 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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102085