[llvm-objcopy] Add --dump-section

[paulsemel]

Summary

Add the --dump-section option to llvm-objcopy.

[paulsemel]

I know that there is this only-keep option that is supposed to be able to have similar behavior but :

• It doesn't work at all (segfault). I made it work only with the dump-section.test, otherwise with real binaries it segfault.
• This one is compliant with GNU binutils command line, and is not file format dependent. (even tho llvm-objcopy only handles ELF for the moment)

[jakehehrlich]

Thanks! I've been meaning to implement this forever. For instance this is the only way to dump non-allocated sections. I've run into cases debugging things where that was useful and had to use objcopy.

tools/llvm-objcopy/Object.h

622

If you use ELFSectionWriter you don't need to keep track of this information.

tools/llvm-objcopy/llvm-objcopy.cpp

241

It probably makes more sense to use llvm::objcopy::Object and to use an ELFSectionWriter to dump the contents.

475

You can just iterate over these instead of checking.

476–494

Could you wrap this in a function somehow?

[jakehehrlich]

Can you file a bug against me on the segfault? That sounds like a serious issue that needs to be addressed.

[jhenderson]

tools/llvm-objcopy/ObjcopyOpts.td

102

Nit: content -> contents

Also, it looks like we're being inconsistent with whether help text should have trailing full stops. I think the majority has none, so you might want to remove it here, and we can do a NFC tidy up later to bring the rest into line with each other (I could easily be persuaded that actually they should all have them - we should aim for consistency with other parts of LLVM).

tools/llvm-objcopy/llvm-objcopy.cpp

475

To be fair, this is the same as how the other options have been implemented.

477–478

I'd prefer less use of auto around here.

[paulsemel]

tools/llvm-objcopy/Object.h

622

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.

tools/llvm-objcopy/llvm-objcopy.cpp

241

See my comment above

475

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.

[paulsemel]

Added Jake and James' suggestions

[jakehehrlich]

tools/llvm-objcopy/Object.h

622

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

tools/llvm-objcopy/llvm-objcopy.cpp

475

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.

[jakehehrlich]

So here's an idea. It is ELF specific but it extends the SectionBase interface in a way I've considered for a while and would make both this and another change quite easy. If we add an OriginalData field (ArrayRef<uint8_t>) to SectionBase we can dump that inside of HandleArgs.

[paulsemel]

Added OriginData field as suggested by Jake

[jhenderson]

In D49979#1183640, @jakehehrlich wrote:

So here's an idea. It is ELF specific but it extends the SectionBase interface in a way I've considered for a while and would make both this and another change quite easy. If we add an OriginalData field (ArrayRef<uint8_t>) to SectionBase we can dump that inside of HandleArgs.

My first instinct on looking at the code as I considered this is that this should be handled in a way similar to how SplitDWOToFile works, i.e. to do it at the start of HandleArgs before any manipulation of Object takes place. I think this would mean that we wouldn't need any additional methods, because it would happen before any modifications occurred. And we have ELFSectionWriter that would achieve what we want, I believe? So just create a specific ELFSectionWriter for the dumping of sections and use that.

I'm not completely opposed to using another method to do it, but it seems like anything else we do would just add additional complexity?

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

475

yeah, i remember that discussion - at some point (and probably for a particular place - i don't remember that exactly) i thought this would provide some benefits
(like early skipping some expensive checks), but later I took a closer look - in comparison with I/O + memory allocations + ELF parsing everything else in llvm-objcopy is almost invisible, so yeah, i think that wasn't right and code clarity is more important here. I think in any case we can refactor it on a separate diff (and it's really easy to do / not a big deal).

[jakehehrlich]

Due to the soon encroaching change where -I binary will be implemented I'm very much in favor of the OriginalData field being added to SectionBase. Really seems to handle everything very cleanly IMO.

[jhenderson]

In D49979#1184977, @jakehehrlich wrote:

Due to the soon encroaching change where -I binary will be implemented I'm very much in favor of the OriginalData field being added to SectionBase. Really seems to handle everything very cleanly IMO.

Okay, go for it. I have nerves around lifetime issues with references like that, but I think the structure means that the Reader (and therefore the file it reads) will be around long enough for it to not matter.

[paulsemel]

So should I go for it guys ?

[jakehehrlich]

This LGTM other than one issue

In D49979#1184039, @jhenderson wrote:

My first instinct on looking at the code as I considered this is that this should be handled in a way similar to how SplitDWOToFile works, i.e. to do it at the start of HandleArgs before any manipulation of Object takes place. I think this would mean that we wouldn't need any additional methods, because it would happen before any modifications occurred. And we have ELFSectionWriter that would achieve what we want, I believe? So just create a specific ELFSectionWriter for the dumping of sections and use that.

I'm not completely opposed to using another method to do it, but it seems like anything else we do would just add additional complexity?

For the record I like this. It runs contrary to the requirement that --dump-section use the original unmodified data however. For instance despite the fact that we try our best (except on section order? I might fix that) to maintain the original file details we can still change things in doing things that way.

test/tools/llvm-objcopy/dump-section.test

4

Can you also dump a non-allocated section with --dump-section to show why its far more useful than the -O binary -j hack?

tools/llvm-objcopy/Object.cpp

858 ↗

(On Diff #158482)

Check for nobits here and use a size of zero if the type is nobits.

[paulsemel]

Added Jake's suggestions.

[jhenderson]

test/tools/llvm-objcopy/dump-section.test

4

I'd also like a test showing what happens when you try to dump a nobits section.

5

Rather than use the implicitly generated .symtab, I'd prefer it if you create a simple small non-alloc section, with explicit contents (assuming this shows what @jakehehrlich is discussing above).

[paulsemel]

Added James suggested tests

[paulsemel]

Should I land this guys ?

[jhenderson]

LGTM, with the suggested changes, but please get @jakehehrlich to confirm he's happy with the test changes.

test/tools/llvm-objcopy/dump-section.test

32

Nit: Move this to the line immediately after the earlier "CHECK".

tools/llvm-objcopy/llvm-objcopy.cpp

246

I think this is a little unusual formatting of an error message, using '[' and '-'. I think a more common style would be to use '"' and ':':

'Can't dump section ".foo": it has no contents'

Also, llvm-objcopy's style is generally to use upper-case for the first letter in the error message.

[paulsemel]

Added Jame's suggestions

[paulsemel]

ping @jakehehrlich

[jakehehrlich]

oh god I apologize. I was wondering why this wasn't already submitted.

test/tools/llvm-objcopy/dump-section.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy -O binary -j .text %t %t2
 # RUN: llvm-objcopy -O binary -only-keep .text %t %t3
+# RUN: llvm-objcopy --dump-section .text=%t4 %t %t5
 # RUN: od -t x1 %t2 | FileCheck %s
 # RUN: wc -c %t2 | FileCheck %s --check-prefix=SIZE
 # RUN: diff %t2 %t3
+# RUN: diff %t4 %t3
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     AddressAlign:    0x0000000000001000
     Content:         "DEADBEEF"
 ProgramHeaders:
 - Type: PT_LOAD
   Flags: [ PF_X, PF_R ]
   Sections:
     - Section: .text
 
 #CHECK: 0000000 de ad be ef
 
 #SIZE: 4

tools/llvm-objcopy/ObjcopyOpts.td

 def K : JoinedOrSeparate<["-"], "K">,
         Alias<keep_symbol>;
 def only_keep_debug : Flag<["-", "--"], "only-keep-debug">,
                           HelpText<"Currently ignored. Only for compaitability with GNU objcopy.">;
 def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
                       HelpText<"Remove all symbols not needed by relocations">;
 def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,
                       HelpText<"Do not remove file symbols">;
+defm dump_section : Eq<"dump-section">,
+                   MetaVarName<"section=file">,
+                   HelpText<"Dump content of section named <section> into file <file>.">;

tools/llvm-objcopy/Object.h

private:
   void init(StringRef File, StringRef Data);
 
 public:
   // If we add this section from an external source we can use this ctor.
   explicit GnuDebugLinkSection(StringRef File);
   void accept(SectionVisitor &Visitor) const override;
 };
 
+using object::Binary;
+
 class Reader {
 public:
   virtual ~Reader();
   virtual std::unique_ptr<Object> create() const = 0;
+  explicit Reader(Binary *B) : Bin(B){};
+  Binary *Bin;
 };
 
-using object::Binary;
 using object::ELFFile;
 using object::ELFObjectFile;
 using object::OwningBinary;
 
 template <class ELFT> class ELFBuilder {
 private:
   using Elf_Addr = typename ELFT::Addr;
   using Elf_Shdr = typename ELFT::Shdr;

 public:
   ELFBuilder(const ELFObjectFile<ELFT> &ElfObj, Object &Obj)
       : ElfFile(*ElfObj.getELFFile()), Obj(Obj) {}
 
   void build();
 };
 
 class ELFReader : public Reader {
-  Binary *Bin;
 
 public:
   ElfType getElfType() const;
   std::unique_ptr<Object> create() const override;
-  explicit ELFReader(Binary *B) : Bin(B){};
+  explicit ELFReader(Binary *B) : Reader(B){};
 };
 
 class Object {
 private:
   using SecPtr = std::unique_ptr<SectionBase>;
   using SegPtr = std::unique_ptr<Segment>;
 
   std::vector<SecPtr> Sections;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   StringRef BinaryArch;
 
   StringRef SplitDWO;
   StringRef AddGnuDebugLink;
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
+  std::vector<StringRef> DumpSection;
   std::vector<StringRef> SymbolsToLocalize;
   std::vector<StringRef> SymbolsToGlobalize;
   std::vector<StringRef> SymbolsToWeaken;
   std::vector<StringRef> SymbolsToRemove;
   std::vector<StringRef> SymbolsToKeep;
   StringMap<StringRef> SectionsToRename;
   StringMap<StringRef> SymbolsToRename;
   bool StripAll = false;
static void SplitDWOToFile(const CopyConfig &Config, const Reader &Reader,
   DWOFile->removeSections(
       [&](const SectionBase &Sec) { return OnlyKeepDWOPred(*DWOFile, Sec); });
   FileBuffer FB(File);
   auto Writer = CreateWriter(Config, *DWOFile, FB, OutputElfType);
   Writer->finalize();
   Writer->write();
 }
 
+static Expected<SectionRef> getSectionByName(ObjectFile *Obj,
+                                             StringRef SecName) {
+  StringRef SectionName;
+  for (SectionRef SecRef : Obj->sections()) {
+    if (std::error_code E = SecRef.getName(SectionName))
+      return errorCodeToError(E);
+
+    if (SectionName == SecName)
+      return SecRef;
+  }
+  return createError("section not found");
+}
+
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority
 // system. The only priority is that keeps/copies overrule removes.
 static void HandleArgs(const CopyConfig &Config, Object &Obj,
for (const auto &Flag : Config.AddSection) {
       auto Buf = std::move(*BufOrErr);
       auto BufPtr = reinterpret_cast<const uint8_t *>(Buf->getBufferStart());
       auto BufSize = Buf->getBufferSize();
       Obj.addSection<OwnedDataSection>(SecName,
                                        ArrayRef<uint8_t>(BufPtr, BufSize));
     }
   }
 
+  if (!Config.DumpSection.empty()) {
+    for (const auto &Flag : Config.DumpSection) {
+      auto SecPair = Flag.split("=");
+      auto SecName = SecPair.first;
+      auto File = SecPair.second;
+      if (ObjectFile *O = dyn_cast<ObjectFile>(Reader.Bin)) {
+        Expected<SectionRef> SecRefOrError = getSectionByName(O, SecName);
+        if (!SecRefOrError)
+          reportError(Config.InputFilename, SecRefOrError.takeError());
+        SectionRef SecRef = *SecRefOrError;
+        StringRef SecContents;
+        SecRef.getContents(SecContents);
+        Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
+            FileOutputBuffer::create(File, SecContents.size());
+        if (!BufferOrErr)
+          reportError(File, BufferOrErr.takeError());
+        auto Buf = std::move(*BufferOrErr);
+        std::copy(SecContents.begin(), SecContents.end(),
+                  Buf->getBufferStart());
+        if (Error E = Buf->commit())
+          reportError(Config.InputFilename, std::move(E));
+      }
+    }
+  }
+
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
 }
 
 static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Binary &Binary,
                                       Buffer &Out) {
   ELFReader Reader(&Binary);
   std::unique_ptr<Object> Obj = Reader.create();
static CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(OBJCOPY_remove_section))
     Config.ToRemove.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep))
     Config.Keep.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_only_keep))
     Config.OnlyKeep.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_add_section))
     Config.AddSection.push_back(Arg->getValue());
+  for (auto Arg : InputArgs.filtered(OBJCOPY_dump_section))
+    Config.DumpSection.push_back(Arg->getValue());
   Config.StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
   Config.StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
   Config.StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
   Config.StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
   Config.StripUnneeded = InputArgs.hasArg(OBJCOPY_strip_unneeded);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);