[Lldb-commits] [PATCH] D96176: Implement jAttachWait

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

In D96176#2546777 <https://reviews.llvm.org/D96176#2546777>, @labath wrote:

> I'm not sure this new functionality is really worth the new packet (or two), but if it solves a use case you care about, then I suppose that's fine.

To be honest, I started this as I thought it'd be useful to LLDB users. If you think it's more trouble than it's worth, I'd be OK abandoning this change (I'm also OK completing it).

In D96176#2547429 <https://reviews.llvm.org/D96176#2547429>, @labath wrote:

> In D96176#2547324 <https://reviews.llvm.org/D96176#2547324>, @augusto2112 wrote:
>
>> In D96176#2546777 <https://reviews.llvm.org/D96176#2546777>, @labath wrote:
>>
>>> One alternative could be to just tack on some extra data to the existing vAttach family packets  (`vAttachWait;foo;interval:47;duration=74`).
>>
>> This was how I did it originally on https://reviews.llvm.org/D93895. @clayborg pointed out that this might not be compatible with existing lldb-server implementations, and suggested a new packet.
>
> If we only append the extra fields when the user explicitly requests them, then I don't think this would be a compatibility issue. The way I see it, one of two things can happen in this case:
> a) the server ignores the extra fields and completes the attach with the default values
> b) the server balks at the packet, and returns an error
> I think both of them are reasonable results and they can be fixed/worked around in the same way -- just drop the special requests...

If you guys think that's less messy, I could do that. Although I think it'd be nice to warn users if the parameters they're passing are ignored.



================
Comment at: lldb/include/lldb/Utility/StringExtractorGDBRemote.h:136
     eServerPacketType_vAttachOrWait,
+    eServerPacketType_jAttachWait,
     eServerPacketType_vAttachName,
----------------
augusto2112 wrote:
> labath wrote:
> > clayborg wrote:
> > > So we currently have 4 flavors from the old way: vAttach, vAttachWait vAttachOrWait, vAttachName. Do we want to possibly make a "jAttach" that works for all cases? The new "jAttach" could be powerful enough to do it all in one packet if needed. I just worry about attaching the "Wait" to the "jAttachWait" command name in case we want to use it for all of the above cases at a later point. 
> > > 
> > > A "jAttach" could implement all of:
> > > - attach to existing pid
> > > - attach to by name
> > >   - unique existing process
> > >   - wait
> > >     - allow existing (true/false)
> > >     - poll interval (time in usec)
> > >     - wait duration (time in sec)
> > > 
> > > Not that we need to add support for all of these flavor with this patch, I'm just trying to forward think about the future in case other attach flags are added to any flavor of attach. 
> > Sounds like a good idea.
> Sure! I was considering that as well. I wasn't sure what granularity lldb's packets should have, which is why I decided to start only with the attachWait first and see what you thought. I can include this in this patch, I don't think it's a lot of extra work.
Actually, it looks like this would be more work than I though. I changed the name to "jAttach" but am keeping only the existing functionality so this patch doesn't get too big.


================
Comment at: lldb/include/lldb/lldb-enumerations.h:600-601
   eArgTypeModuleUUID,
+  eArgTypeWaitforInterval,
+  eArgTypeWaitforDuration,
   eArgTypeLastArg // Always keep this entry as the last entry in this
----------------
labath wrote:
> What's up with this? Though this is the first time I see this table, I suspect it is not necessary to create a new command argument....
Seems like this is necessary when adding arguments to parameters. I think that the name must match whatever is define inside the `Arg` tag in `Options.td`:

```
  def process_attach_waitfor_interval : Option<"waitfor-interval", "I">,
    Group<2>, Arg<"WaitforInterval">,
    Desc<"The interval that waitfor uses to poll the list of processes.">;
  def process_attach_waitfor_duration : Option<"waitfor-duration", "d">,
    Group<2>, Arg<"WaitforDuration">,
```


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:394
   ProcessInstanceInfoList loop_process_list;
-  while (true) {
+  while (!timeout.hasValue() || now < target) {
     loop_process_list.clear();
----------------
labath wrote:
> `<= target` ? In case of a zero timeout, I guess this should loop at least once. Or, even better:
> ```
> do {
>   ...
> } while (std::chrono::steady_clock::now() < target);
> ```
> (At that point it does not matter whether it's `<` or `<=`).
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96176