Created attachment 329602 [details] fix NDEBUG conditionals for a Release build with assertions turned on A Release build (-DNDEBUG) with assertions turned on (i.e. -DASSERT_DISABLED=0 -DASSERT_MSG_DISABLED=0 etc. see Source/WTF/wtf/Assertions.h) currently fails at various places. These failures typically stem from the use of variables in ASSERTS where the variable itself was defined inside an #ifndef NDEBUG ... #endif block. The failures can be fixed by putting the asserts within #ifndef NDEBUG ..#endif blocks. (And there are a few cases where the blocks seem to be conditionalized on ASSERT_DISABLED instead of NDEBUG) Attached is a patch against webkitgtk-2.19.3
Created attachment 342799 [details] tentative patch - against 2.21.4
This is kind of related to bug #177459, I've CC'd Carlos and Michael to make them aware of this patch.
I've actually wanted to do this for a while. For us to review the patch, be sure to include a proper ChangeLog entry created with prepare-ChangeLog, make sure the patch builds against current trunk, and set the r? flag (for review) and also the cq? flag (for commit).
Created attachment 388272 [details] updated patch 2.27.4 (maybe some of these should actually test for ASSERT_ENABLED #205776)
Yes, they should test ASSERT_ENABLED. Also: (In reply to Michael Catanzaro from comment #3) > For us to review the patch, be sure to include a proper ChangeLog entry > created with prepare-ChangeLog, make sure the patch builds against current > trunk, and set the r? flag (for review) and also the cq? flag (for commit).
*some* of the tests have to be for NDEBUG (and not ASSERT_ENABLED) - see my first comment #1 for why. I'm adding mark.lam to the CC list as I think he would be in a position to fix the problem (I'm assuming he was the one who fixed the other issues related to NDEBUG/ASSERT_ENABLED) in the codebase. [I'm still building webkit from the tarball releases on a low end machine (though I can looking at the git repo on a guest machine). I am not in a position to use the prepare-ChangeLog and other developer tools. So I'm hoping that someone with access to those developer resources could act on this report.]
Comment on attachment 329602 [details] fix NDEBUG conditionals for a Release build with assertions turned on This is exactly the wrong thing to do. The right thing to do is to find those fields which are needed for the ASSERT but are guarded by #if !NDEBUGand change them to be guarded by #if ASSERT_ENABLED because they are relevant to the assertion, and not a debug build per se.
I agree with Mark, but want to go a little further: Now that we have more clarity about ASSERT_ENABLED, we have almost no use for NDEBUG; off hand I can’t think of anything we chose to do besides assertions in debug but not release builds. I’m sure there are some things, but it seems that any places we have different behavior in debug builds for something other than assertions, we should discuss with the rest of the WebKit contributors to see if we want to keep it.
(In reply to Darin Adler from comment #8) I think NDEBUG is a CMake concept: It may be legit to have debug variables and assertions enabled only in a "Debug" build and I think CMAKE turns on NDEBUG (Gowever "Debug" also turns on -g and bloats the build) NDEBUG may still make sense in other contexts: LOG_DISABLED is toggled by ASSERT_ENABLED, but logging and assertions ought to be independent.
(In reply to enometh from comment #9) > (In reply to Darin Adler from comment #8) > > I think NDEBUG is a CMake concept: Of course not, it's regularly used in Autotools build systems, and meson supports it as well, and glibc checks for it to modify the behavior of its assert() function.... Anyway, WebKit asserts should usually depend on ASSERT_ENABLED, not NDEBUG. But I think an exception might be appropriate for asserts that are particularly slow. Those asserts should -- arguably -- be guarded by NDEBUG instead, because there exists a hypothetical wonderful future where WebKit is less buggy and we can get rid of the RELEASE_ASSERT() macros and use ASSERT_ENABLED for release builds to run all non-slow asserts in production. (I'm convinced having asserts enabled in production is essential to improve quality.) But stuff like asserts that require loops, though, or asserts that show up in profiling, will usually not ever be OK for production. Anyway, it doesn't matter too much, because that future is not today.
I know where NDEBUG is *from*. It comes from the C standard. It’s how you can turn off the assertions from the <assert.h> header. I don’t see any reason to mix NDEBUG and ASSERT_ENABLED in the WebKit project. If we’re moving to ASSERT_ENABLED we should move off NDEBUG. (In reply to Michael Catanzaro from comment #10) > Anyway, WebKit asserts should usually depend on ASSERT_ENABLED, not NDEBUG. > But I think an exception might be appropriate for asserts that are > particularly slow. Those asserts should -- arguably -- be guarded by NDEBUG > instead, because there exists a hypothetical wonderful future where WebKit > is less buggy and we can get rid of the RELEASE_ASSERT() macros and use > ASSERT_ENABLED for release builds to run all non-slow asserts in production. > (I'm convinced having asserts enabled in production is essential to improve > quality.) But stuff like asserts that require loops, though, or asserts that > show up in profiling, will usually not ever be OK for production. Yes, I think it might be interesting to move to a model where we write assertions with production in mind, renaming ASSERT to something like SLOW_ASSERT and RELEASE_ASSERT to ASSERT. But I’ve written hundreds if not thoughts assertions for WebKit knowing as I wrote them that they are inappropriately slow. So we’re not going to do this by just turning all the existing assertions into fast assertions. Someone at Apple proposed this recently so I’ve thought it through recently. If we do this, I don’t think it should have anything to do with NDEBUG. I think there are some really confusing things going on. For example, I think people want to build optimized builds with assertions on. At least on the Apple ports, our "debug" configuration turns off most optimizations and I think in a lot of people's minds NDEBUG is somehow tied to that configuration. They want those optimizations back on but want to keep all the assertions, even the slow ones. I propose that on the WebKit project we aim to stop using NDEBUG entirely, except if we have a reason to use it to disable <assert.h> assertions, find out where NDEBUG is used for any purpose other than ASSERT_ENABLED. Once we find those purposes, we can figure out what to use to control each of those.
FWIW I'm fine with that proposal too. It's simpler than mine, and that means harder for developers to mess up. :)
I meant hundreds if not thousands.
Created attachment 454546 [details] updated patch 2.35.90