[PATCH] D69304: [llvm-objcopy] - Do not crash on object that has relocations but no symbol table.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/relocations-no-symtab.test:2
+## Check that we can copy an object that has a relocation
+## that holds symbol index 0 even when there is no symbol table.
+
----------------
I think "with a symbol index of 0" would be more natural than "that holds symbol index 0"


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/relocations-no-symtab.test:34
+
+# RUN: yaml2obj --docnum=2 %s > %t3
+# RUN: not llvm-objcopy %t3 %t4 2>&1 | FileCheck %s --check-prefix=ERR
----------------
-o for consistency.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/relocations-no-symtab.test:43
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
----------------
It probably makes slightly more sense for this to be ET_REL.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:908
   for (const Relocation &Reloc : Relocations)
     Reloc.RelocSymbol->Referenced = true;
 }
----------------
This looks dodgy if no symbol exists.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1426-1427
+      if (!SymbolTable)
+        error("'" + Relocs->Name + "': relocation references a symbol (" +
+              Twine(Sym) + "), but there is no symbol table");
+      ToAdd.RelocSymbol = SymbolTable->getSymbolByIndex(Sym);
----------------
I'm not sure this error message is clear enough. I think it is not obvious what the number in it represents. Perhaps it could be rephrased "relocation references symbol with index N, but there is no symbol table". What do you think?


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

https://reviews.llvm.org/D69304