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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a subscriber: shenhan.
rahmanl added a comment.

Thanks for doing this. It has been on our to-do list. @shenhan can share more insights.

Some structural comments.



================
Comment at: llvm/lib/Object/ELF.cpp:642
 template <class ELFT>
-Expected<std::vector<BBAddrMap>>
-ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec) const {
+Expected<bool> ELFFile<ELFT>::shouldSkipBasedOnTextSection(
+    std::optional<unsigned> TextSectionIndex,
----------------
Let's see if we can remove this function too, by incorporating the check into the `IsMatch` predicate.


================
Comment at: llvm/lib/Object/ELF.cpp:688
+    llvm::MapVector<const typename ELFT::Shdr *, const typename ELFT::Shdr *>>
+ELFFile<ELFT>::getBBAddrMapAndRelaSections(
+    std::optional<unsigned> TextSectionIndex) const {
----------------
Let's move `ELFDumper::getSectionAndRelocations` to `ELFFile` and use it here instead of writing a custom version.


================
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;
----------------
Just define a lambda or a static function for this, and read in `Elf_Rela_Range`.


================
Comment at: llvm/lib/Object/ELF.cpp:724
+  llvm::DenseMap<uint64_t, uint64_t> AddrTranslationMap;
+  auto Relas = this->relas(RelaSec);
+  if (!Relas) {
----------------
Let's move this into the caller.


================
Comment at: llvm/lib/Object/ELF.cpp:738-741
+  if (IsRelocatable && !RelaSec) {
+    return createError("decodeBBAddrMap called in a relocatable file with no "
+                       "relocation section");
+  }
----------------
Change this to assertion (since it's checking something about the code logic).


================
Comment at: llvm/lib/Object/ELF.cpp:790-795
+    if(IsRelocatable) {
+      Address = BBAddrMapTranslations[Cur.tell()];
+      Cur.seek(Cur.tell() + sizeof(uintX_t));
+    } else {
+      Address = static_cast<uintX_t>(Data.getAddress(Cur));
+    }
----------------
The IsRelocatable path should also read the Address (even though it's normally zero). Something like:

```
uint64_t SectionOffset = Cur.tell();
Address = static_cast<uintX_t>(Data.getAddress(Cur));
if (IsRelocatable) {
  assert(Address == 0); // maybe
  Address += OffsetToRela.at(SectionOffset).r_addend;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841