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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

No null symbol is a blocker, so I've marked this as requesting changes. Sorry!



================
Comment at: test/tools/llvm-objcopy/binary-input-and-output.test:5
+# RUN: llvm-objcopy -I binary -B i386:x86-64 -O binary %t.txt %t.2.txt
+# RUN: diff %t.txt %t.2.txt
+
----------------
I'm not sure if this is important or not in this case, but the usual pattern in llvm-objcopy tests is to copy the input file to a backup to verify that it hasn't been modified before doing the diff.


================
Comment at: test/tools/llvm-objcopy/binary-input-arch.test:1
+# RUN: echo -n abcd > %t.txt
+
----------------
I don't think -n is important here? Indeed, you could do away with this file entirely and just feed in %s into llvm-objcopy, if you wanted, although this is probably less confusing.


================
Comment at: test/tools/llvm-objcopy/binary-input-error.test:1
+# RUN: echo -n abcd > %t.txt
+
----------------
The -n isn't important here either.


================
Comment at: test/tools/llvm-objcopy/binary-input.test:73
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name:
----------------
This is an illegal symbol table: it has no null symbol.


================
Comment at: tools/llvm-objcopy/Object.cpp:613
+  SymTab.Name = ".symtab";
+  SymTab.Link = size(Obj->sections()) - 1;
+  // TODO: Factor out dependence on ElfType here.
----------------
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?


================
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,
----------------
Is there any point in adding this local section symbol? It can't be referenced, so I think it's superfluous.


Repository:
  rL LLVM

https://reviews.llvm.org/D50343