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:
> > 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. :)
================
Comment at:
lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py:23
+
+ def check_list_output(self, command, active_plans = [], completed_plans =
[], discarded_plans = []):
+ # Check the "thread plan list" output against a list of active &
completed and discarded plans.
----------------
jingham wrote:
> labath wrote:
> > this looks like a model example for using `self.filecheck`
> I don't see that. Do you mean write a file check matching file with the
> contents of the three arrays in this function, and then run file check on
> that? Or did you mean convert the call sites into embedded patterns and then
> call file check on myself? But then I'd have to also embed the headers in
> the body such that they came in the right order for all the tests. Neither
> of these seem like attractive options to me. It doesn't seem like it will be
> hard to maintain this little checker, and I don't see what I would gain by
> using file check instead.
What I meant was doing doing something like:
```
self.filecheck("thread plan list %d"%(current_id), __file__,
"--check-prefix=CHECK1")
# CHECK1: Active plan stack:
# CHECK1-NEXT: Element 0: Base thread plan
# CHECK1-NEXT: Element 1: Stepping over line main.c
# CHECK1-EMPTY:
```
instead of `self.check_list_output(..., [], ["Stepping over line main.c"])`
The main advantage of that I see is in the error messages it produces. If
anything goes wrong, the most likely error message we're going to get is "9 !=
10, Too many elements in match arrays". The second most likely error message
is "False is not True, Didn't find active plan Stepping over line main.c". If
the same happens while using filecheck, it would print the string it is trying
to match, the line it is trying to match it against, and the line of a
"possible intended match", which makes it much easier to figure out what is
going on without debugging the test. Now you could embed all of that into this
matcher function, but then you're sort of reinventing FileCheck.
The second advantage is that it is easier to see what is the "expected" output
supposed to be when it is layout out like this, instead of embedded in the
logic of the matcher. That may introduce some amount of repetition for
specifying the common parts of the output, but that is not necessarily bad
(because it's more readable). If there is a huge amount of repetition, then
there are ways to avoid that via using multiple prefixes:
```
FileCheck --check-prefixes=ALL,FIRST
...
FileCheck --check-prefixes=ALL,SECOND
# ALL: Active plan stack:
# ALL: Element 0: Base thread plan
# FIRST: Element 1: Stepping over line main.c
# SECOND: Element 1: ???
```
But that again makes it harder to see the expected output, so it should be
balanced against that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76814/new/
https://reviews.llvm.org/D76814
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
