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