labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.
================
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.
+
----------------
mib wrote:
> 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.
>
>
Awesome. I'm glad we're in agreement. Thanks.
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
