This is an archive of the discontinued LLVM Phabricator instance.

Fix load address resolution on Windows
ClosedPublic

Authored by zturner on Jan 21 2015, 10:08 PM.

Details

Summary

When loading a module as a result of the "target create" command, there is the potential for ObjectFilePECOFF to stomp the triple. This can confuse later code which depends on the triple. Ultimately this fixes load address resolution on Windows because the correct triple is plumbed all the way through, so DynamicLoaderWindows can now correctly recognize that it should be used.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 18586.Jan 21 2015, 10:08 PM
zturner retitled this revision from to Fix load address resolution on Windows.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: clayborg, jingham.
zturner added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Jan 22 2015, 9:31 AM
clayborg edited edge metadata.

Close. A couple changed as annotated in the source. There are two categories of unknown:

  • specified "unknown" for the vendor or OS where the user actually typed "unknown" in the triple
  • unspecified "unknown" for the vendor or OS where the user didn't specify it.

Add the quick fixes I specify above and this patch is good to go.

source/Core/Module.cpp
1314 ↗(On Diff #18586)

We need to make sure the vendor wasn't specified as "unknown" by also calling:

bool ArchSpec::TripleVendorWasSpecified(). So the above line should be:

if (m_arch.GetTriple().getVendor() == llvm::Triple::UnknownVendor && m_arch.TripleVendorWasSpecified())
1316 ↗(On Diff #18586)

Save thing as the previous, make sure "unknown" wasn't specified:

if (m_arch.GetTriple().getOS() == llvm::Triple::UnknownOS && m_arch.TripleOSWasSpecified())
This revision now requires changes to proceed.Jan 22 2015, 9:31 AM

Just to be clear, we should be checking that the triple vendor was NOT specified right? The code you wrote checks that it was specified.

Would it be worthwhile to make a function called ArchSpec::MergeFrom(const ArchSpec &other), which basically does what I've done here (with a comment explaining how it works)?

zturner updated this revision to Diff 18624.Jan 22 2015, 10:19 AM
zturner edited edge metadata.

Fixed suggested issues, and moved the logic into a function in ArchSpec, since I thought it might be useful.

I reversed your conditional though because it looked like a thinko.

LMK if this looks good.

clayborg accepted this revision.Jan 22 2015, 10:45 AM
clayborg edited edge metadata.

Looks good

This revision is now accepted and ready to land.Jan 22 2015, 10:45 AM
This revision was automatically updated to reflect the committed changes.