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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:622
+  explicit Reader(Binary *B) : Bin(B){};
+  Binary *Bin;
 };
----------------
paulsemel wrote:
> 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.
Hmm those facts are a bit at odds with what I've asked I suppose. The problem, as I see it, is the awkward extension of the Reader interface and the fact that this action is being embedded deep into some code that really has nothing todo with it. My solution appears not to be right either.

The fact that the dump-section happens before modification throws a wrench in my idea. Also thinking back on my use cases what I really always wanted was to know exactly what was in the binary, not what llvm-objcopy/strip modified it to become (which can happen even if no modifications are made just because layout can change). So I think dump-section happening before modification makes sense; this isn't GNU objcopy being strange again. In that case I think it would be ideal to use ExecuteElfObjcopyOnBinary and to handle this option there instead of in HandleArgs. Doing this in HandleArgs requires the kind of plumbing you've used here. Doing it before HandleArgs 1) makes it clear this is to happen independent of HandleArgs and 2) Negates the need for this plumbing which are the cheif issues as I see them.

As a further note, this would not work with an upcoming change to Reader when BinaryReader is implemented. So this change is at odds with the intended abstraction and an abstraction that is going to be used in the near future. So I can't allow the adding of Bin here like this (not unless another solution is presented...coincidentally Alex, James and I agreed on a refactoring scheme that I've yet to implement that would in fact resolve this but it would also change the code in such extensive ways that I'm not sure we'd even have the same issue anymore).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:475
+  if (!Config.DumpSection.empty()) {
+    for (const auto &Flag : Config.DumpSection) {
+      auto SecPair = Flag.split("=");
----------------
paulsemel wrote:
> 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.
Ah indeed. I think I've decided I find that silly. Do it this way for now. I might change that in the future. I remember the review where we did this.


Repository:
  rL LLVM

https://reviews.llvm.org/D49979