Summary: | [CMake] Clean up JSC JIT options | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||
Component: | WebKit2 | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Minor | CC: | commit-queue, fpizlo, mark.lam, mcatanzaro, ossy | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | 143831 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2015-04-21 10:35:57 PDT
This makes a lot of sense! Note, this patch in its current form is part of a long chain of patches (hence no r? yet), and needs to be committed alongside the patch in bug #143997 (or changed to not depend on that). Also note I don't touch the Xcode files here in an attempt to not break anything on Macs, only the CMake files, since they did not already have an ENABLE_JIT option. That would be a good follow-up. I did remove the option from FeatureList.pm, since I think that won't break anything. Alternative: remove the ENABLE_JIT option from the CMake port, and keep just ENABLE_LLINT_C_LOOP. I thought ENABLE_JIT was a nicer option to present to users, but the Xcode files and FeatureList.pm have ENABLE_LLINT_C_LOOP but not ENABLE_JIT, so this would make things consistent between both ports without requiring changes anywhere but the CMake files. Created attachment 251242 [details]
Patch
I took the time to do this properly, so it no longer depends on bug #143997. Created attachment 251248 [details]
Patch
Comment on attachment 251248 [details] Patch Ah but it still depends on bug #143831, whoops.... Created attachment 251359 [details]
Patch
Comment on attachment 251359 [details] Patch Clearing flags on attachment: 251359 Committed r183159: <http://trac.webkit.org/changeset/183159> All reviewed patches have been landed. Closing bug. Comment on attachment 251359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251359&action=review > Tools/Scripts/webkitperl/FeatureList.pm:-438 > - { option => "cloop", desc => "Force use of the llint c loop", > - define => "ENABLE_LLINT_C_LOOP", default => 0, value => \$forceCLoop }, With this change we lost build-webkit's --cloop option. Of course, --cmakeargs="-DENABLE_JIT=0" still works. (I haven't used --cloop option of build-webkit ever and don't plan to do it in the future, just noticed it.) Ah, I didn't realize that list of options was used for anything besides the default values. I think we should probably bring that back, as otherwise you won't be able to use build-webkit on various architectures. Options: (a) convert the XCode build to use ENABLE_JIT so that we can restore the option to FeatureList.pm, or (b) revert this patch and instead remove the ENABLE_JIT option from the CMake build. Maybe (b) is the way to go. When I wrote this patch, I did not realize that only CMake had the ENABLE_JIT option. Created attachment 251439 [details]
[CMake] Should be possible for an option to depend on multiple options
(Ignore that bogus attachment.) (In reply to comment #11) > Options: (a) convert the XCode build to use ENABLE_JIT so that we can > restore the option to FeatureList.pm, or (b) revert this patch and instead > remove the ENABLE_JIT option from the CMake build. > > Maybe (b) is the way to go. When I wrote this patch, I did not realize that > only CMake had the ENABLE_JIT option. Filip, Ossy, what do you prefer? (In reply to comment #11) > Ah, I didn't realize that list of options was used for anything besides the > default values. I think we should probably bring that back, as otherwise you > won't be able to use build-webkit on various architectures. > > Options: (a) convert the XCode build to use ENABLE_JIT so that we can > restore the option to FeatureList.pm, or (b) revert this patch and instead > remove the ENABLE_JIT option from the CMake build. > > Maybe (b) is the way to go. When I wrote this patch, I did not realize that > only CMake had the ENABLE_JIT option. I would prefer option (a), I think ENABLE_JIT is more descriptive name than ENABLE_LLINT_C_LOOP. Additionally they are the opposite of each other, why should we have two different option? So I think we should use ENABLE_JIT everywhere, only the following files should be changed: - Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:464 - */FeatureDefines.xcconfig - Platform.h:762 - build-jsc Aye-aye, see bug #144304. |