[PATCH] D60250: [llvm-objdump] Allow -dynamic-reloc on ET_EXEC files

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Test cases?



================
Comment at: include/llvm/Object/ELFObjectFile.h:755-756
 ELFObjectFile<ELFT>::dynamic_relocation_sections() const {
+  if (getEType() != ELF::ET_DYN && getEType() != ELF::ET_EXEC)
+    return createError("EType is not ET_DYN or ET_EXEC");
+
----------------
I don't think this should be an error. If a client requests dynamic relocations on ET_REL, for example, they should just get no dynamic relocations. The client code should decide whether or not it requires dynamic relocations for there to be no error. Beware that this code could be used by other clients than llvm-objdump that might rely on other behaviour than what you are doing here.


================
Comment at: include/llvm/Object/ELFObjectFile.h:766
+
+  auto Sec =
+      std::find_if(Sections.begin(), Sections.end(), [](const Elf_Shdr &Sec) {
----------------
I'd prefer if this wasn't `auto`, as I don't know exactly what the type is (i.e. is it `Elf_Shdr` or something else).


================
Comment at: include/llvm/Object/ELFObjectFile.h:771-772
+
+  if (Sec == Sections.end())
+    return createError("No DYNAMIC section found");
+
----------------
This is adding an error where there wasn't one before, which seems outside the scope of this patch. It should be up to the client what to do here.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1535
+  if (auto Err = DynRelSecs.takeError()) {
     error("not a dynamic object");
     return;
----------------
Use the value in Err as the error, to give more meaningful error messages.


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

https://reviews.llvm.org/D60250