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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Quite a number of test issues, hence requesting the changes to prevent premature commit. No major functional issues that I could see though.



================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:7
+# CHECK: Sections:
+# CHECK-NEXT: - Type: TYPE
+# CHECK:        Name: .debug_info
----------------
Is this first check here needed (and indeed the TYPE section)? If I understand things correctly, TYPE sections are always kept, but this test is about the --keep-section switch, so we only really need CUSTOM sections, right - one to show it was kept and another to show that the others were not kept.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:10-11
+# CHECK-NEXT:   Payload: DEADBEEF
+## Ensure that there are no more sections after .debug_info.
+# CHECK-NOT: Type
+
----------------
This doesn't show that there are no more sections BEFORE .debug_info though. You might want something more like --implicit-check-not to verify that.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:18-20
+# REMOVE: - Type: TYPE
+# REMOVE: - Type: CUSTOM
+# REMOVE:   Name: producers
----------------
These sections aren't really relevant to this test case, I'd think.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:3
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --only-section=producers %t %t2
+# RUN: obj2yaml %t2 | FileCheck %s
----------------
Test case for --only-section + --keep-section? Also --only-section + --remove-section.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:6
+
+# This file has both known and custom sections. Check that only the producers section is left.
+# CHECK: Sections:
----------------
'##'


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-strip.test:12
+# CHECK-NEXT: - Type: TYPE
+# CHECK:        Name: producers
+# CHECK-NOT: .debug_info
----------------
producers appears to be a section of type CUSTOM, yet it isn't being stripped, which doesn't match up with your comment at the top of the file?


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-strip.test:13
+# CHECK:        Name: producers
+# CHECK-NOT: .debug_info
+
----------------
This only shows that there are no .debug_info sections after the producers section. Use --implicit-check-not=".debug_info" isntead to show that it applies anywhere.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test:1
+## Test that only debug sections are kept with --only-keep-debug.
+# RUN: yaml2obj %s -o %t
----------------
Maybe worth adding an --only-keep-debug + --keep-section test case?


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test:9
+# CHECK-NEXT:   Name: .debug_info
+# CHECK:      - Type: CUSTOM
+# CHECK-NEXT:   Name: .debug_line
----------------
Consider using the following instead here:
```
# CHECK: - Type:
# CHECK-SAME: CUSTOM
```
This ensures that we fetch the next section, and that that section is of type CUSTOM, whereas the current behaviour only shows that the next CUSTOM section is .debug_line (there could be a non-CUSTOM type section in between).


================
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:
> 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.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:26
+static bool isDebugSection(const Section &Sec) {
+  return Sec.Name.startswith(".debug") || Sec.Name == "name";
+}
----------------
I don't see a section with name "name" anywhere being stripped via --strip-debug.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:30
+static bool isLinkerSection(const Section &Sec) {
+  return Sec.Name.startswith("reloc.") || Sec.Name == "linking";
+}
----------------
I don't see this code being tested at all.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:77
+    RemovePred = [](const Section &Sec) {
+      // Explictily keep debug sections regardless of previous removes.
+      // Remove everything else, including known sections.
----------------
Typo: Explictily -> Explicitly


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:79
+      // Remove everything else, including known sections.
+      return !isDebugSection(Sec);
+    };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820