Page MenuHomePhabricator

Update selected thread after loading mach core

Authored by JDevlieghere on Mar 6 2018, 3:00 AM.



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

Diff Detail


Event Timeline

JDevlieghere created this revision.Mar 6 2018, 3:00 AM
labath added a comment.Mar 6 2018, 3:36 AM

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.

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.

JDevlieghere planned changes to this revision.Mar 6 2018, 7:32 AM

Alright, sounds good. Thank you both for the help!

JDevlieghere retitled this revision from WIP: Update selected thread after loading mach core to Update selected thread after loading mach core.
JDevlieghere edited the summary of this revision. (Show Details)
  • Added testcase.
  • Followed Jim's advice to use WaitForProcessToStop.
  • Should now do the right thing for all types of cores.

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.

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

1–2 ↗(On Diff #137762)

It doesn't look like this is actually executable.

Thanks for the review @labath!

jingham accepted this revision.Mar 13 2018, 2:40 PM

Looks great! Thanks for working through this.

This revision is now accepted and ready to land.Mar 13 2018, 2:40 PM
jasonmolenda accepted this revision.Mar 13 2018, 4:26 PM

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?

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

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

Sounds reasonable, I'll do that in a follow-up commit!

This revision was automatically updated to reflect the committed changes.