Bug 157058 - Restrict the availability of some JSC options to local debug builds only.
Summary: Restrict the availability of some JSC options to local debug builds only.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-26 19:12 PDT by Mark Lam
Modified: 2016-04-27 11:36 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch. (38.14 KB, patch)
2016-04-26 19:23 PDT, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch with ChangeLog and debug build fix. (38.14 KB, patch)
2016-04-26 21:59 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.