[PATCH] [Object] Search for architecures by name in MachOUniversalBinary::getObjectForArch()

[Frédéric Riss]

> On Jun 22, 2015, at 11:11 AM, Justin Bogner <mail at justinbogner.com> wrote:
> 
> Frederic Riss <friss at apple.com> writes:
>> Hi samsonov, bogner,
>> 
>> The reason we need to search by name rather than by Triple::ArchType
>> is to handle x86_64h correclty. There is no different ArchType
>> for the x86_64h architecture (it identifies itself as x86_64), and
>> this is by design. The only way to get to the x86_64h slice of an
>> universal binary is to serch by name.
> 
> So, this affects more than just x86_64h - it also applies to the armv7
> variants and generally on anything with subtargets. I'd be happier if we
> didn't have to resort to passing strings around here, but this does seem
> to be the way it's done elsewhere.

Yes. For good measure I added some arm variants in the test case I committed.
This is r240339.

> I have a couple of comments and then this LGTM.
> 
>> This issue led to hard to debug and transient symbolication failures
>> in Asan tests (it mostly works, because the files are very similar).
>> 
>> This also affects the Profiling infrastucture as it is the other user
>> of that API.
>> 
>> http://reviews.llvm.org/D10604
>> 
>> Files:
>>  include/llvm/Object/MachOUniversal.h
>>  include/llvm/ProfileData/CoverageMapping.h
>>  include/llvm/ProfileData/CoverageMappingReader.h
>>  lib/Object/MachOUniversal.cpp
>>  lib/ProfileData/CoverageMapping.cpp
>>  lib/ProfileData/CoverageMappingReader.cpp
>>  test/tools/llvm-symbolizer/Inputs/fat.x86_64h.c
>>  test/tools/llvm-symbolizer/Inputs/fat.x86_64h.o
>>  test/tools/llvm-symbolizer/fat.test
>>  tools/llvm-cov/CodeCoverage.cpp
>>  tools/llvm-symbolizer/LLVMSymbolize.cpp
>> 
>> EMAIL PREFERENCES
>>  http://reviews.llvm.org/settings/panel/emailpreferences/
>> Index: include/llvm/Object/MachOUniversal.h
>> ===================================================================
>> --- include/llvm/Object/MachOUniversal.h
>> +++ include/llvm/Object/MachOUniversal.h
>> @@ -109,7 +109,7 @@
>>   }
>> 
>>   ErrorOr<std::unique_ptr<MachOObjectFile>>
>> -  getObjectForArch(Triple::ArchType Arch) const;
>> +  getObjectForArch(StringRef ArchName) const;
>> };
>> 
>> } // namespace object
>> Index: include/llvm/ProfileData/CoverageMapping.h
>> ===================================================================
>> --- include/llvm/ProfileData/CoverageMapping.h
>> +++ include/llvm/ProfileData/CoverageMapping.h
>> @@ -410,7 +410,7 @@
>>   /// \brief Load the coverage mapping from the given files.
>>   static ErrorOr<std::unique_ptr<CoverageMapping>>
>>   load(StringRef ObjectFilename, StringRef ProfileFilename,
>> -       Triple::ArchType Arch = Triple::ArchType::UnknownArch);
>> +       StringRef Arch = StringRef());
>> 
>>   /// \brief The number of functions that couldn't have their profiles mapped.
>>   ///
>> Index: include/llvm/ProfileData/CoverageMappingReader.h
>> ===================================================================
>> --- include/llvm/ProfileData/CoverageMappingReader.h
>> +++ include/llvm/ProfileData/CoverageMappingReader.h
>> @@ -171,7 +171,7 @@
>> public:
>>   static ErrorOr<std::unique_ptr<BinaryCoverageReader>>
>>   create(std::unique_ptr<MemoryBuffer> &ObjectBuffer,
>> -         Triple::ArchType Arch = Triple::ArchType::UnknownArch);
>> +         StringRef Arch);
>> 
>>   std::error_code readNextRecord(CoverageMappingRecord &Record) override;
>> };
>> Index: lib/Object/MachOUniversal.cpp
>> ===================================================================
>> --- lib/Object/MachOUniversal.cpp
>> +++ lib/Object/MachOUniversal.cpp
>> @@ -123,25 +123,10 @@
>>   ec = std::error_code();
>> }
>> 
>> -static bool getCTMForArch(Triple::ArchType Arch, MachO::CPUType &CTM) {
>> -  switch (Arch) {
>> -    case Triple::x86:    CTM = MachO::CPU_TYPE_I386; return true;
>> -    case Triple::x86_64: CTM = MachO::CPU_TYPE_X86_64; return true;
>> -    case Triple::arm:    CTM = MachO::CPU_TYPE_ARM; return true;
>> -    case Triple::sparc:  CTM = MachO::CPU_TYPE_SPARC; return true;
>> -    case Triple::ppc:    CTM = MachO::CPU_TYPE_POWERPC; return true;
>> -    case Triple::ppc64:  CTM = MachO::CPU_TYPE_POWERPC64; return true;
>> -    default: return false;
>> -  }
>> -}
>> -
>> ErrorOr<std::unique_ptr<MachOObjectFile>>
>> -MachOUniversalBinary::getObjectForArch(Triple::ArchType Arch) const {
>> -  MachO::CPUType CTM;
>> -  if (!getCTMForArch(Arch, CTM))
>> -    return object_error::arch_not_found;
>> +MachOUniversalBinary::getObjectForArch(StringRef ArchName) const {
> 
> We should check for `Triple(Arch).getArch() == Triple::ArchType::UnknownArch`
> here and return an error.

Done.

>>   for (object_iterator I = begin_objects(), E = end_objects(); I != E; ++I) {
>> -    if (I->getCPUType() == static_cast<uint32_t>(CTM))
>> +    if (I->getArchTypeName() == ArchName)
>>       return I->getAsObjectFile();
>>   }
>>   return object_error::arch_not_found;
>> Index: lib/ProfileData/CoverageMapping.cpp
>> ===================================================================
>> --- lib/ProfileData/CoverageMapping.cpp
>> +++ lib/ProfileData/CoverageMapping.cpp
>> @@ -236,7 +236,7 @@
>> 
>> ErrorOr<std::unique_ptr<CoverageMapping>>
>> CoverageMapping::load(StringRef ObjectFilename, StringRef ProfileFilename,
>> -                      Triple::ArchType Arch) {
>> +                      StringRef Arch) {
>>   auto CounterMappingBuff = MemoryBuffer::getFileOrSTDIN(ObjectFilename);
>>   if (std::error_code EC = CounterMappingBuff.getError())
>>     return EC;
>> Index: lib/ProfileData/CoverageMappingReader.cpp
>> ===================================================================
>> --- lib/ProfileData/CoverageMappingReader.cpp
>> +++ lib/ProfileData/CoverageMappingReader.cpp
>> @@ -448,7 +448,7 @@
>>                                         StringRef &CoverageMapping,
>>                                         uint8_t &BytesInAddress,
>>                                         support::endianness &Endian,
>> -                                        Triple::ArchType Arch) {
>> +                                        StringRef Arch) {
>>   auto BinOrErr = object::createBinary(ObjectBuffer);
>>   if (std::error_code EC = BinOrErr.getError())
>>     return EC;
>> @@ -465,7 +465,7 @@
>>     // For any other object file, upcast and take ownership.
>>     OF.reset(cast<object::ObjectFile>(Bin.release()));
>>     // If we've asked for a particular arch, make sure they match.
>> -    if (Arch != Triple::ArchType::UnknownArch && OF->getArch() != Arch)
>> +    if (!Arch.empty() && OF->getArch() != Triple(Arch).getArch())
>>       return object_error::arch_not_found;
>>   } else
>>     // We can only handle object files.
>> @@ -495,7 +495,7 @@
>> 
>> ErrorOr<std::unique_ptr<BinaryCoverageReader>>
>> BinaryCoverageReader::create(std::unique_ptr<MemoryBuffer> &ObjectBuffer,
>> -                             Triple::ArchType Arch) {
>> +                             StringRef Arch) {
>>   std::unique_ptr<BinaryCoverageReader> Reader(new BinaryCoverageReader());
>> 
>>   SectionData Profile;
>> Index: test/tools/llvm-symbolizer/Inputs/fat.x86_64h.c
>> ===================================================================
>> --- /dev/null
>> +++ test/tools/llvm-symbolizer/Inputs/fat.x86_64h.c
>> @@ -0,0 +1,9 @@
>> +/* Compile with:
>> +   $ clang -arch x86_64 -arch x86_64h fat.x86_64h.c -c
>> +*/
>> +
>> +#ifdef __x86_64h__
>> +void x86_64h_function() {}
>> +#else
>> +void x86_64_function() {}
>> +#endif
>> Index: test/tools/llvm-symbolizer/fat.test
>> ===================================================================
>> --- /dev/null
>> +++ test/tools/llvm-symbolizer/fat.test
>> @@ -0,0 +1,8 @@
>> +RUN: echo 0 | llvm-symbolizer -obj=%p/Inputs/fat.x86_64h.o -default-arch=x86_64 | FileCheck --check-prefix=X86_64 %s
>> +RUN: echo 0 | llvm-symbolizer -obj=%p/Inputs/fat.x86_64h.o -default-arch=x86_64h | FileCheck --check-prefix=X86_64H %s
>> +
>> +X86_64-NOT: x86_64h_function
>> +X86_64: x86_64_function
>> +
>> +X86_64H-NOT: x86_64_function
>> +X86_64H: x86_64h_function
>> Index: tools/llvm-cov/CodeCoverage.cpp
>> ===================================================================
>> --- tools/llvm-cov/CodeCoverage.cpp
>> +++ tools/llvm-cov/CodeCoverage.cpp
>> @@ -89,7 +89,7 @@
>>       LoadedSourceFiles;
>>   bool CompareFilenamesOnly;
>>   StringMap<std::string> RemappedFilenames;
>> -  llvm::Triple::ArchType CoverageArch;
>> +  std::string CoverageArch;
> 
> We don't need this variable...
> 
>> };
>> }
>> 
>> @@ -349,15 +349,12 @@
>>       Filters.push_back(std::unique_ptr<CoverageFilter>(StatFilterer));
>>     }
>> 
>> -    if (Arch.empty())
>> -      CoverageArch = llvm::Triple::ArchType::UnknownArch;
>> -    else {
>> -      CoverageArch = Triple(Arch).getArch();
>> -      if (CoverageArch == llvm::Triple::ArchType::UnknownArch) {
>> -        errs() << "error: Unknown architecture: " << Arch << "\n";
>> -        return 1;
>> -      }
>> +    if (!Arch.empty() &&
>> +        Triple(Arch).getArch() == llvm::Triple::ArchType::UnknownArch) {
>> +      errs() << "error: Unknown architecture: " << Arch << "\n";
>> +      return 1;
>>     }
>> +    CoverageArch = Arch;
> 
> ... because we can just use Arch directly now.

But the member variable is used somewhere else. I left it in.

>> 
>>     for (const auto &File : InputSourceFiles) {
>>       SmallString<128> Path(File);
>> Index: tools/llvm-symbolizer/LLVMSymbolize.cpp
>> ===================================================================
>> --- tools/llvm-symbolizer/LLVMSymbolize.cpp
>> +++ tools/llvm-symbolizer/LLVMSymbolize.cpp
>> @@ -436,7 +436,7 @@
>>     if (I != ObjectFileForArch.end())
>>       return I->second;
>>     ErrorOr<std::unique_ptr<ObjectFile>> ParsedObj =
>> -        UB->getObjectForArch(Triple(ArchName).getArch());
>> +        UB->getObjectForArch(ArchName);
>>     if (ParsedObj) {
>>       Res = ParsedObj.get().get();
>>       ParsedBinariesAndObjects.push_back(std::move(ParsedObj.get()));