Bug 205776 – Convert ASSERT_DISABLED to ASSERT_ENABLED, and fix some tests of NDEBUG that should actually test for ASSERT_ENABLED.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

patch and details coming.

[Mark Lam]

Created attachment 386795 [details]
proposed patch.

Let's test this on the EWS first.

[Radar WebKit Bug Importer]

<rdar://problem/58327452>

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Mark Lam]

Created attachment 386798 [details]
proposed patch.

[Mark Lam]

Comment on attachment 386798 [details]
proposed patch.

Ready for a review.

[Saam Barati]

Comment on attachment 386798 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386798&action=review

> Source/JavaScriptCore/ChangeLog:14
> +            1. Release build: ~40 minutes

can you state what the baseline is here (release without debug asserts)?

> Source/JavaScriptCore/ChangeLog:31
> +        This patch replaces ASSERT_DISABLED with ASSERT_ENABLED.

this patch becomes much easier to review if the renaming and introduction of JSC_ and WTF_ asserts are done on their own

> Source/JavaScriptCore/ChangeLog:41
> +            This change also does away with the need for the double negative
> +            !ASSERT_DISABLED test that is commonly used all over the code, thereby
> +            improving code readability.

I agree, this is nicer

> Source/JavaScriptCore/ChangeLog:60
> +            WebCore and higher level stacks aren't ready to run with debug ASSERTs on yet.

Why? Don't we run debug tests? Do tests fail if they get enabled?

> Source/JavaScriptCore/ChangeLog:95
> +            This also means that ASSERTs in WTF and JSC functions that are inlined into
> +            WebCore (and above) will not be enabled.

this seems slightly weird given what you said about field layout.

[Saam Barati]

Comment on attachment 386798 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386798&action=review

>> Source/JavaScriptCore/ChangeLog:95
>> +            WebCore (and above) will not be enabled.
> 
> this seems slightly weird given what you said about field layout.

What happens if
ASSERT_ENABLED == 0
JSC_ASSERT_ENABLED == 1

and I have an ASSERT() inside JSC code?

[Saam Barati]

Comment on attachment 386798 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386798&action=review

Would it be easier to just have a build configuration of debug that compiles with -O2?

> Source/JavaScriptCore/config.h:21
> +#include <wtf/Compiler.h>

what's this used for?

[Saam Barati]

I think a simpler patch would be:
1. Make a debug build that compiles with O2
2. any asserts that are hit while running tests should be changed to some lower form of assert and be given a FIXME

This seems like a lot of code just to tiptoe around things above JSC/WTF have asserts that fire. If those asserts fire, we should either remove them, add a FIXME, or degrade them into a form of assert that never runs.

[Saam Barati]

(In reply to Saam Barati from comment #9)
> I think a simpler patch would be:
> 1. Make a debug build that compiles with O2
> 2. any asserts that are hit while running tests should be changed to some
> lower form of assert and be given a FIXME
> 
> This seems like a lot of code just to tiptoe around things above JSC/WTF
> have asserts that fire. If those asserts fire, we should either remove them,
> add a FIXME, or degrade them into a form of assert that never runs.

Also, since this is not going to be used to run layout tests, just JSC tests, I think we can put (2) on the back burner.

[Mark Lam]

Comment on attachment 386798 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386798&action=review

>> Source/JavaScriptCore/ChangeLog:60
>> +            WebCore and higher level stacks aren't ready to run with debug ASSERTs on yet.
> 
> Why? Don't we run debug tests? Do tests fail if they get enabled?

They did fail for me when I tried with this patch (forced ASSERT_ENABLED to true).  But maybe that's due to other uses of #ifndef NDEBUG that should have been #if ASSERT_ENABLED instead.  The number of failures (crashes) were huge, not just the few that we see on the Debug bots at build.webkit.org.

>>> Source/JavaScriptCore/ChangeLog:95
>>> +            WebCore (and above) will not be enabled.
>> 
>> this seems slightly weird given what you said about field layout.
> 
> What happens if
> ASSERT_ENABLED == 0
> JSC_ASSERT_ENABLED == 1
> 
> and I have an ASSERT() inside JSC code?

The code needed to set up state for ASSERT() in JSC code is guarded by JSC_ASSERT_ENABLED.  Hence, they will execute even though the ASSERT is a no op in WebCore code.  In JSC code, the ASSERT will run and the state it depends on will be consistent.

>> Source/JavaScriptCore/config.h:21
>> +#include <wtf/Compiler.h>
> 
> what's this used for?

Compiler.h defines ASAN_ENABLED which we test below.

[Mark Lam]

Comment on attachment 386798 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386798&action=review

>> Source/JavaScriptCore/ChangeLog:14
>> +            1. Release build: ~40 minutes
> 
> can you state what the baseline is here (release without debug asserts)?

The baseline Release build without debug ASSERTS takes about 25 minutes.

[Mark Lam]

Comment on attachment 386798 [details]
proposed patch.

Now that https://bugs.webkit.org/show_bug.cgi?id=205787 will take care of enabling Debug builds to run fast, we no longer need to try to finagle the code to enable debug ASSERTs in some components and not others.

I'm going to redo this patch to only be a rename of ASSERT_DISABLED to ASSERT_ENABLED plus the fixes where tests of NDEBUG should actually be tests of ASSERT_ENABLED.

[Mark Lam]

Created attachment 386819 [details]
proposed patch.

[Saam Barati]

Comment on attachment 386819 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386819&action=review

r=me

I suggest also:
1. Filing a bug to get rid of more "ifndef NDEBUG"
2. Get all layout tests to pass when enabling ASSERT_ENABLED on release builds (presumably [1] will help with this).
3. Send out a PSA to webkit-dev so folks stop using "ifndef NDEBUG" for most assertions. (Some still might make sense if they're super costly in terms of perf.)

> Source/JavaScriptCore/ChangeLog:8
> +        This patch did the following changes:

nit: probably should go in WTF/ChangeLog?

> Source/JavaScriptCore/ChangeLog:17
> +           as well.  Well do that in another patch.  For now, they are left as is to

"Well" => "We'll"

> Source/WTF/wtf/OptionSet.h:82
> +#ifndef NDEBUG

why isn't this ASSERT_ENABLED?

> Source/WTF/wtf/Platform.h:1050
> +   do debug assertion checks unconditionally (e.g.  treat a debug ASSERT

nit: "  " => " "

> Source/WebCore/html/HTMLTableRowsCollection.cpp:49
> +#else // not ASSERT_ENABLED

nit: why "not" in front? Do we do that in places?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:654
> +    // FIXME: This assertion code was never built, has bit rotted, and needs to be fixed before it can be enabled:
> +    // https://bugs.webkit.org/show_bug.cgi?id=205706.
> +#if ASSERT_ENABLED
>      VisiblePosition visiblePosition = passedPosition;
>      unsigned indexComputedByVisiblePosition = 0;
>      if (visiblePosition.isNotNull())
>          indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */);
>      ASSERT(index == indexComputedByVisiblePosition);

should we just delete it?

[Mark Lam]

Comment on attachment 386819 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=386819&action=review

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch did the following changes:
> 
> nit: probably should go in WTF/ChangeLog?

Makes sense for this updated patch.  I'll move it.

>> Source/JavaScriptCore/ChangeLog:17
>> +           as well.  Well do that in another patch.  For now, they are left as is to
> 
> "Well" => "We'll"

Fixed.

>> Source/WTF/wtf/OptionSet.h:82
>> +#ifndef NDEBUG
> 
> why isn't this ASSERT_ENABLED?

Because the build failure due to constexpr on release builds.  It has nothing to do with ASSERT_ENABLED.  I'm choosing to forego the ASSERT here so that this function can continue to be a constexpr function (which it needs to be).  This is documented in the comment below.

>> Source/WTF/wtf/Platform.h:1050
>> +   do debug assertion checks unconditionally (e.g.  treat a debug ASSERT
> 
> nit: "  " => " "

Fixed.

>> Source/WebCore/html/HTMLTableRowsCollection.cpp:49
>> +#else // not ASSERT_ENABLED
> 
> nit: why "not" in front? Do we do that in places?

Just to say that the following block is for when ASSERT_ENABLED is false.

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:654
>>      ASSERT(index == indexComputedByVisiblePosition);
> 
> should we just delete it?

Darin thinks there's value in keeping it.  See https://bugs.webkit.org/show_bug.cgi?id=205706#c2.

[Mark Lam]

Landed in r254087: <http://trac.webkit.org/r254087>.