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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:707
+        return createError("failed to get relocation data for offset: " +
+                           Twine(SectionOffset) + " in section " +
+                           describe(*this, Sec));
----------------
aidengrossman wrote:
> jhenderson wrote:
> > I wonder if `utohexstr` would be better here, to give a hex offset?
> Thank you for the suggestion. I would think it would definitely be better here to be consistent with the rest of the output from the LLVM tools.
There's a `Twine::utohexstr` (that I didn't know about), which would be more appropriate here, I think.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:71
+## Check that if we're missing a relocation section we get the appropriate
+## warning
+
----------------



================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:92
+# NO-RELA-SECTION: warning: '[[FILE]]': unable to get relocation section for SHT_LLVM_BB_ADDR_MAP section with index 2
\ No newline at end of file

----------------
Nit: please add a new line at the end of the file.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:103
+  Machine:         EM_X86_64
+  SectionHeaderStringTable: .strtab
+Sections:
----------------
There's no particular need for this, is there? Same elsewhere.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:117
+
+## Check that we get a warning if we expect a relocation and it is not present
+
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:150
+
+## Check that a relocatable object file with a bbaddrmap section that
+## doesn't have an associated relocation section gives the appropriate
----------------
Isn't this the same as the test case two up?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:174
+
+## Check that if we have a missing relocation section, getSectionAndRelocations
+## fails and we get the appropriate warning.
----------------
?

Is it worth the following test case: an ET_EXEC or ET_DYN file with a .rela.dyn section that doesn't reference a particular section? To show that we don't get this warning in that situation?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:193
+# NO-RELOCATED-SECTION: warning: '[[FILE]]': failed to get bbaddrmap section(s): SHT_RELA section with index 1: failed to get a relocated section: invalid section index: 255
\ No newline at end of file

----------------
Nit: no new line at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841