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

Tue Jul 13 00:30:37 PDT 2021

jhenderson added a comment.

I'm very cold on this review, but hopefully my comments are useful. Clearly someone with wasm knowledge should make sure to sign off on the behavioural changes, since I have no real knowledge there.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:6
+
+# CHECK: Sections:
+# CHECK:        Name: .debug_info
----------------
Nit: we tend to add spaces to make things line up with where they would be if there were a NEXT suffix (when there are suffixes later in the list of checks).

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:10
+
+## Also Test that only-section overrides remove-section.
+# RUN: llvm-objcopy --only-section=producers --remove-section=producers %t %t2
----------------
Nit

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:12
+# RUN: llvm-objcopy --only-section=producers --remove-section=producers %t %t2
+# RUN: obj2yaml %t2
+# RUN: obj2yaml %t2 | FileCheck --implicit-check-not linking %s
----------------
Looks like this `obj2yaml` line isn't necessary?

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:16-19
+# CHECK: Sections:
+# CHECK-NEXT: - Type: CUSTOM
+# CHECK-NEXT:   Name: producers
+# CHECK-NEXT:   Tools:
----------------

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:21
+
+
+## Test that only-section + keep-section keeps both sections
----------------
Nit: we don't need all these extra blank lines: the `##` comment markers act as a good way of dividing up the test cases.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:22
+
+## Test that only-section + keep-section keeps both sections
+# RUN: llvm-objcopy --only-section=producers --keep-section=linking %t %t2
----------------

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:29-30
+
+
+
+--- !WASM
----------------
Nit: delete the extra blank lines here.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-strip.test:1
+## Test that debug, name, and producers sections are stripped with --strip-all.
+# RUN: yaml2obj %s -o %t
----------------
Not being familiar with WASM, are "name" and "producers" actually special kinds of sections, or are they just arbitrary section names for CUSTOM section types?

Perhaps also worth mentioning that other CUSTOM sections are not stripped?

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-strip.test:10-12
+# CHECK: Sections:
+# CHECK-NEXT: - Type: TYPE
+# CHECK:        Name: foo
----------------

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test:16
+
+# CHECK: Sections:
+# CHECK-NEXT: - Type: CUSTOM
----------------
Is this the full list of sections? If so, would we be better off with an `--implicit-check-not=Name:` or similar, to show that no other sections are in the list, than the need for -SAME? (I know I suggested the latter - sorry!)

Also: nit: add spaces, similar to other locations mentioned.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test:20
+# CHECK:      - Type:
+# CHECK-SAME: CUSTOM
+# CHECK-NEXT:   Name: .debug_line
----------------
Personal opinion: I find the inline edit suggestion easier to follow (i.e. it's at the same vertical position as it would be if the line was all one line).

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test:26-27
+
+
+
+--- !WASM
----------------
Remove unnecessary extra blank lines.

================
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
+
----------------
Similar to my above comment: if we're removing all but the sections explicitly checked for in the list below, you could simplify this by using `--implicit-check-not=Type:` or similar (which would avoid the need for the other implicit checks).

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-debug.test:6
+
+# CHECK: Sections:
+# CHECK-NEXT: - Type: TYPE
----------------
Add additional spaces, as above.

================
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
----------------
dschuff wrote:
> jhenderson wrote:
> > 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).
> I checked, and the same number of tests are discovered when the extension is .yaml or .test. So if you want I can rename all of the .test tests to .yaml in a followup change.
I don't care either way - if there's a desire to do it, I don't oppose doing so.

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:17
+# CHECK: Functions:
+## And check that there are no more sections.
+# CHECK-NOT: - Name:
----------------
Maybe replace this with an `--implicit-check-not=Type:`

================
Comment at: llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test:20
+
+
+--- !WASM
----------------
Delete extra blank line.

================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:79
+      // Remove everything else, including known sections.
+      return !isDebugSection(Sec);
+    };
----------------
dschuff wrote:
> jhenderson wrote:
> > Looks like --only-keep-debug + --remove-section=.debug_info etc will result in .debug_info being kept? Is that consistent with other formats? It feels like in this case, .debug_info should be stripped, as you explicitly requested that section be removed.
> Actually this logic was copied directly from ELFObjcopy.cpp:556
> I agree that it does seem to make more sense to do it your suggested way though. Do you think it's better to be consistent, or to do it that way?
For ELF, we should probably do what GNU objcopy does. I don't know what, if any, precedent there is for wasm though. If there is no precedent, I'd do what ELF does (and if ELFObjcopy.cpp does it wrong compared to GNU, then do what GNU does, and fix ELF or file a bug for it).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820