[llvm-xray] Add llvm-xray extract support for 32 bit ARM

[johnislarry]

Summary

We have support for instrumenting ARM builds with xray sleds,
but we don't have the corresponding infrastructure in llvm-xray extract
so this diff adds it.

[MaskRay]

llvm/lib/XRay/InstrumentationMap.cpp

67

AArch64/ARM/ppc64le/x86-64

128–131

size_t ELFSledEntrySize = ObjFile.getBinary()->makeTriple().isArch32Bit() ? 16 : 32

[johnislarry]

Address reviewer feedback

[MaskRay]

llvm/lib/XRay/InstrumentationMap.cpp

133

In many command line tools, clang/lld/LLVM binary utilities/etc, a diagnostic message is not capitalized. I have noticed that llvm-xray does not follow the convention, so being consistent locally is okay.

llvm/test/tools/llvm-xray/ARM/extract-instrmap-arm-mangled.test

1 ↗

(On Diff #256586)

You can combine the two tests, and move the content of llvm/test/tools/llvm-xray/ARM/Inputs/elf32-pic.yaml here.

## is for comments

# is for RUN and CHECK lines
# RUN: yaml2obj %s -o %t

llvm/test/tools/llvm-xray/ARM/Inputs/elf32-pic.yaml is too large. This is not okay. It should be reduced to the minimum.

[johnislarry]

Thanks for the feedback @MaskRay. I'm going to have to come back to this next week, I'll aim to put up an update Monday

[johnislarry]

Consolidate tests and use more minimal yaml test asset

[johnislarry]

Add yaml test asset

[MaskRay]

llvm/test/tools/llvm-xray/ARM/elf32-pic.yaml

1 ↗

(On Diff #257024)

A non-test supplement file should be placed under Inputs/ , otherwise it can be interpreted as a test.

lit will consider it an unresolved test

9 ↗

(On Diff #257024)

Most program headers and sections in this file are not needed. You can figure out the minimum information llvm-xray extract expects and just provide that much. The extra fields are distracting.

[MaskRay]

If you configure LLVM with -DLLVM_ENABLE_ASSERTIONS=on (default if -DCMAKE_BUILD_TYPE=Debug), you shall notice an assertion failure. This is because the existing ELF targets llvm-xray supports are all RELA targets. EM_ARM uses REL and llvm-xray did not consider the case. This requires a relocation fix in llvm/lib/XRay/InstrumentationMap.cpp

If you don't mind, I can take over the patch and try to fix the problem in my spare time.

Out of curiosity, I want to hear about your use cases on EM_ARM :)

[johnislarry]

Ah yeah, I see the assertion you're talking about. It looks like the RelocationResolver for arm is incomplete maybe? I've attached a picture of something I tried, but down the road it can't find the address for the symbol.

It also looks like this doesn't affect results in release builds because later when we call RelocateOrElse, we always have an address, so we ignore the relocations entirely.

I would appreciate you taking it from here, but please let me know if it falls off your radar so I can pick it back up!

At FB we're starting to use xray on Android to better understand our native code execution, so definitely interested in getting ARM variants working!

[MaskRay]

In D77858#1978965, @johnislarry wrote:

Ah yeah, I see the assertion you're talking about. It looks like the RelocationResolver for arm is incomplete maybe? I've attached a picture of something I tried, but down the road it can't find the address for the symbol.

Object/RelocationResolver.cpp is for static relocation types. It acts like a linker. R_*_RELATIVE are dynamic relocations typically handled by ld.so. Object/RelocationResolver.cpp does not need to know dynamic relocations. For certain dynamic relocations, there simply isn't enough information. In this case (EM_ARM is a REL target), we need to read the value from the executable to get the addend.

It also looks like this doesn't affect results in release builds because later when we call RelocateOrElse, we always have an address, so we ignore the relocations entirely.

I would appreciate you taking it from here, but please let me know if it falls off your radar so I can pick it back up!

The problem is that the addresses in xray_instr_map do not need to use absolute addresses. They should be PC-relative initially to avoid R_*_RELATIVE. I am going to fix that so that we will not see R_*_RELATIVE in executables/shared objects. The status is unfortunately sad for 64-bit MIPS, which does not have a 64-bit relocation type R_MIPS_PC64. Interestingly, I just closed a bug related to R_MIPS_PC64... https://bugs.llvm.org/show_bug.cgi?id=44991

At FB we're starting to use xray on Android to better understand our native code execution, so definitely interested in getting ARM variants working!

Ah, thanks!

[johnislarry]

Thanks for the explanation @MaskRay !

[MaskRay]

Cleaned up a bit 4113e388c82f0530622149d61d92f630a80b0abb.

Created D78082.

[johnislarry]

Awesome - thanks for pushing the ball forward on this @MaskRay!

[MaskRay]

In D77858#1987814, @johnislarry wrote:

Awesome - thanks for pushing the ball forward on this @MaskRay!

I am mostly waiting for @dberris to make a new name for the readonly xray_instr_maps section...

[smeenai]

In D77858#1987868, @MaskRay wrote:

In D77858#1987814, @johnislarry wrote:

Awesome - thanks for pushing the ball forward on this @MaskRay!

I am mostly waiting for @dberris to make a new name for the readonly xray_instr_maps section...

Have you seen https://reviews.llvm.org/D78082#1982766? It seems he wants you to use versioning instead of a different name.

llvm/lib/XRay/InstrumentationMap.cpp

 //===- InstrumentationMap.cpp - XRay Instrumentation Map ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
loadObj(StringRef Filename, object::OwningBinary<object::ObjectFile> &ObjFile,
           InstrumentationMap::FunctionAddressMap &FunctionAddresses,
           InstrumentationMap::FunctionAddressReverseMap &FunctionIds) {
   InstrumentationMap Map;
 
   // Find the section named "xray_instr_map".
   if ((!ObjFile.getBinary()->isELF() && !ObjFile.getBinary()->isMachO()) ||
       !(ObjFile.getBinary()->getArch() == Triple::x86_64 ||
         ObjFile.getBinary()->getArch() == Triple::ppc64le ||
+        ObjFile.getBinary()->getArch() == Triple::arm ||
         ObjFile.getBinary()->getArch() == Triple::aarch64))
     return make_error<StringError>(
-        "File format not supported (only does ELF and Mach-O little endian 64-bit).",
+        Twine("File format not supported.  Supports: AArch64/ARM/ppc64le/x86-64."),
         std::make_error_code(std::errc::not_supported));
 
   StringRef Contents = "";
   const auto &Sections = ObjFile.getBinary()->sections();
   auto I = llvm::find_if(Sections, [&](object::SectionRef Section) {
     Expected<StringRef> NameOrErr = Section.getName();
     if (NameOrErr)
       return *NameOrErr == "xray_instr_map";
for (const object::SectionRef &Section : Sections) {
             Relocs.insert({Reloc.getOffset(), *AddendOrErr});
         }
       }
     }
   }
 
   // Copy the instrumentation map data into the Sleds data structure.
   auto C = Contents.bytes_begin();
-  static constexpr size_t ELF64SledEntrySize = 32;
 
-  if ((C - Contents.bytes_end()) % ELF64SledEntrySize != 0)
+  bool Is32Bit = ObjFile.getBinary()->makeTriple().isArch32Bit();
+  size_t ELFSledEntrySize = Is32Bit ? 16 : 32;
+
+  if ((C - Contents.bytes_end()) % ELFSledEntrySize != 0)
     return make_error<StringError>(
         Twine("Instrumentation map entries not evenly divisible by size of "
-              "an XRay sled entry in ELF64."),
+              "an XRay sled entry."),
         std::make_error_code(std::errc::executable_format_error));
 
   auto RelocateOrElse = [&](uint64_t Offset, uint64_t Address) {
     if (!Address) {
       uint64_t A = I->getAddress() + C - Contents.bytes_begin() + Offset;
       RelocMap::const_iterator R = Relocs.find(A);
       if (R != Relocs.end())
         return R->second;
     }
     return Address;
   };
 
   int32_t FuncId = 1;
   uint64_t CurFn = 0;
-  for (; C != Contents.bytes_end(); C += ELF64SledEntrySize) {
+  for (; C != Contents.bytes_end(); C += ELFSledEntrySize) {
     DataExtractor Extractor(
-        StringRef(reinterpret_cast<const char *>(C), ELF64SledEntrySize), true,
+        StringRef(reinterpret_cast<const char *>(C), ELFSledEntrySize), true,
         8);
     Sleds.push_back({});
     auto &Entry = Sleds.back();
     uint64_t OffsetPtr = 0;
     uint64_t AddrOff = OffsetPtr;
+    if (Is32Bit)
+      Entry.Address = RelocateOrElse(AddrOff, Extractor.getU32(&OffsetPtr));
+    else
       Entry.Address = RelocateOrElse(AddrOff, Extractor.getU64(&OffsetPtr));
     uint64_t FuncOff = OffsetPtr;
+    if (Is32Bit)
+      Entry.Function = RelocateOrElse(FuncOff, Extractor.getU32(&OffsetPtr));
+    else
       Entry.Function = RelocateOrElse(FuncOff, Extractor.getU64(&OffsetPtr));
     auto Kind = Extractor.getU8(&OffsetPtr);
     static constexpr SledEntry::FunctionKinds Kinds[] = {
         SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT,
         SledEntry::FunctionKinds::TAIL,
         SledEntry::FunctionKinds::LOG_ARGS_ENTER,
         SledEntry::FunctionKinds::CUSTOM_EVENT};
     if (Kind >= sizeof(Kinds))
       return errorCodeToError(

llvm/test/tools/llvm-xray/ARM/extract-instrmap-arm.test

+## This test makes sure we can extract the instrumentation map from an
+## XRay-instrumented PIE file.
+
+RUN: yaml2obj %S/elf32-pic.yaml -o %t.so
+RUN: llvm-xray extract %t.so -s > %t.test
+RUN: llvm-xray extract %t.so -s --no-demangle >> %t.test
+RUN: cat %t.test | FileCheck %s
+
+CHECK:      ---
+CHECK-NEXT: - { id: 1, address: 0x000000000000043C, function: 0x000000000000043C, kind: function-enter, always-instrument: true, function-name: 'bar()' }
+CHECK-NEXT: - { id: 2, address: 0x000000000000045C, function: 0x000000000000045C, kind: function-enter, always-instrument: true, function-name: 'jar()' }
+CHECK-NEXT: - { id: 3, address: 0x000000000000047C, function: 0x000000000000047C, kind: function-enter, always-instrument: true, function-name: 'foo()' }
+CHECK:      ...
+CHECK:      ---
+CHECK-NEXT: - { id: 1, address: 0x000000000000043C, function: 0x000000000000043C, kind: function-enter, always-instrument: true, function-name: _Z3barv }
+CHECK-NEXT: - { id: 2, address: 0x000000000000045C, function: 0x000000000000045C, kind: function-enter, always-instrument: true, function-name: _Z3jarv }
+CHECK-NEXT: - { id: 3, address: 0x000000000000047C, function: 0x000000000000047C, kind: function-enter, always-instrument: true, function-name: _Z3foov }
+CHECK:      ...

llvm/test/tools/llvm-xray/X86/bad-instrmap-sizes.txt

 ; RUN: not llvm-xray extract %S/Inputs/elf64-badentrysizes.bin 2>&1 | FileCheck %s
 ; CHECK: llvm-xray: Cannot extract instrumentation map from '{{.*}}elf64-badentrysizes.bin'.
-; CHECK-NEXT: Instrumentation map entries not evenly divisible by size of an XRay sled entry in ELF64.
+; CHECK-NEXT: Instrumentation map entries not evenly divisible by size of an XRay sled entry.

llvm/test/tools/llvm-xray/X86/unsupported-elf32.txt

 ; RUN: not llvm-xray extract %S/Inputs/elf32-noxray.bin 2>&1 | FileCheck %s
 ; CHECK: llvm-xray: Cannot extract instrumentation map from '{{.*}}elf32-noxray.bin'.
-; CHECK-NEXT: File format not supported (only does ELF and Mach-O little endian 64-bit).
+; CHECK-NEXT: File format not supported.  Supports: AArch64/ARM/ppc64le/x86-64.