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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Haven't looked at the tests yet. Thanks for the revision.



================
Comment at: llvm/lib/Object/ELF.cpp:646
+  assert(!IsRelocatable || (IsRelocatable && RelaSec));
+  llvm::DenseMap<uint64_t, uint64_t> BBAddrMapTranslations;
+  if(IsRelocatable && RelaSec) {
----------------
Add a comment for this variable. The name is too general and does not convey what it really is.
Also maybe reword to `FunctionAddressTranslaton`  since this is just about function addresses in `SHT_LLVM_BB_ADDR_MAP`.


================
Comment at: llvm/lib/Object/ELF.cpp:650
+    if(!Relas) 
+      return Relas.takeError();
+    for (Elf_Rela Rela: *Relas) {
----------------
Append a context to this error (e.g, "Unable to read the relocation section for section: ....").


================
Comment at: llvm/lib/Object/ELF.cpp:651
+      return Relas.takeError();
+    for (Elf_Rela Rela: *Relas) {
+      BBAddrMapTranslations[Rela.r_offset] = Rela.r_addend;
----------------
remove  these braces : https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Object/ELF.cpp:698
+      assert(Address == 0);
+      Address = BBAddrMapTranslations[SectionOffset];
+    }
----------------
Should we report Error here if the translation entry doesn't exist?


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:696
       if (!TextSecOrErr)
-        return createError("unable to get the linked-to section for " +
-                           describe(EF, Sec) + ": " +
-                           toString(TextSecOrErr.takeError()));
+        return TextSecOrErr.takeError();
       if (*TextSectionIndex != std::distance(Sections.begin(), *TextSecOrErr))
----------------
Let's use `reportError` here. `SHT_LLVM_BB_ADDR_MAP` should always link to a text section. We may have to change the ``llvm-objdump` tests a bit then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841