Bug 139295

Created attachment 242627 [details] Patch Added the missing code for stack unwinding and some additional small fixes

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. 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.)

(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.

Created attachment 242908 [details] Patch

(In reply to comment #4) > Created attachment 242908 [details] > Patch Looks good to me now. (But that does not mean much. Let's see what a reviewer has to say.)

I also think JSC reviewer should review this patch. Pizlo ?

Comment on attachment 242908 [details] Patch r=me

Comment on attachment 242908 [details] Patch Clearing flags on attachment: 242908 Committed r177315: <http://trac.webkit.org/changeset/177315>

All reviewed patches have been landed. Closing bug.