Page MenuHomePhabricator

[debugserver] Fix LC_BUILD_VERSION load command handling.

Authored by friss on Apr 4 2018, 4:05 PM.



In one of the 2 places the LC_BUILD_VERSION load command is handled, there
is a bug preventing us from actually handling them (the address where to
read the load command was not updated). This patch factors reading the
deployment target load commands into a helper and adds testing for the 2
code paths calling the helper.

The testing is a llittle bit complicated because the only times those load
commands matter is when debugging a simulator process. I added a new
decorator to check that a specific SDK is available. The actual testing was
fairly easy once I knew how to run a simulated process.

Adding Pavel as a reviewer given I slightly modified the lldb-server test

Event Timeline

friss created this revision.Apr 4 2018, 4:05 PM
labath added a comment.Apr 5 2018, 2:49 AM

I thing the changes are fine. The only part that worries me is the in-class initialization of the simulator variables. I think this will fail on non-apple hosts.

16–18 ↗(On Diff #141083)

I think these will cause a problem as they will be executed even if the test is skipped. Will this throw an exception if "xcrun" fails (e.g. because you're on linux). It might be best to move this into some function..

113 ↗(On Diff #141083)

If you make this a list (['watchos']), you won't have to add the weird-looking comma at the end.

50 ↗(On Diff #141083)

I don't think this needs to be a thread command, as the pid is not thread-specific. We could just make this a regular command like print-message et al.

friss added inline comments.Apr 5 2018, 8:33 AM
16–18 ↗(On Diff #141083)

Yep, I coincidentally realized this on my commute this morning. I'll fix it.

50 ↗(On Diff #141083)

No problem.

jasonmolenda accepted this revision.Apr 5 2018, 4:36 PM

Looks good to me.

359 ↗(On Diff #141083)

It might be helpful for anyone debugging this if examples of the names this is matching were included in a comment. iphonesimulator, appletvsimulator, watchsimulator. Different capitalization is used in so many places (and whether to include "os" suffix or not, and whether to include "apple" prefix or not) that it can be hard to know what's expected unless you run the command yourself.

584 ↗(On Diff #141083)

Not important, but we're requiring a MacOS SDK of 10.11 or newer and the tvos and watchos SDKs were available by then; we could remove these ifdefs. (LC_BUILD_VERSION is newer than that & needs to be kept around.)

588 ↗(On Diff #141083)

It's equivalent, no big deal, but this is a bitwise | not a logical ||.

This revision is now accepted and ready to land.Apr 5 2018, 4:36 PM
This revision was automatically updated to reflect the committed changes.