Closed Bug 1042833 Opened 5 years ago Closed 5 years ago

Remove JS_ION ifdef


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: bhackett, Assigned: bhackett)




(2 files)

Mentions of the JS_ION ifdef are sprinkled around nearly 500 places in the code base, and the --disable-ion build keeps breaking because so many features are gated on whether JS_ION is enabled.  While we need to continue supporting builds where the JITs are disabled it would be nice if this was handled a bit more elegantly.  Maybe have a backend set by --disable-ion which MOZ_CRASH'es everywhere and an IsIonEnabled() method so we avoid trying to compile anything?  While this might require more code, it should minimize the differences between --enable-ion and --disable-ion builds and we should only get build breakage when changing the interfaces used by IonMacroAssembler and similar classes.
This patch changes JS_ION so that it is always defined, and adds a 'none' architecture with associated files that are compiled as the target when --disable-ion is used.  The 'none' architecture crashes anytime anyone tries to use it, so there are a few places where we test for this architecture (via JS_CODEGEN_NONE) before deciding to JIT compile anything.  If this patch lands, as a followup we can just remove all references to JS_ION.
Assignee: nobody → bhackett1024
Attachment #8462940 - Flags: review?(jdemooij)
Comment on attachment 8462940 [details] [diff] [review]
add 'none' architecture

Review of attachment 8462940 [details] [diff] [review]:

Nice, thanks for doing this!

This can/will break the --disable-ion build in new ways (when methods are added to the macro-assembler for instance), but whenever this happens it should be more trivial to fix than most of the current --disable-ion bustage in random parts of the tree. It's certainly much nicer to have most of the --disable-ion code in its own directory than having #ifdef JS_ION everywhere.

::: js/src/
@@ +359,5 @@
>              ]
> +else:
> +        'jit/none/Trampoline-none.cpp'
> +    ]

Indent this block, so that it comes after elif CONFIG['JS_CODEGEN_ARM']
Attachment #8462940 - Flags: review?(jdemooij) → review+
Attached patch remove JS_IONSplinter Review
This patch removes all references to JS_ION
Attachment #8463713 - Flags: review?(jdemooij)
Comment on attachment 8463713 [details] [diff] [review]
remove JS_ION

Review of attachment 8463713 [details] [diff] [review]:

r+++, would review again.
Attachment #8463713 - Flags: review?(jdemooij) → review+
Pushed a long-overdue support for --disable-ion via fuzzing rev 6170cde887c2.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1046452
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.