This is an archive of the discontinued LLVM Phabricator instance.

A better initializing the OSType in elf object file's arch_spec
ClosedPublic

Authored by trixirt on Apr 27 2015, 7:23 AM.

Details

Summary

Setting the OSType in the ArchSpec triple is needed to correctly setup up
the register context plugin. ArchSpec::SetArchitecture, for Mach-O only, sets
the OSType. For ELF it was left to the ObjectFileELF to fill in the missing
OSType.

This change moves the ObjectFileELF logic into ArchSpec.

A new optional 'os' parameter has been added to SetArchitecture.
For ELF, this value is the from the ELF header.e_ident[EI_OSABI].
The default value is 0 or ELFOSABI_NONE.

The real work of determining the OSType was done by the ObjectFileELF
helper function GetOsFromOSABI. This logic has been moved SetArchitecture.

GetOsFromOSABI has been commented as being deprectated.
It is left in to support asserts.

For ELF the vendor value returned from SetArchitecture should be UnknownVendor.
An unneeded resetting in ObjectFileELF has been removed and replaced with
an assert.

This fixes a problem reading a core file on FreeBSD/ARM because the spec triple
was arm-unknown-unknown

Diff Detail

Repository
rL LLVM

Event Timeline

trixirt updated this revision to Diff 24473.Apr 27 2015, 7:23 AM
trixirt retitled this revision from to A better initializing the OSType in elf object file's arch_spec.
trixirt updated this object.
trixirt edited the test plan for this revision. (Show Details)
trixirt added reviewers: clayborg, emaste.
trixirt set the repository for this revision to rL LLVM.
trixirt added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Apr 27 2015, 10:11 AM
clayborg edited edge metadata.

See inlined comments.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1400 ↗(On Diff #24473)

After this line can you make sure that if the OS or vendor is unknown that it is left as an unspecified unknown?

assert(arch_spec.GetTriple().getOS() != llvm::Triple::UnknownOS || arch_spec.TripleOSWasSpecified() == false);
assert(arch_spec.GetTriple().getVendor() != llvm::Triple::UnknownVendor || arch_spec.TripleVendorWasSpecified() == false);

If the above assert asserts when you run this, you will need to do one of:

arch_spec.GetTriple().setOSName(llvm::StringRef());
arch_spec.GetTriple().setVendorName(llvm::StringRef());

This will clear the name, but leave the enum value as UnknownOS or UnknownVendor.

This will make sure that any "unknown" values are said to be "unspecified unknowns" so that the arch will effectively be one of

<arch>-*-*
<arch>-*-<os>

If the triple contains "unknown" as the string value in their triple, plug-ins won't be able to wildcard match the ELF file when needed. See the implementations of ArchSpec::TripleVendorWasSpecified() and ArchSpec::TripleOSWasSpecified() to see how we do this, but a quick sum is if the triple has a enum value of unknown and no string value, it is a "unspecified unknown" and if the enum is unknown and the string value is "unknown" then is is a "specified unknown". Some plug-ins will match the specified unknown values and also allow wildcard matching when the unknown isn't specified. It would be nice to eventually have llvm::Triple support for "none" for both the OS and vendor so we can specifiy "no OS" and "no vendor" for things like baseboard binaries, but right now we use "specified unknown" as the marker for "no OS" and "no vendor".

This revision now requires changes to proceed.Apr 27 2015, 10:11 AM

I believe that an 'unknown' vendor is normal for some triples.
For example i386-unknown-freebsd10.1

Tom

Ok, just make sure that the unknown is wanted. I would say that GetOsFromOSABI() should be renamed to UpdateOSAndVendorFromOSABI() and it should take the ArchSpec and set the vendor and OS correctly. Specified unknowns are OK when they are desired, but if this function doesn't recognize the OSABI and it doesn't set anything, they should be left and unspecified unknowns.

emaste edited edge metadata.Apr 27 2015, 11:58 AM

See also https://svnweb.freebsd.org/base?view=revision&revision=278909 for a workaround in the FreeBSD tree for ELF core file arch setting

It seems like GetOsFromOSABI's purpose is to compensate for
ArchSpec::SetArchitecture. I think it would be better to improve
SetArchitecture. I will post a change showing how this could be done
shorty.
Tom

trixirt updated this revision to Diff 24623.Apr 29 2015, 6:11 AM
trixirt updated this object.
trixirt edited the test plan for this revision. (Show Details)
trixirt edited edge metadata.

Move ObjectFileELF::GetOsFromOSABI functionality to ArchSpec::SetArchitecture

clayborg accepted this revision.Apr 29 2015, 9:41 AM
clayborg edited edge metadata.

Looks fine.

This revision is now accepted and ready to land.Apr 29 2015, 9:41 AM
This revision was automatically updated to reflect the committed changes.