Page MenuHomePhabricator

Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH
Needs ReviewPublic

Authored by jingham on Jan 21 2021, 12:41 PM.

Details

Summary

The header docs in SBDebugger.i suggest:

target = debugger.CreateTargetWithFileAndArch (exe, lldb.LLDB_ARCH_DEFAULT)

but that call doesn't actually produce a valid target. The equivalent API - FindTargetWithFileAndArch does work, because it uses the current platform to translate LLDB_ARCH_DEFAULT. This patch just adds that same step to CreateTargetWithFileAndArch, and adds a test.

This API was untested so I also added a test for this one and the similar CreateTargetWithFileAndTargetTriple.

I don't see anywhere where we say what the difference between an "Arch" and a "TargetTriple" is. If anybody has some good verbiage for that, I'll add it to the docs for the API.

Diff Detail

Event Timeline

jingham requested review of this revision.Jan 21 2021, 12:41 PM
jingham created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 12:41 PM

I think the only difference is the triple is supposed to be a complete triple and the arch can be just "arm64" and like you added, we let the currently selected platform help fill it out? I don't remember why this was added. Nothing in the source history?

jingham added a comment.EditedJan 21 2021, 2:56 PM

I think the only difference is the triple is supposed to be a complete triple and the arch can be just "arm64" and like you added, we let the currently selected platform help fill it out? I don't remember why this was added. Nothing in the source history?

The TargetTriple doesn't seem to need to be a full triple:

>>> target = lldb.debugger.CreateTargetWithFileAndTargetTriple("/tmp/foo", "x86_64")
>>> target.GetTriple()
'x86_64-apple-macosx11.0.0'

Looking at the FindTargetWithFileAndArch, it looks like the only difference is we call the Platform::GetAugmentedArchSpec in the Arch case, and that translates some symbolic constants (systemArch, systemArch32, systemArch64).

This code was from way before the dread reformatting and I don't feel like spelunking past that today...

This revision was not accepted when it landed; it landed in state Needs Review.Jan 25 2021, 12:54 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
rsmith added a subscriber: rsmith.Jan 25 2021, 6:06 PM

I reverted this and the follow-up fix in rG8b1171488575bd0b5ccb23bc1a3d22e2aaccb244, because it's introduced several test failures. Example: http://lab.llvm.org:8011/#/builders/96/builds/3777

It appears that these tests use CreateTargetWithFileAndArch(None, None), which worked before these changes and not afterwards.

MaskRay reopened this revision.Jan 28 2021, 9:28 PM