[Lldb-commits] [PATCH] D76814: Preserve ThreadPlanStacks for unreported threads

Thu Apr 2 02:41:03 PDT 2020

labath added inline comments.

================
Comment at: lldb/source/Target/TargetProperties.td:183
     Desc<"A path to a python OS plug-in module file that contains a OperatingSystemPlugIn class.">;
+  def PluginReportsAllThreads: Property<"plugin-reports-all-threads", "Boolean">,
+    Global,
----------------
jingham wrote:
> labath wrote:
> > jingham wrote:
> > > labath wrote:
> > > > jingham wrote:
> > > > > labath wrote:
> > > > > > If this is relevant only for os plugins, then it would be good to reflect that in the name as well.
> > > > > I thought about that.  In the future a regular Process plugin might decide it was too expensive to report all threads as well.  There's nothing in this patch that wouldn't "just work" with that case as well.  Leaving the OS out was meant to indicate this was about how the Process plugin OR any of its helpers (e.g. the OS Plugin) produces threads.
> > > > Well, I am hoping that if we ever extend this support to the regular process plugins, we will implement it in a way where the plugin itself can tell us whether it is operating/supporting this mode (which I guess would involve a new gdb-remote packet and some specification of what exactly should happen when it gets sent), instead of relying on the user to set this correctly.
> > > > 
> > > > I mean, in an ideal world this is I would want to happen with the python plugins as well, but it seems that we are stuck with some existing plugins which already do that. However, I wouldn't want to plan on the same thing happening again. :)
> > > Right, I put this in mostly to help backwards compatibility.  For instance, another OS Plugin I know about handles some older cooperative threading scheme.  That one does report all threads on each stop.  I didn't want to force them to do anything to keep their plugin working as well as it did before.  That's also why I set the default to true here.
> > > 
> > > Even when we have plugins that actually support not reporting all threads, you could imagine somebody having a Process plugin that supports both modes - particularly early in the development of its support for this feature, and in some corner cases the "doesn't report all threads" mode has some subtle problem.  Having this setting will allow people to get the slower but more assuredly correct behavior till it works 100% reliably.  So I still think the setting has some value.
> > > 
> > > But I agree, going forward there should be some kind of handshake between the ThreadPlanStackMap and the Process Plugin, either a "I've reported all threads now" which could trigger a prune, or a "Is TID X still alive" which the generic code could use to balance the cost of keeping outdated stacks alive against when we want to ask about all threads.
> > All of this sounds fine, and I wouldn't mind having a setting like that, even after the support for partial thread lists is considered "stable". However, that sounds like a different setting to me -- that's like a directive to the process plugin about how it should behave, whereas this setting is more like a statement of fact about what the plugin does.
> > 
> > The setting could be later repurposed to do both, but it's not clear to me whether that is useful. Like, since we already support plugin-specific settings, the plugin which decides to implement both modes of operation could expose that as a custom setting. That way, one wouldn't get the impression that this setting applies to any process plugin...
> Okay.  I don't want to have to design that right now.  
> 
> Since this is just a workaround for the fact that the extant OS plugins don't have a way to tell me this fact, I'll go back to using this just for OS plugins.  In that case, I think I should move it to experimental.  Once we add an API to the various plugins to advertise how they work, it won't be needed.  The benefit of experimental settings is that their absence doesn't cause the "settings set" to raise an error, so people can leave this in their lldbinit's and it won't cause problems once we use it.
> 
> Now I just have to figure out how experimental settings work in the new tablegen way of doing properties...
An experimental setting sounds good to me.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76814/new/

https://reviews.llvm.org/D76814