mib marked 5 inline comments as done.
mib added inline comments.
================
Comment at: lldb/examples/python/scripted_process/scripted_platform.py:31
+ def list_processes(self):
+ """ Get a list of processes that can be ran on the platform.
+
----------------
labath wrote:
> mib wrote:
> > labath wrote:
> > > mib wrote:
> > > > labath wrote:
> > > > > mib wrote:
> > > > > > labath wrote:
> > > > > > > mib wrote:
> > > > > > > > mib wrote:
> > > > > > > > > mib wrote:
> > > > > > > > > > labath wrote:
> > > > > > > > > > > I am surprised that you want to go down the "run" path
> > > > > > > > > > > for this functionality. I think most of the launch
> > > > > > > > > > > functionality does not make sense for this use case
> > > > > > > > > > > (e.g., you can't provide arguments to these processes,
> > > > > > > > > > > when you "run" them, can you?), and it is not consistent
> > > > > > > > > > > with what the "process listing" functionality does for
> > > > > > > > > > > regular platforms.
> > > > > > > > > > >
> > > > > > > > > > > OTOH, the "attach" flow makes perfect sense here -- you
> > > > > > > > > > > take the pid of an existing process, attach to it, and
> > > > > > > > > > > stop it at a random point in its execution. You can't
> > > > > > > > > > > customize anything about how that process is run (because
> > > > > > > > > > > it's already running) -- all you can do is choose how you
> > > > > > > > > > > want to select the target process.
> > > > > > > > > > For now, there is no support for attaching to a scripted
> > > > > > > > > > process, because we didn't have any use for it quite yet:
> > > > > > > > > > cripted processes were mostly used for doing post-mortem
> > > > > > > > > > debugging, so we "ran" them artificially in lldb by
> > > > > > > > > > providing some launch options (the name of the class
> > > > > > > > > > managing the process and an optional user-provided
> > > > > > > > > > dictionary) through the command line or using an
> > > > > > > > > > `SBLaunchInfo` object.
> > > > > > > > > >
> > > > > > > > > > I guess I'll need to extend the `platform process
> > > > > > > > > > launch/attach` commands and `SBAttachInfo` object to also
> > > > > > > > > > support these options since they're required for the
> > > > > > > > > > scripted process instantiation.
> > > > > > > > > >
> > > > > > > > > > Note that we aren't really attaching to the real running
> > > > > > > > > > process, we're creating a scripted process that knows how
> > > > > > > > > > to read memory to mock the real process.
> > > > > > > > > @labath, I'll do that work on a follow-up patch
> > > > > > > > @labath here D139945 :)
> > > > > > > Thanks. However, are you still planning to use the launch path
> > > > > > > for your feature? Because if you're not, then I think this
> > > > > > > comment should say "Get a list of processes that **are running**"
> > > > > > > (or that **can be attached to**).
> > > > > > >
> > > > > > > And if you are, then I'd like to hear your thoughts on the
> > > > > > > discrepancy between what "launching" means for scripted and
> > > > > > > non-scripted platforms.
> > > > > > >
> > > > > > The way I see it is that the scripted platform will create a
> > > > > > process with the right process plugin. In the case of scripted
> > > > > > processes, the `ProcessLaunchInfo` argument should have the script
> > > > > > class name set (which automatically sets the process plugin name to
> > > > > > "ScriptedProcess" in the launch info). Once the process is
> > > > > > instantiated (before the launch), the scripted platform will need
> > > > > > to redirect to process stop events through its event multiplexer.
> > > > > > So the way I see it essentially, running a regular process with the
> > > > > > scripted platform should be totally transparent.
> > > > > >
> > > > > > Something that is also worth discussing IMO, is the discrepancy
> > > > > > between launching and attaching from the scripted platform:
> > > > > >
> > > > > > One possibility could be that `platform process launch` would
> > > > > > launch all the scripted processes listed by the scripted platform
> > > > > > and set them up with the multiplexer, whereas `platform process
> > > > > > attach` would just create a scripted process individually. I know
> > > > > > this doesn't match the current behavior of the platform commands so
> > > > > > if you guys think we should preserve the expected behavior, I guess.
> > > > > >
> > > > > > May be @jingham has some opinion about this ?
> > > > > Before we do that, maybe we could take a step back. Could you explain
> > > > > why you chose to use the "launch" flow for this use case?
> > > > >
> > > > > To me, it just seems confusing to be using "launching" for any of
> > > > > this, particularly given that "attaching" looks like a much better
> > > > > match for what is happening here:
> > > > > - launch allows you to specify process cmdline arguments, attach does
> > > > > not - I don't think you will be able to specify cmdline arguments for
> > > > > these scripted processes
> > > > > - launch allows you to specify env vars, attach does not -- ditto
> > > > > - launch allows you to stop-at-entry, attach does not -- you cannot
> > > > > stop at entry for these processes, as they have been started already
> > > > > - attach allows you to specify a pid, launch does not -- you (I
> > > > > think) want to be able to choose the process (pid) that you want to
> > > > > create a scripted process for
> > > > >
> > > > > For me, the choice is obvious, particularly considering that there
> > > > > *is* an obvious equivalent for "launching" for the kernel
> > > > > co-debugging use case. One could actually have the kernel create a
> > > > > new process --and then it **would** make sense to specify cmdline
> > > > > arguments, environment, and all of the other launch flags. I don't
> > > > > expect anyone to actually support this, as creating a brand new
> > > > > process like this is going to be very tricky, but one could still
> > > > > conceivably do that.
> > > > >
> > > > > Now, I don't want to be designing the feature for you, but I do have
> > > > > a feeling that building the scripted platform feature around this
> > > > > "launch-is-attach" model is going to limit its usefulness to other,
> > > > > more conventional use cases. However, if the feature you're looking
> > > > > for is "launching all processes", then I don't see a problem with
> > > > > adding something like `attach --all`, which would attach to all
> > > > > (attachable) processes. It's probably not something one would want to
> > > > > use for normal platforms very often (so we may want to implement some
> > > > > kind of a "are you sure?" dialog), but functionally that makes sense
> > > > > to me regardless of the platform.
> > > > I don't have any strong opinion for one over the other. The reason I'm
> > > > going with launch is because this is what Scripted Processes already
> > > > support. Originally, Scripted Processes were made for post-mortem
> > > > debugging, so "re-launching" the process made sense to me, instead of
> > > > attaching to a non-running process.
> > > >
> > > > Regarding passing command line arguments, env variables, etc. this
> > > > could be done using the `-k/-v` options or a structured data dictionary
> > > > in the `Process{Launch,Attach}Info`, so both cases should be covered.
> > > >
> > > > For the `attach --all` suggestion, I was thinking of something similar
> > > > and I actually like it :) That would iterate over every process on the
> > > > platform and call the attach method on it. For scripted processes, the
> > > > process attach behavior could be customized by the implementor.
> > > Well, I do have a medium-strong opinion on that. :)
> > > I believe you that you can make it work through the launch code path. The
> > > -k/-v thing is a stringly typed api, and you can pass anything through
> > > that. But I have to ask: why would you be doing that, if you already have
> > > a bespoke api to do that? My concern is two fold:
> > > - Even if "process launch" does an "attach" to userspace process and the
> > > process-to-attach is specified using -k/-v pairs, the user can still pass
> > > "launchy" arguments (cmdline, env) to that command. So you now have to
> > > either ignore them, or do extra work to make sure they are rejected
> > > - Doing it this way would mean duplicating some existing lldb
> > > functionality. The attach command already supports attach-by-pid and
> > > attach-by-name modes, and I would expect that the users would want to use
> > > that in a scripted scenario as well. If they are meant to go through the
> > > launch path, then the launch code would have to support that as well.
> > >
> > > I think that, for the post-mortem use case, the "load-core" flow (which
> > > actually uses parts of the attach code under the hood) would make more
> > > sense. I'm not sure what it would take to make that usable from a script.
> > Hi @labath, I just getting back to this, and I'm a bit confused by what
> > changes you're asking for exactly. Would you mind clarifying ? Thanks!
> Ideally, I would like you to say "I'm not going to implement the 'user/kernel
> co-debugging feature' using the 'launch' flow" :D. I don't know if you
> already decided that, but if you have, I was not aware of that.
> If you really do want to go with the launch approach, I'd appreciate some
> response my concerns above (i.e., what are you going to do if the user
> specifies arguments when "launching" these processes? Will you be duplicating
> the attach-by-name functionality?) Or just some general description of how
> will the launch look like (e.g. what arguments will the user be providing to
> launch a given process).
>
> In terms of this patch, I think the only part that's rubbing me the wrong way
> is the function description highlighted above -- specifically the "ran or"
> part. If a platform reports running processes (like normal platforms do),
> then you cannot "run" them again. I think we should drop that part and say
> "can be attached" (or simply "are running"). `help platform process list`
> says "List processes on a remote platform by name, pid, or many other
> matching attributes." and I think this should say something effectively
> equivalent to that (modulo the filtering part, as that does not happen here).
>
Done! I had misunderstood what you were asking for originally, but I actually
think it makes more sense to use `attach` instead of `launch` in the context of
user/kernel co-debugging :-) So just to recap, Scripted Platform will have both
affordances but we will use `attach` for the co-debugging part.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139250/new/
https://reviews.llvm.org/D139250
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
