Page MenuHomePhabricator

Preserve ThreadPlanStacks for unreported threads
ClosedPublic

Authored by jingham on Mar 25 2020, 5:27 PM.

Details

Summary

This is the final patch in the work I've done so far to support Process plugins (or their associated OS Plugins) not reporting all threads on every stop.

This commit moves the thread plan updating and dumping into the ThreadPlanStackMap. It adds a setting (target.process.plugin-reports-all-threads) you can use to indicate that you want us to preserve unreported thread plans, and a "thread plan prune" command you can use to remove any orphaned plans.

If this is approved, I'd like to check in all these pieces. At this stage if you opt into preserving ThreadPlanStacks, the process is pretty minimal. I want to get this more by-hand version working because there are a lot of OS Plugins in the wild (there's on in every mach_kernel dSYM) and we have not supported them well. So I want to get something that supports the extant plugins.

The next step is to add a way for the plugins to sync up with the thread plans, and then call that at public stop time, but I'd like to do that as a separate patch.

Diff Detail

Event Timeline

jingham created this revision.Mar 25 2020, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 5:27 PM

I still don't know much about thread plans, but I didn't see anything that would stand out. All my comments are fairly superficial.

lldb/source/Target/TargetProperties.td
187

If this is relevant only for os plugins, then it would be good to reflect that in the name as well.

lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp
2–8

We don't put licence headers on tests.

21

unistd.h is not portable, and it doesn't look like you are using anything from it.

34

The synchronization here seems to rely on the fact that there will be no spurious wakeups. The simplest way to guard against that is to pass a lambda into the wait function g_cv.wait(func_lock, [&] { return g_value == 2; })

39–42

The clang/gcc/icc anotations are not used, and who knows if they are even accurate at this point.

lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
24

this looks like a model example for using self.filecheck

jingham updated this revision to Diff 252982.Mar 26 2020, 2:33 PM
jingham marked 2 inline comments as done.

Addressed most of Pavel's review comments.

Addressed most of the review comments. I don't see that using file check would make the plan output checking any easier. The current function seems clear and easy to use. Trying to dynamically insert the passed in matches into a filecheck test file and then run file check on that doesn't seem like it would be any easier than what is here. I don't need any of the substitutions or any of the other fancy things FileCheck provides, that just seems like overkill. But maybe I'm misunderstanding what you meant.

lldb/source/Target/TargetProperties.td
187

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.

lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
24

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.

labath added inline comments.Mar 27 2020, 2:28 AM
lldb/source/Target/TargetProperties.td
187

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

lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
24

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.

jingham marked 2 inline comments as done.Mar 27 2020, 10:52 AM
jingham added inline comments.
lldb/source/Target/TargetProperties.td
187

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.

lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
24

Yeah, if you don't mind, I'm going to keep it as it is. To do it the way you suggest you either would have to repeat all the header stuff over and over or make some compound section for the filecheck part of the test with FIRST and SECOND etc for the tests. That's sort of okay for a few cases but if you have more that gets hard to follow and edit without making mistakes.

The way it is now, you say explicitly what you want to see in the the categories at the check site, which seems really clear to me. And the processing code is not terribly difficult, so I don't think maintaining it will be a problem.

labath added inline comments.Mar 30 2020, 6:42 AM
lldb/source/Target/TargetProperties.td
187

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

lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp
41

spurious wake up danger here too

lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
24

Well, I wouldn't say I don't mind, but I am not interested in making a protracted debate out of this. But if you are going to stick to this, then I think you should do something to make the error messages coming out of here more reasonable. The least you could do is use self.runCmd instead of handle command (so that the command output is visible in trace mode at least), and replace all assertTrue(needle in haystack) with assertIn(needle, haystack) (to make it easier to see what is being matched against what).

jingham updated this revision to Diff 254372.Apr 1 2020, 5:16 PM

Add a variable backing the std::condition_variable in the source for the stepping test to catch spurious wakeups.

AssertTrue -> AssertIn in the ThreadPlanList test.

jingham marked 5 inline comments as done.Apr 1 2020, 5:39 PM
jingham added inline comments.
lldb/source/Target/TargetProperties.td
187

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

lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/main.cpp
41

g_value had a different purpose and using it more widely as the variable backing the condition_variable was awkward, so I added an explicit condition variable.

lldb/test/API/functionalities/thread_plan/TestThreadPlanCommands.py
24

I should have guessed there was an assertIn as well! Converted over to use that.

I didn't use runCmd since I wasn't using any of its facilities besides dumping the command result. But I added a check for self.TraceOn and if true I dump the command output. So that should do what you wanted.

labath added inline comments.Apr 2 2020, 2:32 AM
lldb/source/Target/TargetProperties.td
187

An experimental setting sounds good to me.

jingham updated this revision to Diff 254664.Apr 2 2020, 6:13 PM

I changed target.process.plugin-reports-all-threads to target.experimental.os-plugin-reports-all-threads.

jingham marked an inline comment as done.Apr 2 2020, 6:14 PM
labath accepted this revision.Apr 3 2020, 12:28 AM

Looks fine to me, though I can't say I'm much of a thread plan expert (but then again, I don't know if anyone except you is).

This revision is now accepted and ready to land.Apr 3 2020, 12:28 AM

Looks fine to me, though I can't say I'm much of a thread plan expert (but then again, I don't know if anyone except you is).

That's not a great situation, but it is where we currently are...

This revision was automatically updated to reflect the committed changes.