[Webkit-unassigned] [Bug 139295] [EFL] FTL JIT not working on ARM64

[bugzilla-daemon at webkit.org]

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

--- Comment #2 from Akos Kiss <akiss at inf.u-szeged.hu> ---
Comment on attachment 242627
  --> https://bugs.webkit.org/attachment.cgi?id=242627
Patch

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

> Source/JavaScriptCore/ftl/FTLCompile.cpp:68
> +#elif OS(LINUX)

This is strange to me. Why do you need to care about this code in mmAllocateCodeSection? Both EFL & GTK use LLVM 3.5.0 now, so no "older" LLVM. Did you experience LLVM emitting eh_frame as a code section? If not, I would not touch this legacy code here.

However, I'd tacke the same code in mmAllocateDataSection below, since the #if/#elif/#endif guards, as they stand now, may produce invalid C++ code after the preprocessing is done. If both OS(DARWIN) and OS(LINUX) are false then we are left with a missing "if (...) {" line and a dangling closing curly brace. So, it seems to be worthwhile to add
#else
#error "Unrecognized OS"
here, so that we are on the safe side.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:635
> +#else

I'd apply the "if Linux then this, if Darwin then that, error otherwise" idea here as well, instead of assuming that if we are not on Linux then it's Darwin.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:396
> +#else

Similar to the above: make sure that we are on x86-64 and bail out otherwise.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:-737
> -    // FIXME: Implement stackunwinding based on eh_frame on ARM64

In the x86-64 code path, there are some RELEASE_ASSERTs here. Have they no counterpart on ARM64?

> Tools/efl/patches/llvm-elf-add-stackmaps.patch:60
> +

I don't think that patching up an "official" patch is a good idea. That is, both llvm-elf-add-stackmaps.patch and llvm-elf-allow-fde-references-outside-the-2gb-range.patch have been accepted to and landed in LLVM trunk. They bear their commit ID, author's name, timestamp, comments, etc. Now, this would just change the "content" of the patches leaving everything else unchanged. It may become confusing later if someone checks out the changes from the LLVM repository by commit ID and finds that there are differences.

So, I'd either replace the "official" patches with new patches that don't have references to the LLVM repository, or add two new patches in Tools/efl/patches that add whatever is missing for ARM64 to work. (I'd vote for the second option.)

-- 
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/20141205/aeb1da75/attachment-0002.html>