labath added a comment.
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. One
alternative could be to just tack on some extra data to the existing vAttach
family packets (`vAttachWait;foo;interval:47;duration=74`).
> 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?
> 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**.
$ bin/lldb
(lldb) process attach --name asdgoijhpweiogjawe -w
^C^C
... Interrupted.
(lldb) ^D
================
Comment at: lldb/include/lldb/Target/Process.h:113-114
m_plugin_name(), m_resume_count(0), m_wait_for_launch(false),
+ m_wait_for_launch_interval(llvm::Optional<uint64_t>()),
+ m_wait_for_launch_duration(llvm::Optional<uint64_t>()),
m_ignore_existing(true), m_continue_once_attached(false),
----------------
Just delete this, that's the default. If you really want to be explicit, you
can put ` = llvm::None` in the member declaration.
================
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;
----------------
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
================
Comment at: lldb/include/lldb/Utility/StringExtractorGDBRemote.h:136
eServerPacketType_vAttachOrWait,
+ eServerPacketType_jAttachWait,
eServerPacketType_vAttachName,
----------------
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.
================
Comment at: lldb/include/lldb/lldb-enumerations.h:600-601
eArgTypeModuleUUID,
+ eArgTypeWaitforInterval,
+ eArgTypeWaitforDuration,
eArgTypeLastArg // Always keep this entry as the last entry in this
----------------
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....
================
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) {
----------------
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...
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:388
+ auto now = std::chrono::system_clock::now();
+ auto target = now;
----------------
I think this ought to be `steady_clock`
================
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();
----------------
`<= 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 `<=`).
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3379-3380
+ StringRef process_name;
+ auto polling_interval = llvm::Optional<uint64_t>();
+ auto polling_duration = llvm::Optional<uint64_t>();
+ bool include_existing = false;
----------------
This is not an approved use of `auto`:
<https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96176/new/
https://reviews.llvm.org/D96176
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
