[PATCH] D50343: [llvm-objcopy] Add support for -I binary -B <arch>.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:613
+  SymTab.Name = ".symtab";
+  SymTab.Link = size(Obj->sections()) - 1;
+  // TODO: Factor out dependence on ElfType here.
----------------
rupprecht wrote:
> jhenderson wrote:
> > Doesn't this assignment rely on knowing that the symbol table is added immediately after the string table? That seems like poor design to me. Better would be to pass in the index or string table section.
> > 
> > I might well be forgetting how llvm-objcopy is designed in this area, but don't we need to explicitly add a null symbol as the first symbol in the symbol table?
> Yes, this is very fragile, I think I understand the issue I was having before a little better.
> 
> The Link needs to be set to the StrTab Index, as you mention. The Object::addSection() helper was not assigning any index, so this would implicitly be zero (which is SHN_UNDEF), and that was throwing errors when initializing it. Using size(Obj->sections()) - 1 was more of a reverse-engineered way of getting things working.
> 
> Changing Object::addSection() to automatically assign the index lets us use the index from StrTab directly. I think this might allow us to stop assigning manually indices elsewhere in this file, but I'll save that for another change.
> 
> Also -- fixed the symbol table to include a null symbol.
Yeah that probably should have functioned that way the whole time. Thanks for fixing that!


Repository:
  rL LLVM

https://reviews.llvm.org/D50343