[llvm-objcopy] Reorder --dump-section before --remove-section for ELF.

[sameerarora101]

Summary

Reorder DumpSection under handleArgs in
file ELFObjcopy.cpp. DumpSection is placed
before replaceAndRemoveSections and is therefore
now the first operation under handleArgs. Thus,
it is now performed before both add and remove
section operations.

Change for the MachO format at D81123. Together fixes https://bugs.llvm.org/show_bug.cgi?id=44283.

[sameerarora101]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

275

Added by the linter!

[smeenai]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

275

I'd probably undo this change, given that this part of the code isn't close to the part of the code you're modifying.

[sameerarora101]

This comment was removed by sameerarora101.

[sameerarora101]

Remove linter changes.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

623

StringRef SectionName;
StringRef FileName;
std::tie(SectionName, FileName) = Flag.split('=');

[sameerarora101]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

623

Got it, thanks!

[sameerarora101]

Decouple SecPair into SectionName and FileName.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

622

nit:

for (StringRef Flag : Config.DumpSection)

StringRef is usually passed by value (it's essentially just a pointer + size)

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

622

and the same comment applies to https://reviews.llvm.org/D81123

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test

11 ↗

(On Diff #268274)

nit: it looks like it's the only place where these macros (INPUT, SECTION) are used. In this case we don't really need them, simply use the corresponding values directly (on the line 37)

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test

1 ↗

(On Diff #268274)

Perhaps dump-section-before-add-remove

3 ↗

(On Diff #268274)

##

For new tests we use ## to make comments stand out from RUN/CHECK lines.

6 ↗

(On Diff #268274)

You can dump the second line of od output (it prints the size) and delete wc -c

8 ↗

(On Diff #268274)

the newly added

11 ↗

(On Diff #268274)

I think we typically place CHECK lines before the tests. Some earlier tests do not follow the convention, but newer tests can.

18 ↗

(On Diff #268274)

Delete excess indentation. Keep one just after Machine: , the longest key.

ditto below

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

620

Section must be dumped -> Dump sections

[jhenderson]

Sorry, didn't see this patch when I posted on your Mach-O one! Thanks for it. @MaskRay and @alexshap seem to have been pretty comprehensive, so no more comments from me for now.

[sameerarora101]

llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test

11 ↗

(On Diff #268274)

I was able to do this for SECTION but it seems I still need to pass in the INPUT macro with FileCheck (it explicitly prints %t otherwise). Is that fine?

[sameerarora101]

Updating source and test file according to the inline commments.

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test

11 ↗

(On Diff #268274)

I was able to do this for SECTION but it seems I still need to pass in the INPUT macro with FileCheck (it explicitly prints %t otherwise). Is that fine?

I would remove -DSECTION - it would slightly trim down the command line / look simpler

[sameerarora101]

llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test

11 ↗

(On Diff #268274)

Yup, removed it in the new revision, thanks.

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test

10

nit: to me it's somehow easier to read the test when the "check:" lines follow the corresponding invocations:

# RUN: llvm-objcopy ...
# RUN: llvm-objdump %t | FileCheck %s

# CHECK: Expected line 1
# CHECK: Expected line 2

!ELF
FileHeader:
   Class: ...

not sure how it compares with what @MaskRay mentioned above, I do not have a strong opinion regarding this.

[Ktwu]

llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test

10

I visually prefer CHECKs after RUNs as well, and it's what folks in lld have been doing, too (and it's also consistent with the NODUMP check below -- the FileCheck happens before the NODUMP).

</2cents>

[sameerarora101]

Updating %ts and '='.

[jhenderson]

LGTM, once my two comments have been addressed. Also, please replace the PR reference with a real bug URL.

llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test

7

Nit: "before it is removed".

10

+1 - this is also my prefererred style

[sameerarora101]

llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test

10

ok, moving it down then.

[sameerarora101]

Putting CHECKs after the RUNs.

[sameerarora101]

Thanks for the reviews everyone 😊

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test

9

CHECK-NEXT:

[sameerarora101]

Updating test: CHECK -> CHECK-NEXT

[alexander-shaposhnikov]

LGTM

llvm/test/tools/llvm-objcopy/ELF/dump-section-before-add-remove.test

+# RUN: yaml2obj %s -o %t
+
+## Verify that section is dumped before it is removed.
+# RUN: llvm-objcopy --dump-section .test2=%t1.dump -R .test2 %t %t1
+# RUN: od -t x1 %t1.dump | FileCheck %s --ignore-case --match-full-lines
+
+# CHECK: 0000000 de ad be ef
+# CHECK-NEXT: 0000004
+
+## Verify that the newly added section is not dumped.
+# RUN: echo CAFE > %t3.txt
+# RUN: not llvm-objcopy --dump-section .test3=%t3.dump --add-section .test3=%t3.txt %t %t3 2>&1 | \
+# RUN:     FileCheck %s --check-prefix=NODUMP -DINPUT=%t
+
+# NODUMP: error: '[[INPUT]]': section '.test3' not found
+
+!ELF
+FileHeader:
+  Class:    ELFCLASS64
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+  Machine:  EM_X86_64
+Sections:
+  - Name:     .test1
+    Type:     SHT_PROGBITS
+    Flags:    [ ]
+  - Name:     .test2
+    Type:     SHT_PROGBITS
+    Flags:    [ ]
+    Content:  "DEADBEEF"
+  - Name:     .test4
+    Type:     SHT_PROGBITS
+    Flags:    [ ]
+    Content:  "BEEFDEAD"
+Symbols: []

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

 }
 
 static Error splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
                             StringRef File, ElfType OutputElfType) {
   auto DWOFile = Reader.create(false);
   auto OnlyKeepDWOPred = [&DWOFile](const SectionBase &Sec) {
     return onlyKeepDWOPred(*DWOFile, Sec);
   };
-  if (Error E = DWOFile->removeSections(Config.AllowBrokenLinks, 
+  if (Error E = DWOFile->removeSections(Config.AllowBrokenLinks,
                                         OnlyKeepDWOPred))
     return E;
   if (Config.OutputArch) {
     DWOFile->Machine = Config.OutputArch.getValue().EMachine;
     DWOFile->OSABI = Config.OutputArch.getValue().OSABI;
   }
   FileBuffer FB(File);
   auto Writer = createWriter(Config, *DWOFile, FB, OutputElfType);
   if (Error E = Writer->finalize())
     return E;
if ((!Config.SymbolsToKeep.empty() || Config.KeepFileSymbols) &&
     RemovePred = [&Obj, RemovePred](const SectionBase &Sec) {
       if (&Sec == Obj.SymbolTable || &Sec == Obj.SymbolTable->getStrTab())
         return false;
       return RemovePred(Sec);
     };
   }
 
   if (Config.CompressionType != DebugCompressionType::None)
-    replaceDebugSections(Obj, RemovePred, isCompressable, 
+    replaceDebugSections(Obj, RemovePred, isCompressable,
                          [&Config, &Obj](const SectionBase *S) {
                            return &Obj.addSection<CompressedSection>(
                                 *S, Config.CompressionType);
                          });
   else if (Config.DecompressDebugSections)
     replaceDebugSections(
         Obj, RemovePred,
         [](const SectionBase &S) { return isa<CompressedSection>(&S); },
         [&Obj](const SectionBase *S) {
           auto CS = cast<CompressedSection>(S);
           return &Obj.addSection<DecompressedSection>(*CS);
         });
if (Error E =
             splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType))
       return E;
 
   if (Config.OutputArch) {
     Obj.Machine = Config.OutputArch.getValue().EMachine;
     Obj.OSABI = Config.OutputArch.getValue().OSABI;
   }
 
+  // Dump sections before add/remove for compatibility with GNU objcopy.
+  for (StringRef Flag : Config.DumpSection) {
+    StringRef SectionName;
+    StringRef FileName;
+    std::tie(SectionName, FileName) = Flag.split('=');
+    if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
+      return E;
+  }
+
   // It is important to remove the sections first. For example, we want to
   // remove the relocation sections before removing the symbols. That allows
   // us to avoid reporting the inappropriate errors about removing symbols
   // named in relocations.
   if (Error E = replaceAndRemoveSections(Config, Obj))
     return E;
 
   if (Error E = updateAndRemoveSymbols(Config, Obj))
ArrayRef<uint8_t> Data(
         reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
         Buf->getBufferSize());
     OwnedDataSection &NewSection =
         Obj.addSection<OwnedDataSection>(SecName, Data);
     if (SecName.startswith(".note") && SecName != ".note.GNU-stack")
       NewSection.Type = SHT_NOTE;
   }
 
-  for (const auto &Flag : Config.DumpSection) {
-    std::pair<StringRef, StringRef> SecPair = Flag.split("=");
-    StringRef SecName = SecPair.first;
-    StringRef File = SecPair.second;
-    if (Error E = dumpSectionToFile(SecName, File, Obj))
-      return E;
-  }
-
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
   for (const NewSymbolInfo &SI : Config.ELF->SymbolsToAdd) {
     SectionBase *Sec = Obj.findSection(SI.SectionName);
     uint64_t Value = Sec ? Sec->Addr + SI.Value : SI.Value;
     Obj.SymbolTable->addSymbol(