[PATCH] D150305: ObjCopy: support `--dump-section` on COFF

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Requesting changes, just to stop this landing before my comments have been addressed. I don't see any serious issues, just a couple of stylistic things and some missing testing.



================
Comment at: llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp:135
+  for (const coff::Section &Section : O.getSections()) {
+    if (Section.Name != S) continue;
+
----------------
This doesn't look to conform to standard LLVM coding style. Please make sure you have run clang-format on your code.


================
Comment at: llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp:152
+  return createStringError(errc::no_such_file_or_directory,
+                           "Section '%s' not found", S.str().c_str());
+}
----------------
Please see the coding standards for errors and warning messages (use lower-case "s" for this message): https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:1
+# RUN: yaml2obj %s -o %t.obj
+# RUN: llvm-objcopy --dump-section .data=%t.dat %t.obj
----------------
Please add the following two test cases: 1) the section does not exist; 2) the file cannot be created (e.g. because you're trying to create a file with the same path as an existing directory).

Check that llvm-objcopy emits an error with the expected message in these cases.

It may also be interesting to show that the contents of an existing file on disk are overwritten if it is the target of `--dump-section`.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:16
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
----------------
I'm not too familiar with coff yaml2obj, so my comments may not be applicable, but can you drop most of these sections from the YAML? I think you only need two sections (one empty and one non-empty). If possible, I'd even name those sections accordingly (i.e. something like ".empty" and ".nonempty"), and remove their characteristics, since they aren't relevant to the behaviour of llvm-objcopy.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:42
+symbols:
+  - Name:            .text
+    Value:           0
----------------
Similar to the above, if you can, please remove all the symbols (or at least the ones that aren't required).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150305