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

[Pavel Labath via Phabricator via lldb-commits]

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