Bug 157058

Summary: Restrict the availability of some JSC options to local debug builds only.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, bfulgham, fpizlo, ggaren, keith_miller, msaboff, oliver, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch with ChangeLog and debug build fix. ggaren: review+

Description Mark Lam 2016-04-26 19:12:26 PDT
Patch coming.
Comment 1 Mark Lam 2016-04-26 19:21:46 PDT
<rdar://problem/25577473>
Comment 2 Mark Lam 2016-04-26 19:23:22 PDT
Created attachment 277437 [details]
proposed patch.
Comment 3 Filip Pizlo 2016-04-26 19:34:21 PDT
Comment on attachment 277437 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/Options.cpp:58
> +#if NDEBUG

I'm really not a fan of this policy.  A lot of bugs require a release build to repro.
Comment 4 Brent Fulgham 2016-04-26 19:51:34 PDT
Could this be handled as a runtime flag?
Comment 5 Mark Lam 2016-04-26 21:45:55 PDT
Comment on attachment 277437 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/Options.cpp:58
>> +#if NDEBUG
> 
> I'm really not a fan of this policy.  A lot of bugs require a release build to repro.

I think you missed the part where allowRestrictedOptions() only applies to options with "Restricted" availability.  For now, the only options with "Restricted" availability are JSC_functionOverrides and JSC_useDollarVM.  If we need those in a release build, we can change this function and rebuild (which requires a rebuild of 1 file only).  All other options are "Normal" and are not gated by this allowRestrictedOptions() function.
Comment 6 Mark Lam 2016-04-26 21:48:42 PDT
(In reply to comment #4)
> Could this be handled as a runtime flag?

This is not an option (pardon the pun).  All JSC options are runtime flags.  The whole point of this patch is that there are some "Restricted" options that we don't want to be runtime flags.  Hence, it doesn't make sense to use a runtime flag to disable those JSC runtime options / flags.
Comment 7 Mark Lam 2016-04-26 21:49:20 PDT
Comment on attachment 277437 [details]
proposed patch.

I need to revise this patch to fix the broken debug build, and some non-ascii chars in the ChangeLog.
Comment 8 Mark Lam 2016-04-26 21:59:53 PDT
Created attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.
Comment 9 Oliver Hunt 2016-04-27 11:18:08 PDT
Comment on attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.

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

> Source/JavaScriptCore/runtime/Options.cpp:62
> +#ifdef NDEBUG
> +    return false;
> +#else
> +    return true;
> +#endif

Consider doing an internal test. Something like:
#if PLATFORM(OSX)
return !csr_check(CSR_ALLOW_APPLE_INTERNAL); // CSR check
#elif PLATFORM(IOS)
return MGGetBoolAnswer(kMGQAppleInternalInstallCapability); // Check with mobile gestalt
#endif
Comment 10 Geoffrey Garen 2016-04-27 11:27:22 PDT
Comment on attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.

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

r=me

>> Source/JavaScriptCore/runtime/Options.cpp:62
>> +#endif
> 
> Consider doing an internal test. Something like:
> #if PLATFORM(OSX)
> return !csr_check(CSR_ALLOW_APPLE_INTERNAL); // CSR check
> #elif PLATFORM(IOS)
> return MGGetBoolAnswer(kMGQAppleInternalInstallCapability); // Check with mobile gestalt
> #endif

I think the debug check is enough. We want non-Apple-internal engineers to be able to debug too.
Comment 11 Geoffrey Garen 2016-04-27 11:29:23 PDT
> > Source/JavaScriptCore/runtime/Options.cpp:58
> > +#if NDEBUG
> 
> I'm really not a fan of this policy.  A lot of bugs require a release build
> to repro.

The plan for using these features in a release build is to edit this function and recompile. Since it's in a .cpp file, it's a single translation unit to recompile.
Comment 12 Mark Lam 2016-04-27 11:36:44 PDT
Thanks for the review.  Landed in r200136: <http://trac.webkit.org/r200136>.