This is an archive of the discontinued LLVM Phabricator instance.

[llvm-strip][WebAssembly] Support strip flags
ClosedPublic

Authored by dschuff on Jan 31 2020, 4:25 PM.

Details

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

Diff Detail

Event Timeline

dschuff created this revision.Jan 31 2020, 4:25 PM

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.

Mostly lgtm with some comments.

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

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

12

What is this NOT checking for?

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

ditto above, what is this checking for?

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

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
101

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

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
101

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

aardappel accepted this revision.Feb 3 2020, 10:29 AM
This revision is now accepted and ready to land.Feb 3 2020, 10:29 AM
dschuff updated this revision to Diff 244477.Feb 13 2020, 10:36 AM
dschuff marked 4 inline comments as done.

rebase, add comments to test expectations

Updated, please take a look.

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

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.

12

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

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

Added comment here too.

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

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

sbc100 added inline comments.Feb 13 2020, 11:07 AM
llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
71
-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.

78

Just return !isDebugSection(Sec))?

90

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

138–139

Did we loose SymbolsToKeep by accident here?

dschuff updated this revision to Diff 244552.Feb 13 2020, 5:08 PM
dschuff marked 4 inline comments as done.

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

sbc100 accepted this revision.Feb 13 2020, 7:21 PM
sbc100 added inline comments.
llvm/test/tools/llvm-objcopy/wasm/basic-keep.test
22

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

jhenderson requested changes to this revision.Feb 17 2020, 7:39 AM

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

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 added inline comments.Feb 17 2020, 7:39 AM
llvm/test/tools/llvm-objcopy/wasm/basic-keep.test
10–11

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

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

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

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

6

'##'

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

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

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

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

9

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
2

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
26

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

30

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

77

Typo: Explictily -> Explicitly

79

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.

This revision now requires changes to proceed.Feb 17 2020, 7:39 AM
dschuff updated this revision to Diff 292559.Sep 17 2020, 10:32 AM
dschuff marked 8 inline comments as done.

Address feedback

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

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

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

Tweaked the behavior, and made the comment match.

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

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

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

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

71

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

79

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?

90

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.

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

Nit: Remove this line.

15–17

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
2

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

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.

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

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 updated this revision to Diff 358106.Jul 12 2021, 5:13 PM
dschuff marked 22 inline comments as done.
  • review round 3

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
2

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.

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
7

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
11

Nit

13

Looks like this obj2yaml line isn't necessary?

17–20
22

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

23
30–31

Nit: delete the extra blank lines here.

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

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?

11–13
llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test
17

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.

21

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

27–28

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
2

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

7

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
79

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

lgtm with comments addressed

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

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 updated this revision to Diff 358414.Jul 13 2021, 1:34 PM
dschuff marked 17 inline comments as done.
  • address more comments
llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test
22

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
2

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
17

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
79

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 added inline comments.Jul 14 2021, 1:40 AM
llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test
22

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
13

Spacing looks off on this line?

llvm/test/tools/llvm-objcopy/wasm/strip-all.test
7 ↗(On Diff #358414)

Spacing?

jhenderson accepted this revision.Jul 14 2021, 1:40 AM

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

This revision is now accepted and ready to land.Jul 14 2021, 1:40 AM
sbc100 accepted this revision.Jul 14 2021, 12:17 PM
This revision was landed with ongoing or failed builds.Jul 14 2021, 2:17 PM
This revision was automatically updated to reflect the committed changes.
dschuff marked 3 inline comments as done.