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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht 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.
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.cpp:639-640
+
+  SymTab->addSymbol("", STB_LOCAL, STT_SECTION, &DataSection, /*Value=*/0,
+                    STV_DEFAULT, 0, 0);
+  SymTab->addSymbol(Prefix + "_start", STB_GLOBAL, STT_NOTYPE, &DataSection,
----------------
jhenderson wrote:
> Is there any point in adding this local section symbol? It can't be referenced, so I think it's superfluous.
GNU objcopy adds it, but it doesn't seem to be necessary. I'll add it back if it turns out to be needed.


Repository:
  rL LLVM

https://reviews.llvm.org/D50343