Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only strip the producers' section in optimized builds #11996

Merged
merged 5 commits into from
Aug 23, 2020
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 21, 2020

In -O0 builds we want to leave the wasm from LLVM unmodified as much as
possible (see WebAssembly/binaryen#3043), so this PR makes us stop modifying it to remove the
producers section.

There is no change to optimized builds (-O1+), so release builds are not changed in
any way.

There is also no change if the user told LLVM to not emit a producer's section
in the first place. We basically just do less things on -O0 builds after this PR,
and leave it to LLVM.

@kripken kripken requested review from sbc100 and tlively August 21, 2020 23:13
# the specific difference is that LLVM emits language info (C_plus_plus_14)
# when emitting debug info, but not otherwise.
self.assertLess(no_size, line_size)
self.assertEqual(line_size, full_size)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change, given that we already test this specifically below?

I find this test already to be a little to long/complex for a unittest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is important in general for checking what happens with debug info when compiling to object files first and then linking them. That is not checked by the main test for the producer's section that you mentioned lower down (it compiles straight to an executable, and it doesn't look at debug info). I think it's worth adding this as I found a curious interaction between the two (mentioned in the comment here) which confused me when working on this. The test addition here both explains that, and will prevent accidentally breaking anything with the producers' section / debugging interaction as we make changes on both sides.

ChangeLog.md Outdated
in release builds, which is not changing here; if you want to not have a
producer's section in debug builds, you can either tell LLVM to not emit it in
the first place, or you can run `wasm-opt --strip-producers` (which is what
Emscripten does in release builds).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t remember having an LLVM option to skip the producer’s section. I think it may be unconditional. It would be simple to add an LLVM option for this if you think it is important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't need an LLVM option, if it hasn't come up yet. We can wait to see if people ask for it I guess.

@kripken kripken merged commit af76833 into master Aug 23, 2020
@kripken kripken deleted the no-noprod branch August 23, 2020 22:45
kripken added a commit that referenced this pull request Aug 28, 2020
kripken added a commit that referenced this pull request Aug 31, 2020
…opt (#12075)

As part of WebAssembly/binaryen#3043 we stopped
stripping the producers section in unoptimized builds in #11996

However there is a way to still strip it with very little overhead, using llvm-objcpy.
This PR makes us strip it using wasm-opt normally if we are running wasm-opt
anyhow, but if we are not (as we hope to get to soon in some builds) then it uses
llvm-objcpy. This does a little more work after link but it's pretty trivial and does
not rewrite code or anything like that.

This does add some complexity, but it is complexity we'll need anyhow for
not running wasm-opt when it isn't needed.

Fixes #12071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants