[Webkit-unassigned] [Bug 220365] [JSC] Allow to build WebAssembly without B3

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=220365

Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #418027|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

--- Comment #12 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 418027
  --> https://bugs.webkit.org/attachment.cgi?id=418027
Make Wasm/B3 a compile-time option, v6

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

r=me with improvements.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:29
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

I think you can make this just `ENABLE(WEBASSEMBLY_B3JIT)`.  `ENABLE(WEBASSEMBLY_B3JIT)` should imply `ENABLE(WEBASSEMBLY)`.  Otherwise, it's pointless.

Please also update the comment at the matching `#endif` below with `ENABLE(WEBASSEMBLY_B3JIT)` instead of `ENABLE(WEBASSEMBLY_B3JIT)`.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.h:28
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

Ditto here and in other files below.  Also, please update all the corresponding `#endif`s.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:29
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

As sanity check, please add the following after the #includes below:

#if !ENABLE(WEBASSEMBLY)
#error ENABLE(WEBASSEMBLY_B3JIT) is enabled, but ENABLE(WEBASSEMBLY) is not.
#endif

This ensures that we'll get a build error if anyone ever misconfigures these flags.  We'll probably get a build error anyway due to use of Wasm data structures, but having this #error will make it more explicit, plus it documents this invariant explicitly.  At first, I thought to put this in PlatformEnable.h, but reconsidered and decided that we don't really need to do this check repeatedly.

> Source/JavaScriptCore/wasm/WasmCallee.h:138
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmCallee.h:191
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:77
> +#if ENABLE(WEBASSEMBLY_B3JIT)

nit: let's move this `#if` above the `} else {` since the entire else clause if not needed otherwise.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:106
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.h:32
> +#include "WasmModuleInformation.h"
> +#include "WasmSignature.h"

Let's just forward declare these like we do with the FunctionCodeBlock class below.  It's better to reduce the amount of #include load unless we really need it.

> Source/JavaScriptCore/wasm/WasmOperations.cpp:457
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text is definitely long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmPlan.cpp:191
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:258
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmThunks.cpp:99
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210121/1b8ab0be/attachment.htm>