Page MenuHomePhabricator

Implement jAttachWait
Needs ReviewPublic

Authored by augusto2112 on Feb 5 2021, 1:24 PM.

Details

Summary

jAttachWait provides the same functionality as vAttachWait/vAttachOrWait. Additionaly, it allows for the usage of two new options, 'waitfor-duration' and 'waitfor-interval'

Diff Detail

Event Timeline

augusto2112 created this revision.Feb 5 2021, 1:24 PM
augusto2112 requested review of this revision.Feb 5 2021, 1:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 1:24 PM

As discussed in the vAttachWait patch (https://reviews.llvm.org/D93895), I've implemented a jAttachWait packet with supports two additional parameters (polling interval and polling duration) when attaching to a process by name and waiting for it to appear.

jAttachWait send a json packet of the format: {"process_name": string, "include-existing: bool, "waitfor-interval-usec": microseconds, waitfor-duration-sec: seconds} where waitfor-interval-usec and waitfor-duration-sec are only sent if specified by the user.

For this first implementation I decided to add the arguments as additional flags to process attach, but I can change this if you think that's not a good idea.

I was also wondering if this could/should be expanded in a more general jAttach packet, where the packet contents would specify if lldb-server should wait or not, what are your thoughts on that?

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

See inlined comment about the packet name issue and let me know what you think

lldb/include/lldb/Target/Process.h
180–181
221–222

Should we attach _usec and sec to these names to make it clear what the units are?

lldb/include/lldb/Utility/StringExtractorGDBRemote.h
136

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.

labath added a comment.Feb 6 2021, 5:20 AM

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
lldb/include/lldb/Target/Process.h
113–114

Just delete this, that's the default. If you really want to be explicit, you can put = llvm::None in the member declaration.

221–222

I'd make that std::chrono::(micro)seconds

lldb/include/lldb/Utility/StringExtractorGDBRemote.h
136

Sounds like a good idea.

lldb/include/lldb/lldb-enumerations.h
600–601

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

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
261–273

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

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
388

I think this ought to be steady_clock

394

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

3377–3378

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.

lldb/include/lldb/Target/Process.h
221–222

I agree that using chrono would make it evident what the units are.

lldb/include/lldb/Utility/StringExtractorGDBRemote.h
136

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.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
261–273

Do you mean the qJAttachWaitSupported packet? I'm mimicking the existing qVAttachOrWaitSupported packet, I think it's good to use the same pattern, no?

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

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.

Hm.. I don't know.. I guess you'll have to step through the code to find where it diverges...

augusto2112 marked 7 inline comments as done and an inline comment as not done.

Address comments

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

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.

lldb/include/lldb/Utility/StringExtractorGDBRemote.h
136

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.

lldb/include/lldb/lldb-enumerations.h
600–601

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">,
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
394

Good catch!