emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.81k stars 3.31k forks source link

Should we ship NDEBUG (no asserts) versions of system libraries #15628

Open sbc100 opened 2 years ago

sbc100 commented 2 years ago

Our use of NDEBUG to disable asserts in system libraries is inconsistent today:

libcxx: Always define NDEBUG libunwind: Always define NDEBUG libmalloc: Defines -debug variant which doesn't define NDEBUG libstubs: Defines -debug variant which doesn't define NDEBUG libstandalonewasm: Always define NDEBUG

All other libraries do not define NDEBUG which means they include assertions. This includes libc, compiler-rt and libwasmfs.

For most of libc its not a huge deal since musl doesn't do at lot of asserting, but where have modified musl (for example in the pthread code we do have a bunch of asserts).

I guess we should probably define -debug versions of all libraries and link them when ASSERTIONS is enabled?

sbc100 commented 2 years ago

@dschuff @kripken

kripken commented 2 years ago

Good find. I'd suggest disabling assertions in all libraries by default.

I'm not sure how important a -debug version is. During development for ourselves it might be, and I think that's why we have some -debug variations of libraries - looks like emmalloc and stubs have that atm. But in general it's probably not worth building that variation by default etc., or adding the ability to get debug variations of other libraries. I'm not opposed though.

ensisoft commented 2 years ago

How about always having asserts built in? Removing asserts from release builds is like leaving all the safety gear at home when going sailing across the pacific, and that's exactly when you need them the most..

sbc100 commented 2 years ago

How about always having asserts built in? Removing asserts from release builds is like leaving all the safety gear at home when going sailing across the pacific, and that's exactly when you need them the most..

Asserts add a fair amount of code size. If you want them in your release build its easy enough to enable them with -sASSERTIONS, but I don't think we want that as the default. Folks generally assume that production builds do not contain asserts and think this is even more true on the web that in other places (since we care so much about code size).

ensisoft commented 2 years ago

Just curious have you actually measured the difference in code size? If you have a simple way to pass a flag that will turn them on/that that's nice. Personally I always keep my asserts on since I correctness and dumping core on bugs is important to me. Only if profiling tells me that some assert has a problem then I consider removing it (hasn't happened often yet).

sbc100 commented 2 years ago

A lot of asserts have strings attached to them, and we have a lot of asserts in emscripten JS library code.

In general in emscripten we care about even a few bytes regression in codesize. Enable assertions in JS library code (I would guess) would be many KB .. not just many bytes.