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.
Details
Details
- Reviewers
dberris MaskRay smeenai ianlevesque
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
| Time | Test | |
|---|---|---|
| 50 ms | LLVM.tools/llvm-xray/ARM::Unknown Unit Message ("") |
Event Timeline
| 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. |
Comment Actions
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
| 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. | |
Comment Actions
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 :)
Comment Actions
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!
Comment Actions
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!
Comment Actions
I am mostly waiting for @dberris to make a new name for the readonly xray_instr_maps section...
Comment Actions
Have you seen https://reviews.llvm.org/D78082#1982766? It seems he wants you to use versioning instead of a different name.
clang-format not found in user's PATH; not linting file.