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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

Well it's been an interesting 6 months since I last looked at this patch!
Thanks for the thorough review, I think I've addressed all the feedback, please take a look.



================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:7
+# CHECK: Sections:
+# CHECK-NEXT: - Type: TYPE
+# CHECK:        Name: .debug_info
----------------
jhenderson wrote:
> 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.
I made a change to the behavior (see my reply to sbc below) so I made  a new test (strip-all.test) checking for the presence of an unknown custom section and the absence of the debug, linking and producers sections.
Now this test is just as you suggested, using implicit-check-not.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-keep.test:18-20
+# REMOVE: - Type: TYPE
+# REMOVE: - Type: CUSTOM
+# REMOVE:   Name: producers
----------------
jhenderson wrote:
> These sections aren't really relevant to this test case, I'd think.
They are relevant to the behavior of strip-all but now that's in another test.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-strip.test:12
+# CHECK-NEXT: - Type: TYPE
+# CHECK:        Name: producers
+# CHECK-NOT: .debug_info
----------------
jhenderson wrote:
> 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?
Tweaked the behavior, and made the comment match.


================
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
----------------
jhenderson wrote:
> Maybe worth adding an --only-keep-debug + --keep-section test case?
Done. Also added an unknown custom section and checked that it gets removed.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:30
+static bool isLinkerSection(const Section &Sec) {
+  return Sec.Name.startswith("reloc.") || Sec.Name == "linking";
+}
----------------
jhenderson wrote:
> I don't see this code being tested at all.
Linking is now relevant for --strip-all, so that part is tested in several other tests now.
For "name" and "reloc" I added a new test (which has to be a valid object because yaml2obj understands relocs and name sections).


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


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:67
+    RemovePred = [RemovePred](const Section &Sec) {
+      return RemovePred(Sec) || Sec.SectionType == llvm::wasm::WASM_SEC_CUSTOM;
+    };
----------------
sbc100 wrote:
> ```
>        -S
>        --strip-all
>            Do not copy relocation and symbol information from the source file.
> 
>        -g
>        --strip-debug
>            Do not copy debugging symbols or sections from the source file.
> ```
> 
> Are you sure we want to remove all custom sections.. seems like strip-all relates mostly to symbols/relocations/debugging, not general custom sections.
I had been thinking that strip-all also removed all other things like .comment and .note sections but looking again it's even a bit more subtle. e.g. it seems to remove .comment sections but not .note sections (which sort of makes sense since those can specify an ABI AFAIU).

So how about the behavior that strip-all removes all debug and linking sections, and sections which are known to be just informational (such as producers), but no unknown custom sections? (since unknown custom sections could be inserted by a frontend for its own purposes, and be required). 


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:86
+        return false;
+      // Remove everything else, inluding known sections.
+      return true;
----------------
sbc100 wrote:
> Ditto.. or is this some kind of pattern being followed from ELF?
No, I probably just copied it from ELF and simplified it by removing irrelevant stuff, but failed to simplify the structure.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:139
       !Config.SymbolsToAdd.empty() || !Config.RPathToAdd.empty() ||
-      !Config.OnlySection.empty() || !Config.SymbolsToGlobalize.empty() ||
-      !Config.SymbolsToKeep.empty() || !Config.SymbolsToLocalize.empty() ||
+      !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToLocalize.empty() ||
       !Config.SymbolsToRemove.empty() ||
----------------
sbc100 wrote:
> Did we loose SymbolsToKeep by accident here?
Oops, yes. Or maybe I was thinking SectionsToKeep.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73820