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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp:133
 
+static Error dumpSection(Object &O, StringRef S, StringRef File) {
+  for (const coff::Section &Section : O.getSections()) {
----------------
compnerd wrote:
> hjyamauchi-test wrote:
> > nit: "S" -> "Section" to match "File"?
> That collides with the iterator - I can use `SectionName` but that also is different from `File` ... I guess I can do `SectionName` and `FileName`.
+1 to `SectionName` and `FileName`. I think this nicely improves clarity.


================
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());
+}
----------------
compnerd wrote:
> jhenderson wrote:
> > 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
> Probably should do the same in the ELF path :)
By all means do so (in a separate patch). I no longer directly work on LLVM, due to a change in job roles. I just review stuff :)


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:6-7
+# RUN: od -t x1 %t.txt | FileCheck %s --ignore-case --check-prefix CHECK-TEXT-F
+# RUN: not llvm-objcopy --dump-section non-existent=/dev/null %t.obj 2>&1 | FileCheck %s -check-prefix CHECK-NO-SECTION
+# RUN: not llvm-objcopy --dump-section .text=%T %t.obj 2>&1 | FileCheck %s --ignore-case -check-prefix CHECK-INVALID-DESTINATION
+
----------------
Consistency fixes, and no need to ignore the case. Instead, use the %errc... substitutions, if needed (you might need to extend the list of available ones to cover the "is a directory" case).

Also `%T` is considered deprecated (from the docs: "%T 	parent directory of %t (not unique, deprecated, do not use)"). You can use `%S` instead in this case.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:13-15
+# CHECK-NO-SECTION: section 'non-existent' not found
+
+# CHECK-INVALID-DESTINATION: : is a directory
----------------
Could you check the full error message, please (less any tool name), using FileCheck -D to parameterise any file names. In particular, this will show that this is an error, and not a warning.


================
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
----------------
compnerd wrote:
> jhenderson wrote:
> > 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`.
> Should probably add those to the ELF tests as well :)
Same comment as above :)


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

https://reviews.llvm.org/D150305