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

[Ian Levesque via Phabricator via llvm-commits]

ianlevesque marked 3 inline comments as done.
ianlevesque added a comment.

I'll keep looking at this but @MaskRay if you could point me to something in LLVM that checks whether an addend is present that I can actually access from this module that would be helpful. I added getOptionalAddend as a last resort :(



================
Comment at: llvm/lib/XRay/InstrumentationMap.cpp:120
         if (SupportsRelocation && SupportsRelocation(Reloc.getType())) {
-          auto AddendOrErr = object::ELFRelocationRef(Reloc).getAddend();
-          auto A = AddendOrErr ? *AddendOrErr : 0;
+          auto A = object::ELFRelocationRef(Reloc).getOptionalAddend();
           Expected<uint64_t> ValueOrErr = Reloc.getSymbol()->getValue();
----------------
MaskRay wrote:
> R_ARM_RELATIVE does not need this.
This branch still gets hit for the absolute relocations in the xray_fn_idx section, and with the Expected<> return it asserts in debug mode. I could not figure out a way to detect if the Addend was present from this point in the code, because all of the methods you would use to do so are protected visibilty on the ELF* classes. Can you make a more specific suggestion?


================
Comment at: llvm/lib/XRay/InstrumentationMap.cpp:127
         } else if (Reloc.getType() == RelativeRelocation) {
-          if (auto AddendOrErr = object::ELFRelocationRef(Reloc).getAddend())
-            Relocs.insert({Reloc.getOffset(), *AddendOrErr});
+          auto A = object::ELFRelocationRef(Reloc).getOptionalAddend();
+          Relocs.insert({Reloc.getOffset(), A});
----------------
MaskRay wrote:
> Perhaps we can special case R_ARM_* for now. This can avoid a `getOptionalAddend` change.
> 
> I am not sure `getOptionalAddend` will have more use cases.
I am ok with this, but I'm not sure what the check would look like - the same visibility problem from my other comment applies. Can you suggest something?


================
Comment at: llvm/test/tools/llvm-xray/ARM/extract-instrmap.test:73
+    EntSize:         0x0000000000000010
+  - Name:            .hash
+    Type:            SHT_HASH
----------------
MaskRay wrote:
> Please check whether more sections can be dropped. For example, `.hash` is not read. Many tags in .dynamic are probably not needed. `xray_fn_idx` may not be needed.
> 
> Sorry for letting you do more, but I hope we can get a minimal test case which can be used as a template to improve tests for other architectures.
Sure I can delete even more - I would not cut away xray_fn_idx because it forces that other branch to be taken right now, which exposes another assert().


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80185/new/

https://reviews.llvm.org/D80185