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

Mon Jul 12 17:16:55 PDT 2021

dschuff added a comment.

I went over all the tests and I think I've addressed all the issues.

There is one functional change since the last round: namely that the `name` section is now not removed by `--strip-debug`. That may change based on the outcome of the discussion in https://github.com/emscripten-core/emscripten/issues/14623 but it's trivial to change it one way or the other so we don't need to block this change on that.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820