[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:

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

>> Lastly, this current implementation has a bug I couldn't figure out, where if we're sending l an error response, lldb loses connection to lldb-server (this happens even if the first thing I do in the handle_jAttachWait function is return an error)
>
> I'm not sure that's a bug (i.e., lldb may be intentionally dropping the connection). What would you expect lldb to do when it gets an error response?

To expand on the error, if I change `handle_vAttachWait` to immediately return an error with string "foo", lldb will print out: `error: attach failed: foo`, while if I do the same on `handle_jAttachWait` if will print out: `error: attach failed: lost connection`, so we lose the error message.

>> and, also, CTRL+C from lldb doesn't interrupt lldb-server. If anyone know why this might be happening, I'd be glad to hear it.
>
> Seems to work for me (in that I get back to the lldb prompt and the server connection is terminated), if I press ^C **twice**.

What I mean is that my new packet has these problems, and `vAttachWait` doesn't. I'm confused because I believe I implemented all the parts the same as the other `vAttach` functions.

Also, I misunderstood the bug I was having. It looks like two `^C` will interrupt my function as well, the problem is that it lags for around 5 seconds before doing so (and I was inputing a third `^C` before the interrupt, which killed lldb, and left lldb-server running), whereas the `vAttachWait` variant interrupts immediately.



================
Comment at: lldb/include/lldb/Target/Process.h:223-224
   bool m_wait_for_launch;
+  llvm::Optional<uint64_t> m_wait_for_launch_interval;
+  llvm::Optional<uint64_t> m_wait_for_launch_duration;
   bool m_ignore_existing;
----------------
labath wrote:
> clayborg wrote:
> > Should we attach _usec and sec to these names to make it clear what the units are?
> I'd make that std::chrono::(micro)seconds
I agree that using chrono would make it evident what the units are.


================
Comment at: lldb/include/lldb/Utility/StringExtractorGDBRemote.h:136
     eServerPacketType_vAttachOrWait,
+    eServerPacketType_jAttachWait,
     eServerPacketType_vAttachName,
----------------
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.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:261-273
+bool GDBRemoteCommunicationClient::GetJAttachWaitSupported() {
+  if (m_j_attach_wait_reply == eLazyBoolCalculate) {
+    m_j_attach_wait_reply = eLazyBoolNo;
+
+    StringExtractorGDBRemote response;
+    if (SendPacketAndWaitForResponse("qJAttachWaitSupported", response,
+                                     false) == PacketResult::Success) {
----------------
labath wrote:
> I'm not sure this is really useful. We could infer that the packet is not supported by the "unsupported" response to the packet itself...
Do you mean the `qJAttachWaitSupported` packet? I'm mimicking  the existing `qVAttachOrWaitSupported` packet, I think it's good to use the same pattern, no? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96176