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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've taken a low-level view of the code, as I'm not massively familiar with the higher level stuff. In addition to my various inline comments, please add testing for all the new error/warning paths.



================
Comment at: llvm/lib/Object/ELF.cpp:673
+      continue;
+    auto TextSectionSkip = shouldSkipBasedOnTextSection(TextSectionIndex, Sec);
+    if(!TextSectionSkip) {
----------------
Don't use `auto` here. It's not obvious what the return type is.


================
Comment at: llvm/lib/Object/ELF.cpp:680
+    }
+    BBAddrMapSections[&Sec] = (const Elf_Shdr *)nullptr;
+  }
----------------
Are you sure you need the cast here?


================
Comment at: llvm/lib/Object/ELF.cpp:694-696
+    if (Sec.sh_type != ELF::SHT_RELA) {
+      continue;
+    }
----------------
Here and in many other places, don't use braces for single line if statements like this. See 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:700
+      return createError(
+          "Failed to read section linked to from section sh_info");
+    }
----------------
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/lib/Object/ELF.cpp:707
+    }
+    // we know we have a BBAddrMapSection with the associated rela section
+    auto TextSectionSkip = shouldSkipBasedOnTextSection(TextSectionIndex, *PossibleBBAddrMapSection);
----------------



================
Comment at: llvm/lib/Object/ELF.cpp:728
+  }
+  for (auto Rela : *Relas) {
+    AddrTranslationMap[Rela.r_offset] = Rela.r_addend;
----------------
This probably shouldn't use `auto`, as the type isn't obvious from the context.


================
Comment at: llvm/lib/Object/ELF.cpp:736
+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;
----------------
Should `RelaSec` be `const &` not just `const`?


================
Comment at: llvm/lib/Object/ELF.cpp:744
+  if(IsRelocatable) {
+    auto TranslationsOrErr = getBBAddrMapTranslations(*RelaSec);
+    if(!TranslationsOrErr) {
----------------
Again, don't use `auto` here.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:5
+
+# UNSUPPORTED: system-windows
+
----------------
Why is this unsupported on Windows?


================
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
----------------
This Content is a bit of an opaque blob. Would it be possible and clearer to write this test using assembly instead?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:4
+
+## UNSUPPORTED: system-windows
+
----------------
Again, why is this unsupported on Windows?


================
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
----------------
Again, would it be practical and cleare to write this test using assembly?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7117
+    if(!SecsOrErr) {
+      this->reportUniqueWarning("Cannot get section");
+    }
----------------
This warning should provide more context about why it cannot get the section. That context should be available in the error returned by the function you called. Indeed, I wouldn't be surprised if you had a failure in some build configurations because the Error within the Expected has been thrown away without any usage.

Also applies below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841