[PATCH] D143841: [Propeller] Make decoding BBAddrMaps trace through relocations

[Aiden Grossman via Phabricator via llvm-commits]

aidengrossman added a comment.

Thank you everyone for all the feedback. It has been quite helpful. I believe I have addressed most of the feedback given (and probably opened up room for quite a bit more). I still need to add some tests for some of the new failure cases, but they should be relatively sparse after refactoring to use `getSectionAndRelocations()` instead of rewriting it. I've also split this patch into two (https://reviews.llvm.org/D144783). I tried to get Phabricator to pick it up as a patch stack, but for whatever reason I couldn't get it working.



================
Comment at: llvm/lib/Object/ELF.cpp:722
+Expected<llvm::DenseMap<uint64_t, uint64_t>>
+ELFFile<ELFT>::getBBAddrMapTranslations(const Elf_Shdr &RelaSec) const {
+  llvm::DenseMap<uint64_t, uint64_t> AddrTranslationMap;
----------------
rahmanl wrote:
> Just define a lambda or a static function for this, and read in `Elf_Rela_Range`.
I'm not sure what you mean here by defining a lambda or static function for this. I've moved it into the caller as is.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:25
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: 554889E5897DFC8B45FCC1E0015DC390554889E5897DF8837DF8050F8E0E0000008B45F883E8058945FCE9060000008B45F88945FC8B45FC5DC3
+  - Name:    .llvm_bb_addr_map
----------------
jhenderson wrote:
> This Content is a bit of an opaque blob. Would it be possible and clearer to write this test using assembly instead?
The content is definitely quite opaque. From what I can gather from the yaml2obj tests, it is possible to write these tests in assembly (without piping anything around), but all the content has to be in hex with assembly commentary beside it. The current precedent for tests regarding this feature is to use similar opaque blobs (although maybe this should be changed at some point). However, I'm not sure how much value adding assembly would bring. The assembly is essentially the same as the disassembled input that I'm checking later on plus a couple extra irrelevant annotations. For debugging the test case, I think having the expected output would be the most helpful and adding assembly for the input would not add a lot of value. I might be missing something obvious here though, so please let me know if I'm incorrect in one of my points or there's something I haven't thought of.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:80
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: 554889E5897DFC8B45FCC1E0015DC390554889E5897DF8837DF8050F8E0E0000008B45F883E8058945FCE9060000008B45F88945FC8B45FC5DC3
+  - Name:    .llvm_bb_addr_map
----------------
jhenderson wrote:
> Again, would it be practical and cleare to write this test using assembly?
This test can have the content of the `.text` section removed, so I've removed it completely for clarity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841