Stop building with assertions by default even in release builds. Fixes #3058 #3065
Conversation
|
Hmm.. I guess in that past we were using asserts for normal error reporting in some cases? I'm curious, how we build out upstream llvm? Do we use release+assert like chromium does for its clang? |
Yes, I had to fix one such place in
Looks like just a Release build, no asserts: |
|
As long as we've removed places where we have asserts for conditions that can arise from user inputs (as opposed to things that should "never happen" i.e. if it fails there's a bug), then this seems OK to me. (IMO we should also never call We actually build LLVM as "Release+Asserts" (https://github.com/WebAssembly/waterfall/blob/bf095321091328f1be1c1f250c26700f4cfaa6cb/src/build.py#L903) which keeps asserts (and |
src/tools/wasm-shell.cpp
Outdated
| assert(valid); | ||
| if (!valid) { | ||
| std::cerr << "Invalid module!\n"; | ||
| abort(); |
There was a problem hiding this comment.
Actually I guess I just said we shouldn't use abort for this purpose. The reason is that it's treated by the system more like a crash (e.g. you get a core file on Linux, and the exit status is different). Did we have a Fatal class somewhere for this purpose at one point?
There was a problem hiding this comment.
Ah yes, good point. Looks like this file is old and didn't use it, and I was copying the other behavior, but I fixed them all now.
|
(Ah, I just looked at LLVM's |
|
ping on this - what do people think? |
|
So we would loose both assertions and the I don't think an 8% win on size of binaryen makes much difference to emsdk users. As for the 5% win on speed do you think that would knock 5% of the wasm-opt and finalize times for large builds? If so then i might be worth it. I'm actaully quite surprised about that 5%... I guess we do a lot of asserting? |
|
I generally agree with #3065 (comment) that as long as we keep testing the release build with assertions enabled, the build we ship to users can be (or is even better be) highly optimized. But you are the person who got and fixed larger number of bug reports than anyone else here, so you would know the best how much the bug reports with assertions enabled helped. If you think it was OK without assertions it should be fine on that point too. |
|
I would prefer if we had an easy way to do a ReleaseWithAsserts build, since that's what I would want to do most local development with. I have no problem shipping release binaries without asserts, though. |
|
Thanks for the feedback. Well, it sounds like we are unsure what to do here. Maybe the safe thing is to eventually do @dschuff 's idea from earlier, to get the builders capable of building both Release and Release+Asserts, then to use the former for final releases to users and the latter for our test runs? I'll switch this PR to a draft. @tlively I think a Release+Asserts build is as easy as |
|
The sense I got from the feedback was that you should go for it, not that we were unsure, but maybe I'm reading the sentiment incorrectly. |
|
Yeah. I don't feel strongly about this at this point. |
|
Yeah, sorry, I didn't mean to imply people were opposed. But I'm also not 100% sure this is the right move. So I think the safe approach is probably better, and to wait for builder work for that. |
| // We have an option to build with assertions disabled | ||
| // BYN_ASSERTIONS_ENABLED=OFF, but we currently don't recommend using and we | ||
| // don't test with it. | ||
| #error "binaryen is currently designed to be built with assertions enabled." |
There was a problem hiding this comment.
Maybe we should just remove these errors independently of this PR, and then once we have separate CI and Release builds as we've been discussing, then we can run with assertions on CI and without on our release builds.
There was a problem hiding this comment.
sgtm. @sbc100 you added those specific ones - any concerns? if not i can open a PR.
There was a problem hiding this comment.
I can't remember exactly why a I added this. Perhaps we were depending on assert or something like them for normal error reporting. I'm not sure if that is still true. I guess now.
There was a problem hiding this comment.
I assumed it was because we were at one point depending on asserts for error reporting. But we aren't doing that anymore.
As mentioned in #3058 this made sense in the early days when bugs were
everywhere, but we probably don't need all our users doing this all the time.
Some numbers: Testing on two benchmarks, this makes us about 5% faster,
and builds are 8% smaller.
This adds CI run for a debug build, which has assertions enabled, so we
don't lose coverage here. (We will lose coverage in the emscripten CI
though which will use a release build.) Looks like that build is ~6 minutes
vs ~5 minutes for the opt build (faster compilation, but slower to run the
tests).