[PATCH] D106942: Consider section flags when adding section

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/set-flags-on-setion-added.test:7
+# RUN: llvm-objcopy --set-section-flags=.test.section1=code --add-section=.test.section1=%t.sec --set-section-flags=.test.section2=data --add-section=.test.section2=%t.sec %t %t1
+# RUN: llvm-readobj --sections --section-data %t1 | FileCheck %s --check-prefixes=CHECK-ADD
+
----------------
There's only one test case in this file, so there's no need for the --check-prefixes argument. Just use the default CHECK for your check prefix throughout the file.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/set-flags-on-setion-added.test:9-24
+# CHECK-ADD:    Name: .test.section1
+# CHECK-ADD-NEXT:    VirtualSize: 0x9
+# CHECK-ADD-NEXT:    VirtualAddress: 0x0
+# CHECK-ADD-NEXT:    RawDataSize: 9
+# CHECK-ADD:      IMAGE_SCN_CNT_CODE (0x20)
+# CHECK-ADD-NEXT:      IMAGE_SCN_MEM_EXECUTE (0x20000000)
+# CHECK-ADD-NEXT:      IMAGE_SCN_MEM_READ (0x40000000)
----------------
Let's clean up the spacing so that the values all line up nicely.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/set-flags-on-setion-added.test:13
+# CHECK-ADD-NEXT:    RawDataSize: 9
+# CHECK-ADD:      IMAGE_SCN_CNT_CODE (0x20)
+# CHECK-ADD-NEXT:      IMAGE_SCN_MEM_EXECUTE (0x20000000)
----------------
Is there a reason you don't check the full set of Characteristics here (i.e. including the title etc)? Something like the following:
```
# CHECK:       Characteristics [
# CHECK-NEXT:   IMAGE_SCN_CNT_CODE (0x20)
# CHECK-NEXT:   IMAGE_SCN_MEM_EXECUTE (0x20000000)
# CHECK-NEXT:   IMAGE_SCN_MEM_READ (0x40000000)
# CHECK-NEXT:   IMAGE_SCN_MEM_WRITE (0x80000000)
# CHECK-NEXT: ]
```
This will ensure no spurious flags have been set too.

Same applies for the second section.


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

https://reviews.llvm.org/D106942