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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:643
+Expected<std::vector<BBAddrMap>> ELFFile<ELFT>::decodeBBAddrMap(
+    const Elf_Shdr &Sec, const std::optional<Elf_Shdr> &RelaSec) const {
+  bool IsRelocatable = getHeader().e_type == ELF::ET_REL;
----------------
Do not pass arguments by const `std::optional&` as it will make a copy if we pass it a `Elf_Shdr`. I suggest we pass by `const Elf_Shdr *` and let `nullptr` indicate that the value doesn't exist. 


================
Comment at: llvm/lib/Object/ELF.cpp:645
+  bool IsRelocatable = getHeader().e_type == ELF::ET_REL;
+  assert(!IsRelocatable || (IsRelocatable && RelaSec));
+
----------------
Callers may call this function on relocatable sections without providing `RelaSec`. If assertions are not enabled, we will still get zero addresses for all functions. I think it's better to report error here instead.

In general, assertions should be used to check the invariants of the code, not things like input validation.


================
Comment at: llvm/lib/Object/ELF.cpp:648
+  // This DenseMap maps the offset of each function (the location of the
+  // reference to the function in the bbaddrmap section) to the addend
+  // (the location of the function in the text section).
----------------
Here and elsewhere, please use `SHT_LLVM_BB_ADDR_MAP` when referring to the bb-address-map data. This will make code search easier.


================
Comment at: llvm/lib/Object/ELF.cpp:657
+                         toString(Relas.takeError()));
+    for (Elf_Rela Rela : *Relas) {
+      FunctionOffsetTranslations[Rela.r_offset] = Rela.r_addend;
----------------
Remove braces for this loop.


================
Comment at: llvm/lib/Object/ELF.cpp:706
+      if (FOTIterator == FunctionOffsetTranslations.end()) {
+        if (!Cur)
+          return Cur.takeError();
----------------
Move this above:

```
uintX_t Address = static_cast<uintX_t>(Data.getAddress(Cur));
if (!Cur) return Cur.takeError();
if (IsRelocatable) {
   assert(Address == 0);
   auto FOTIterator = FunctionOffsetTranslations.find(SectionOffset);
   if (FOTIterator == FunctionOffsetTranslations.end()) 
             return createError("failed to get relocation data for offset: " +
                          Twine::utohexstr(SectionOffset) + " in section " +
                          describe(*this, Sec));
...
```


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:711-716
+    if (IsRelocatable) {
+      if (!RelocSec) {
+        return createError("unable to get relocation section for " +
+                           describe(EF, *Sec));
+      }
+      RelaSec = *RelocSec;
----------------
Moving this error handling to `decodeBBAddr` and passing `RelaSec` by `const Elf_Shdr *` will greatly simplify this code:

```
for (auto const &[Sec, RelocSec] : *SectionRelocMapOrErr) {
    Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
        EF.decodeBBAddrMap(*Sec, RelocSec);
}
```


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:11
+# RUN: llvm-objdump %t1 -d --symbolize-operands -M att --no-show-raw-insn --no-leading-addr | \
+# RUN:   FileCheck %s -DSYM=symbol --match-full-lines
+
----------------
nit: unnecessary spaces.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:168
+## Check that if we have a ET_DYN file with a .rela.dyn section, we don't get
+## a warning about a missing relocation section and can get the baddrmap data.
+
----------------
Change to `SHT_LLVM_BB_ADDR_MAP`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841