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

[Justin Bogner]

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.

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.

>    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.

>
>      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()));