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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/binary-input-and-output.test:1
+# RUN: echo -n abcd > %t.txt
+
----------------
Is the no new line important to this test? I think it isn't any more.


================
Comment at: tools/llvm-objcopy/Object.cpp:642
+                    0, 0);
+  SymTab->addSymbol(S + "_end", STB_GLOBAL, STT_NOTYPE, &DS, DS.Size,
+                    STV_DEFAULT, 0, 0);
----------------
rupprecht wrote:
> jhenderson wrote:
> > Should the end symbol have a size? That seems weird that it does. I might expect the start symbol to, but not the end symbol. What does GNU objcopy do?
> Nope, this is Value, not Size, although it's confusing, especially because addSymbol has so many parameters. I commented the param names here to make this clear.
> The sizes/values here match gnu objcopy.
Right, I get it now. Thanks for the comments.


================
Comment at: tools/llvm-objcopy/Object.h:457
+                 uint64_t Value, uint8_t Visibility, uint16_t Shndx,
+                 uint64_t Sz);
   void prepareForLayout();
----------------
rupprecht wrote:
> jhenderson wrote:
> > Don't abbreviate variable names: Sz -> Size.
> Done, although this is existing code -- it's only showing up as a diff because clang-format put it on a new line after I changed StringRef->Twine...
Ah, yes of course. But the renaming is good anyway.


================
Comment at: tools/llvm-objcopy/Object.h:703
   std::unique_ptr<Object> create() const override;
-  explicit ELFReader(Binary *B) : Bin(B){};
+  explicit ELFReader(Binary *B) : Bin(B) {}
 };
----------------
If this is a clang-format added difference, please do it in another patch.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:133
   StringRef InputFormat;
-  StringRef BinaryArch;
+  MachineInfo BinaryArch;
 
----------------
rupprecht wrote:
> jhenderson wrote:
> > It looks to me like we are grouping things in CopyConfig by type, so this should probably be moved to before (or after) the StringRef block.
> Done -- I added some comments to try to logically explain how this config is laid out, but I'm not very familiar with all of them... I'm open to suggestions here.
I'd like @jakehehrlich to comment on these comments, if that's okay, as he may have a specific desire as to how this class is laid out.


Repository:
  rL LLVM

https://reviews.llvm.org/D50343