[PATCH] D75949: [llvm-objcopy] Allow empty sections in --dump-section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Ideally, we'd fix yaml2obj for Mach-O first, before checking this in without a test.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-section-empty.test:22
+    Flags:           [ SHF_WRITE ]
+  - Name:            .nobits
+    Type:            SHT_NOBITS
----------------
To emphasise the point, this should probably have a non-zero size.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-section.test:6
 # RUN: llvm-objcopy --dump-section .foo=%t6 %t %t7
-# RUN: not llvm-objcopy --dump-section .bar=%t8 %t %t9 2>&1 | FileCheck %s --check-prefix=NOBITS -DINPUT=%t
 # RUN: od -t x1 %t2 | FileCheck %s --ignore-case
----------------
I'm not sure I agree with removing this test from here. A nobits section is not dumpable because it is nobits, not because it is empty.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:291
     if (Sec.Name == SecName) {
-      if (Sec.OriginalData.empty())
+      // FIXME: this should be a warning, not an error.
+      if (Sec.Type == SHT_NOBITS)
----------------
Is this really a FIXME? It seems like it should be an error here, because llvm-objcopy cannot do something you've explicitly asked it to do.

I don't know what GNU objcopy does, but I think this is an okay place to diverge.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75949/new/

https://reviews.llvm.org/D75949