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