[webkit-reviews] review denied: [Bug 217079] REGRESSION(r259582): Build fails on aarch64 Linux with WebKit 2.30.1 on LLIntOffsetsExtractor.cpp.o : [Attachment 410127] Patch.

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied  review:
Bug 217079: REGRESSION(r259582): Build fails on aarch64 Linux with WebKit
2.30.1 on LLIntOffsetsExtractor.cpp.o
https://bugs.webkit.org/show_bug.cgi?id=217079

Attachment 410127: Patch.

https://bugs.webkit.org/attachment.cgi?id=410127&action=review




--- Comment #11 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 410127
  --> https://bugs.webkit.org/attachment.cgi?id=410127
Patch.

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

Please roll out this patch and fix the issues below.

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:377
> +#if ENABLE(JIT)

This change doesn't make sense.  LinkBuffer::copyCompactAndLinkCode() should
only be built in if the assembler is enabled.  Not only that, this change is
actually wrong.  If useFastJITPermissions() is true, we should not be doing the
performJITMemcpy() here at all.  You've actually introduced a bug. 
Fortunately, this bug will not manifest if ENABLE(JIT) is true.  It still makes
the code lie about what is correct and expected.

I think the real fix for this should be to ensure that all the support for the
ASSEMBLER is not built in if none of the JITs are enabled.  For now, can you
roll out this patch and move this ENABLE(JIT) this to just around the if
statement for the dumpJITMemory?  Also add a FIXME comment here that this is a
temporary build fix until we can figure out why all this JIT and ASSEMBLER
infrastructure is built in on Linux when JIT is disabled.

You should also check why ENABLE(ASSEMBLER) is enabled for you.  For example,
do you have ENABLE(YARR_JIT) set to true, or do you have ENABLE(C_LOOP) set to
false for your configuration?  Maybe there's no bug there, but you should make
sure.

> Source/WTF/wtf/PlatformEnable.h:888
> +#if CPU(ARM64) && CPU(ADDRESS64) && ENABLE(JIT)

This change also doesn't make sense.  USE(JUMP_ISLANDS) should at most be
dependent on ENABLE(ASSEMBLER), but it is only ever used in assembler code. 
Hence, this change should not be necessary at all.