[PATCH] D65040: [llvm-objcopy] Add support for --add-section for COFF

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please make sure to update the documentation at llvm/docs/CommandGuide/llvm-objcopy.rst to move this switch out of the ELF-specific directory.

Please add a test case for trying to add a section with contents from a non-existent file. Also a test case where the argument format is invalid, e.g. `--add-section foo` (i.e. no '=' sign).



================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:6-12
+# CHECK: SectionCount: 5
+# CHECK: Name: .text
+# CHECK: Name: .bss
+# CHECK: Name: .comdat
+# CHECK: Name: .associative
+# CHECK: Name: .test.section
+# CHECK:    Characteristics [
----------------
Nit: It would be a bit easier to read if these had some extra spaces to make the whole lot line up with the block below. Also, the Characteristics block being indented further seems weird to me. I think the following would look better:

```
# CHECK:      Name: .test.section
# CHECK:      Characteristics [
# CHECK-NEXT:   IMAGE_SCN_ALIGN_1BYTES
# CHECK-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
# CHECK-NEXT: ]
```


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:16
+# CHECK-NEXT:    ]
+# CHECK:    SectionData (
+# CHECK-NEXT:      0000: {{.+}}|DEADBEEF{{.+}}|
----------------
Same comment applies here as above.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:26-32
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     488B0500000000C3
+    Relocations:
+      - VirtualAddress:  3
+        SymbolName:      foo
+        Type:            IMAGE_REL_AMD64_REL32
----------------
I suspect you can delete all of this. Let's keep the YAML to the minimum required for this test.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:33-36
+  - Name:            .bss
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     ''
----------------
You can probably delete this and most other pre-existing sections.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:45
+    SectionData:     '0000000000000000'
+symbols:
+  - Name:            '@feat.00'
----------------
I'm guessing you don't need these symbols for this test case.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:175
+  for (const auto &Flag : Config.AddSection) {
+    StringRef SecName, FileName;
+    std::tie(SecName, FileName) = Flag.split("=");
----------------
Rather than having a big block of code that is broadly similar to the addGnuDebugLink code, I wonder if some of the body of this loop and that function could be factored out into a function, which takes an Object, some section contents, and maybe one or two other parameters to do with addresses and adds a section to that Object.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65040