[PATCH] D73820: [llvm-strip][WebAssembly] Support strip flags

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've done a partial review to keep you going. I'm out of time to look at this further today though, especially given how cold I am on the patch. Many of my comments apply to multiple tests, even if I haven't got to those tests.



================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:9
 # CHECK-NEXT:   Payload: DEADBEEF
-## Ensure that there are no more sections after .debug_info.
-# CHECK-NOT: Type
+#
 
----------------
Nit: Remove this line.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:15-17
+# KEEP: Sections:
+# KEEP: - Type: CUSTOM
+# KEEP:   Name: .debug_info
----------------
Are these lines adjacent in the output? If so, you should use `KEEP-NEXT` for the second and third lines, as it makes diagnosing issues easier, and prevent situations where the Type is for an unrelated section to the one with the Name.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-all.test:5
+# RUN: llvm-objcopy --strip-all %t %t2
+# RUN: obj2yaml %t2 | FileCheck --implicit-check-not=linking --implicit-check-not=.debug --implicit-check-not producers %s
 
----------------
Aside: isn't there llvm-readobj support for dumping wasm? That's how we usually test the output is correct in other testing.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-all.test:9
 # CHECK-NEXT: - Type: TYPE
-# CHECK:      - Type: CUSTOM
-# CHECK-NEXT:   Name: producers
-## Ensure that there are no more sections after producers.
-# CHECK-NOT: Type
+# CHECK:        Name: foo
+
----------------
Is this a separate section? If so, you probably want to make that clear, by checking e.g. the Type field for it too. It might be worth considering checking the other properties of the kept sections too, though I'm less sure about that.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-all.test:10
+# CHECK:        Name: foo
+
 
----------------
Nit: delete extra blank line.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-all.test:37
+    Payload: CAFE
\ No newline at end of file

----------------
Nit: no new line at EOF.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-debug.test:40
+    Payload: DEADBEEF
\ No newline at end of file

----------------
Nit: missing new line at EOF.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-debug.test:1
+## Test that debug sections are stripped with --strip-debug
+# RUN: yaml2obj %s -o %t
----------------
jhenderson wrote:
> dschuff wrote:
> > sbc100 wrote:
> > > Can you do a followup that renames all the `.test` files in this directory to `.yaml` .. its more descriptive and enables syntax highlighting .
> > Maybe? ELF and COFF use .test instead of .yaml
> I have no problem with ELF and COFF tests being renamed too, but beware that by default, lit doesn't recognise files ending with `.yaml` extensions as tests.
> I have no problem with ELF and COFF tests being renamed too, but beware that by default, lit doesn't recognise files ending with `.yaml` extensions as tests.

I think that this comment doesn't apply any more - try running lit in the directory, with the tests renamed, and see what happens (don't explicitly specify the test themselves, as lit will then run them regardless of their extension).


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:2
+## Test that reloc sections are stripped by --strip-all.
+## Test that name sections are stripped by --strip-debug.
+
----------------
It might be better to split these two up into separate tests. I often take the approach of one option/significant behaviour -> one or more tests, as it helps separate concerns.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:11
+
+# Check that the known sections are still present
+# CHECK: Sections:
----------------
Nit: here and below - `##` for comments, and missing trailing full stops.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:26
+# RUN: llvm-objcopy --strip-debug %t %t2
+# RUN: obj2yaml %t2 | FileCheck --implicit-check-not=FunctionNames --check-prefix=NAME  %s
+
----------------
Nit: there's a double space before `%s`.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:32
+
+--- !WASM
+FileHeader:
----------------
This YAML doesn't look particularly minimal to me, or it at least has way too much padding within the YAML. Consider which of the properties are actually required for your test, I recommend, and delete the rest.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:90
+         Name:           foo
\ No newline at end of file

----------------
Nit: add new line at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820