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