[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 #3 from Dániel Bátyai <dbatyai.u-szeged at partner.samsung.com> ---
(In reply to comment #2)
> Comment on attachment 242627 [details]
> 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.
You're probably right, let me double-check this.
> 
> 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.
Good point.
> 
> > 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?
You are correct, I shouldn't have forgot about those. My bad.
> 
> > 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.)
Okay, I'll add seperate patches for the ARM64 parts.

-- 
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/3c6b1486/attachment-0002.html>