Summary: | Restrict the availability of some JSC options to local debug builds only. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2016-04-26 19:12:26 PDT
Created attachment 277437 [details]
proposed patch.
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. Could this be handled as a runtime flag? 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. (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 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.
Created attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.
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 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. > > 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.
Thanks for the review. Landed in r200136: <http://trac.webkit.org/r200136>. |