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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
