[llvm-strip][WebAssembly] Support strip flags

[dschuff]

Summary

Add support for the basic section stripping (and keeping) flags for wasm:
strip with no flags, --strip-all, --strip-debug,
--only-section, --keep-section, and --only-keep-debug.

Factor section removal into a function and use a predicate chain like
the ELF implementation.

Depends on D70970

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[sbc100]

Mostly lgtm with some comments.

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

7 ↗

(On Diff #241843)

So strip-all doesn't apply to the TYPE section?

11 ↗

(On Diff #241843)

What is this NOT checking for?

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

11 ↗

(On Diff #241843)

ditto above, what is this checking for?

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

Can you do a followup that renames all the .test files in this directory to .yaml .. its more descriptive and enables syntax highlighting .

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

106

Interesting technique of chaining the predicates together like this using lamda. Might be a little too clever? Do other object backends use this too?

[jhenderson]

I'll take a look at this in the coming days. Got a lot on my plate at the moment!

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

106

This is how it is done in ELF objcopy, and I think maybe the others too.

[dschuff]

rebase, add comments to test expectations

[dschuff]

Updated, please take a look.

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

7 ↗

(On Diff #241843)

Currently objcopy only ever touches custom sections. To me it's an open question whether it makes sense to allow it to do anything to the known sections, since they have to be updated together, with a lot more knowledge about wasm.

11 ↗

(On Diff #241843)

Checks that there are no more sections after foo. I'll add a comment.

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

11 ↗

(On Diff #241843)

Added comment here too.

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

Maybe? ELF and COFF use .test instead of .yaml

[sbc100]

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

76

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

83

Just return !isDebugSection(Sec))?

95

Ditto.. or is this some kind of pattern being followed from ELF?

138–139

Did we loose SymbolsToKeep by accident here?

[dschuff]

Don't remove all custom sections with --strip-ally

[sbc100]

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

22 ↗

(On Diff #244552)

Maybe this should be called KEEP because its verifying that something is being kept?

[jhenderson]

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

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

7 ↗

(On Diff #244552)

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.

[jhenderson]

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

10–11 ↗

(On Diff #244552)

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.

18–20 ↗

(On Diff #244552)

These sections aren't really relevant to this test case, I'd think.

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

3 ↗

(On Diff #244552)

Test case for --only-section + --keep-section? Also --only-section + --remove-section.

6 ↗

(On Diff #244552)

'##'

llvm/test/tools/llvm-objcopy/wasm/basic-strip.test

12 ↗

(On Diff #244552)

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?

13 ↗

(On Diff #244552)

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.

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

1 ↗

(On Diff #244552)

Maybe worth adding an --only-keep-debug + --keep-section test case?

9 ↗

(On Diff #244552)

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).

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

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.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

25

I don't see a section with name "name" anywhere being stripped via --strip-debug.

29

I don't see this code being tested at all.

82

Typo: Explictily -> Explicitly

84

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.

[dschuff]

Address feedback

[dschuff]

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.

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

7 ↗

(On Diff #244552)

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.

18–20 ↗

(On Diff #244552)

They are relevant to the behavior of strip-all but now that's in another test.

llvm/test/tools/llvm-objcopy/wasm/basic-strip.test

12 ↗

(On Diff #244552)

Tweaked the behavior, and made the comment match.

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

1 ↗

(On Diff #244552)

Done. Also added an unknown custom section and checked that it gets removed.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

29

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).

76

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).

84

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?

95

No, I probably just copied it from ELF and simplified it by removing irrelevant stuff, but failed to simplify the structure.

138–139

Oops, yes. Or maybe I was thinking SectionsToKeep.

[jhenderson]

I've done a partial review to keep you going. I'm out of time to look at this further today though, especially given how cold I am on the patch. Many of my comments apply to multiple tests, even if I haven't got to those tests.

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

9 ↗

(On Diff #292559)

Nit: Remove this line.

15–17 ↗

(On Diff #292559)

Are these lines adjacent in the output? If so, you should use KEEP-NEXT for the second and third lines, as it makes diagnosing issues easier, and prevent situations where the Type is for an unrelated section to the one with the Name.

llvm/test/tools/llvm-objcopy/wasm/strip-all.test

5 ↗

(On Diff #292559)

Aside: isn't there llvm-readobj support for dumping wasm? That's how we usually test the output is correct in other testing.

9 ↗

(On Diff #292559)

Is this a separate section? If so, you probably want to make that clear, by checking e.g. the Type field for it too. It might be worth considering checking the other properties of the kept sections too, though I'm less sure about that.

10 ↗

(On Diff #292559)

Nit: delete extra blank line.

37 ↗

(On Diff #292559)

Nit: no new line at EOF.

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

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).

40 ↗

(On Diff #292559)

Nit: missing new line at EOF.

llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test

2 ↗

(On Diff #292559)

It might be better to split these two up into separate tests. I often take the approach of one option/significant behaviour -> one or more tests, as it helps separate concerns.

11 ↗

(On Diff #292559)

Nit: here and below - ## for comments, and missing trailing full stops.

26 ↗

(On Diff #292559)

Nit: there's a double space before %s.

32 ↗

(On Diff #292559)

This YAML doesn't look particularly minimal to me, or it at least has way too much padding within the YAML. Consider which of the properties are actually required for your test, I recommend, and delete the rest.

90 ↗

(On Diff #292559)

Nit: add new line at EOF.

[sbc100]

What is the status of this patch? I think it would be useful to get this landed still.

[dschuff]

There are still outstanding comments after the last round, I just haven't had time to get back to it yet and it wasn't on the top of the list. I can bump the priority if you have need for it though?

[dschuff]

rebase

[dschuff]

fix diff

[dschuff]

• review round 3

[dschuff]

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.

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

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.

[jhenderson]

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.

llvm/test/tools/llvm-objcopy/wasm/basic-keep.test

6 ↗

(On Diff #358106)

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).

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

10 ↗

(On Diff #358106)

Nit

12 ↗

(On Diff #358106)

Looks like this obj2yaml line isn't necessary?

16–19 ↗

(On Diff #358106)

21 ↗

(On Diff #358106)

Nit: we don't need all these extra blank lines: the ## comment markers act as a good way of dividing up the test cases.

22 ↗

(On Diff #358106)

29–30 ↗

(On Diff #358106)

Nit: delete the extra blank lines here.

llvm/test/tools/llvm-objcopy/wasm/basic-strip.test

1 ↗

(On Diff #358106)

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?

10–12 ↗

(On Diff #358106)

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

16 ↗

(On Diff #358106)

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.

20 ↗

(On Diff #358106)

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).

26–27 ↗

(On Diff #358106)

Remove unnecessary extra blank lines.

llvm/test/tools/llvm-objcopy/wasm/strip-all.test

5 ↗

(On Diff #358106)

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).

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

I don't care either way - if there's a desire to do it, I don't oppose doing so.

6 ↗

(On Diff #358106)

Add additional spaces, as above.

llvm/test/tools/llvm-objcopy/wasm/strip-reloc.test

17 ↗

(On Diff #358106)

Maybe replace this with an --implicit-check-not=Type:

20 ↗

(On Diff #358106)

Delete extra blank line.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

84

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).

[sbc100]

lgtm with comments addressed

llvm/test/tools/llvm-objcopy/wasm/strip-debug.test

1 ↗

(On Diff #241843)

I agree that landing this with the same convention as the surrounding files makes sense.

Whether you want to do the mass renaming before or after this lands I don't this it matters.

[dschuff]

• address more comments

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

21 ↗

(On Diff #358106)

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).

llvm/test/tools/llvm-objcopy/wasm/basic-strip.test

1 ↗

(On Diff #358106)

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 "producers", as well as "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

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

16 ↗

(On Diff #358106)

Ah, I didn't realize you could mix --implicit-check-not with a check for the same string.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

84

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.

[jhenderson]

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

21 ↗

(On Diff #358106)

Yes, single line is fine. Sorry if that wasn't clear (2+ lines is where I complain :) )

I tend to use the following pattern:

## Top-level test file comment.

## Test-case comment.
# RUN: <test case1> command
# RUN: <test case1> command2

# CHECK1: ...
# CHECK1: ...

<YAML>

## Test case 2 comment.
# ...

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

12 ↗

(On Diff #358414)

Spacing looks off on this line?

llvm/test/tools/llvm-objcopy/wasm/strip-all.test

7 ↗

(On Diff #358414)

Spacing?

[jhenderson]

LGTM, aside from the mentioned nits. Please wait for someone else with wasm understanding to approve though.

[dschuff]

Thanks!

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

 namespace llvm {
 namespace objcopy {
 namespace wasm {
 
 using namespace object;
 using SectionPred = std::function<bool(const Section &Sec)>;
 
 static bool isDebugSection(const Section &Sec) {
   return Sec.Name.startswith(".debug") || Sec.Name == "name";
 }
 
 static bool isLinkerSection(const Section &Sec) {
   return Sec.Name.startswith("reloc.") || Sec.Name == "linking";
 }
 
 // Sections which are known to be "comments" or informational and do not affect
 // program semantics.
 static bool isCommentSection(const Section &Sec) {
   return Sec.Name == "producers";
 }
 
if (Sec.Name == SecName) {
         return E;
       return Error::success();
     }
   }
   return createStringError(errc::invalid_argument, "section '%s' not found",
                            SecName.str().c_str());
 }
 
-static void removeSections(const CopyConfig &Config, Object &Obj) {
+static void removeSections(const CommonConfig &Config, Object &Obj) {
   SectionPred RemovePred = [](const Section &) { return false; };
 
   // Explicitly-requested sections.
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config](const Section &Sec) {
       return Config.ToRemove.matches(Sec.Name);
     };
   }
 
   if (Config.StripDebug) {
     RemovePred = [RemovePred](const Section &Sec) {
       return RemovePred(Sec) || isDebugSection(Sec);
     };
   }
 
   if (Config.StripAll) {
     RemovePred = [RemovePred](const Section &Sec) {
       return RemovePred(Sec) || isDebugSection(Sec) || isLinkerSection(Sec) ||
              isCommentSection(Sec);
     };
   }
 
   if (Config.OnlyKeepDebug) {
     RemovePred = [](const Section &Sec) {
       // Explicitly keep debug sections regardless of previous removes.
       // Remove everything else, including known sections.
       return !isDebugSection(Sec);
     };
   }
 
   if (!Config.OnlySection.empty()) {
     RemovePred = [&Config](const Section &Sec) {
       // Explicitly keep these sections regardless of previous removes.
       // Remove everything else, inluding known sections.
       return !Config.OnlySection.matches(Sec.Name);
     };
   }
 
   if (!Config.KeepSection.empty()) {
     RemovePred = [&Config, RemovePred](const Section &Sec) {
       // Explicitly keep these sections regardless of previous removes.
       if (Config.KeepSection.matches(Sec.Name))
         return false;
       // Otherwise defer to RemovePred.
       return RemovePred(Sec);
     };
   }
 
   Obj.removeSections(RemovePred);
 }
 
-static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+static Error handleArgs(const CommonConfig &Config, Object &Obj) {
   // Only support AddSection, DumpSection, RemoveSection for now.
   for (StringRef Flag : Config.DumpSection) {
     StringRef SecName;
     StringRef FileName;
     std::tie(SecName, FileName) = Flag.split("=");
     if (Error E = dumpSectionToFile(SecName, FileName, Obj))
       return createFileError(FileName, std::move(E));
   }
for (StringRef Flag : Config.AddSection) {
     Sec.SectionType = llvm::wasm::WASM_SEC_CUSTOM;
     Sec.Name = SecName;
     std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
     Sec.Contents = makeArrayRef<uint8_t>(
         reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
         Buf->getBufferSize());
     Obj.addSectionWithOwnedContents(Sec, std::move(Buf));
   }
 
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CommonConfig &Config, const WasmConfig &,
                              object::WasmObjectFile &In, raw_ostream &Out) {
   Reader TheReader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = TheReader.create();
   if (!ObjOrErr)
     return createFileError(Config.InputFilename, ObjOrErr.takeError());