The OS plugins might have updated the thread list after a core file has
been loaded. The physical thread in the core file may no longer be the
one that should be selected. Hence we should run the thread
selection logic after loading the core, which is part of
WaitForProcessToStop.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
I don't know much about OS plugins, but I don't see a reason why this would be mach-specific (so yes, it should probably be done for elf cores as well). Oh, and we also have windows minidumps, but I'm not sure if it should be up to you to update all of these (I'm saying not sure, because the fact that each format needs to be updated specifically may mean that this should be actually done at a different level, but I have no idea if that's possible).
As for testing, the only approach I can think of is a combination of a checked-in core file + a fake os plugin.
We currently don't have a good way of generating core files (at least for elf, it requires some additional features in yaml2obj). We have a couple of tiny (dozens of KB) elf core files checked in, and you may be even able to reuse one of them. For an example of a fake OS plugin, you can look at TestPythonOSPlugin.
I don't guarantee this will actually work (did I say I don't know OS plugins ? :D), but it's the only solution I can think of.
Comment Actions
Change looks fine to me.
What's the best/easiest way to test this?
You can just run:
(lldb) process save-core /tmp/test.core"
This will allow you to take any process and save a core file for it from lldb. Then you could obj2yaml it and check that part in. This is only implemented for mach-o right now. But at least that is a way to test something. Like Pavel stated, write a simple OS plug-in that adds a thread with a stop reason and have the test auto select it. Probably best to test that a core file with a stop reason also still selects the thread it used to when there is no OS plug-in to ensure we don't break anything.
Should we also do this for ELF cores?
We can.
Comment Actions
- Added testcase.
- Followed Jim's advice to use WaitForProcessToStop.
- Should now do the right thing for all types of cores.
Comment Actions
I'll leave the cpp change for others to approve (though it certainly looks simpler than the previous one). I just have a couple of drive-by comments on the test.
| packages/Python/lldbsuite/test/functionalities/postmortem/mach-core/TestMachCore.py | ||
|---|---|---|
| 32 ↗ | (On Diff #137762) | this should be getBuildArtifact("test.core") , so it does not end up in the test tree (we may also consider changing the interface of this function to make it harder to get wrong). |
| packages/Python/lldbsuite/test/functionalities/postmortem/mach-core/operating_system.py | ||
| 1–2 ↗ | (On Diff #137762) | It doesn't look like this is actually executable. |
Comment Actions
Looks good, thanks for doing this Jonas. I thought there would be a problem with ThreadMachCore::CalculateStopInfo() in source/Plugins/Process/mach-core/ThreadMachCore.cpp which will mark every thread that comes from the actual core file has having a stop reason -- won't that conflict with your OperatingSystemPlugIn which is providing a stop reason?
Comment Actions
(or rather, not "conflict with the OperatingSystemPlugIn stop reason" -- but would make it confusing to users. I think it might be best to remove ThreadMachCore::CalculateStopInfo.)