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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test:21
+
+
+## Test that only-section + keep-section keeps both sections
----------------
jhenderson wrote:
> Nit: we don't need all these extra blank lines: the `##` comment markers act as a good way of dividing up the test cases.
Even with the ## markers, I still find it much more readable with a single blank line (although I agree that more than one isn't necessary).


================
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
----------------
jhenderson wrote:
> 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?
The Wasm spec has a distinction between "known" sections (i.e. those that affect execution) and "custom" sections (those that don't). Known section names are specified, and are represented in caps. Custom sections have arbitrary names and are ignored by engines. But tool ecosystems also have designated custom sections that they write and/or read for particular purposes. LLVM generates "name" and "[[ https://github.com/WebAssembly/tool-conventions/blob/master/ProducersSection.md | producers ]]", as well as "[[ https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md | linking ]]" and "reloc.foo" (consumed by the linker) and ".debug_info" and friends for debugging.
So for the purposes of LLVM, these are handled specially (e.g. obj2yaml knows how to read and write them).
I added a comment about sections unknown to LLVM


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test:16
+
+# CHECK: Sections:
+# CHECK-NEXT: - Type: CUSTOM
----------------
jhenderson wrote:
> 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.
Ah, I didn't realize you could mix `--implicit-check-not` with a check for the same string. 


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:79
+      // Remove everything else, including known sections.
+      return !isDebugSection(Sec);
+    };
----------------
jhenderson wrote:
> 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).
LLVM objcopy has the same behavior as GNU objcopy with `--only-keep-debug --remove-section=.debug_info`, with `.debug_info` being removed. (It's not 100% comparable because `--only-keep-debug` just changes section flags in ELF rather than removing sections.) 
But I changed the wasm behavior to have `--remove-section` override `--only-keep-debug` which makes more sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820