[PATCH] D49979: [llvm-objcopy] Add --dump-section

[Paul Semel via Phabricator via llvm-commits]

paulsemel marked 3 inline comments as done.
paulsemel added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:622
+  explicit Reader(Binary *B) : Bin(B){};
+  Binary *Bin;
 };
----------------
jakehehrlich wrote:
> If you use ELFSectionWriter you don't need to keep track of this information.
Actually, the fact is that I truly think the way I am doing permits to be fully file format independent, which is a very good point for the expansion of the tool.

And I actually do not see problems doing this way because :
- I compared with GNU `objcopy`, and it seems that this option is doing first, which means that we cannot really interfer with the changes made to the binary.
- I couldn't really trigger an option that modifies the section and where the `dump-section` output is changed according to the modifications we made.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:241
 
+static Expected<SectionRef> getSectionByName(ObjectFile *Obj,
+                                             StringRef SecName) {
----------------
jakehehrlich wrote:
> It probably makes more sense to use llvm::objcopy::Object and to use an ELFSectionWriter to dump the contents.
See my comment above


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:475
+  if (!Config.DumpSection.empty()) {
+    for (const auto &Flag : Config.DumpSection) {
+      auto SecPair = Flag.split("=");
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > You can just iterate over these instead of checking.
> To be fair, this is the same as how the other options have been implemented.
Yes, my point was to follow what's being done in the rest of the code, but I can change I don't really matter.


Repository:
  rL LLVM

https://reviews.llvm.org/D49979