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