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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:908
   for (const Relocation &Reloc : Relocations)
     Reloc.RelocSymbol->Referenced = true;
 }
----------------
jhenderson wrote:
> This looks dodgy if no symbol exists.
"relocation references symbol with index..." error is triggered earlier than execution flow might reach here.

It is reported from `initRelocations` which is called inside `Reader.create()` here:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L773

`markSymbols()` is called from `updateAndRemoveSymbols` which is itself caled in `handleArgs`:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L799
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L613
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L402

`handleArgs` is called later than `Reader.create()`. 

Also, it checks for an empty symbol table at its begining:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L350


================
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);
----------------
MaskRay wrote:
> jhenderson wrote:
> > 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?
> > "relocation references symbol with index N, but there is no symbol table"
> 
> +1
Yep, that is better.


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

https://reviews.llvm.org/D69304