Page MenuHomePhabricator

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

Authored by johnislarry on Apr 9 2020, 10:32 PM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
50 msLLVM.tools/llvm-xray/ARM::Unknown Unit Message ("")
Script: -- : 'RUN: at line 4'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/yaml2obj /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/tools/llvm-xray/ARM/elf32-pic.yaml -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/test/tools/llvm-xray/ARM/Output/extract-instrmap-arm.test.tmp.so

Event Timeline

johnislarry created this revision.Apr 9 2020, 10:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 10:32 PM
MaskRay added inline comments.Apr 10 2020, 12:13 AM
llvm/lib/XRay/InstrumentationMap.cpp
67

AArch64/ARM/ppc64le/x86-64

128–131

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

Address reviewer feedback

MaskRay requested changes to this revision.Apr 10 2020, 9:32 AM
MaskRay added inline comments.
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.

This revision now requires changes to proceed.Apr 10 2020, 9:32 AM

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

Consolidate tests and use more minimal yaml test asset

Add yaml test asset

MaskRay added inline comments.Apr 13 2020, 11:34 AM
llvm/test/tools/llvm-xray/ARM/elf32-pic.yaml
1

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

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 added a comment.EditedApr 13 2020, 11:38 AM

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 :)

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!

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!

Thanks for the explanation @MaskRay !

johnislarry abandoned this revision.Apr 16 2020, 3:14 PM

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

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

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.