Add include path of cxxabi.h if FTL JIT is enabled.
Created attachment 231452 [details] Proposed patch
Comment on attachment 231452 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=231452&action=review > ChangeLog:7 > + Could you explain why we should include cxxabi.h when FTL JIT is enabled ?
(In reply to comment #2) > (From update of attachment 231452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=231452&action=review > > > ChangeLog:7 > > + > > Could you explain why we should include cxxabi.h when FTL JIT is enabled ? Because of this include: #include <mach-o/compact_unwind_encoding.h> in JavaScriptCore/ftl/FTLUnwindInfo.cpp:31
(In reply to comment #0) > Add include path of cxxabi.h if FTL JIT is enabled. The description is not accurate. We need the mach-o/compact_unwind_encoding.h header from libc++abi-dev package. See: http://packages.ubuntu.com/hu/trusty/i386/libc++abi-dev/filelist Is the libc++abi-dev package added in the Tools/efl/install-dependencies script?
Comment on attachment 231452 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=231452&action=review > Source/cmake/FindLIBCXXABI.cmake:8 > + HINTS "/usr/include/libcxxabi/" Doesn't libcxxabi have .pc file ? Other FindFoo.cmake files have used ${PC_FOO_INCLUDEDIR}. Additionally, how to install libcxxabi library ? Should we install it manually ?
(In reply to comment #4) > (In reply to comment #0) > > Add include path of cxxabi.h if FTL JIT is enabled. > > The description is not accurate. We need the mach-o/compact_unwind_encoding.h header from libc++abi-dev package. See: http://packages.ubuntu.com/hu/trusty/i386/libc++abi-dev/filelist > > Is the libc++abi-dev package added in the Tools/efl/install-dependencies script? Yes, please add it to there.
Comment on attachment 231452 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=231452&action=review If you just need to look for a header file, it is better to just do that directly in OptionsEfl.cmake. Otherwise, you need to write a proper find file with a license and which handles its arguments and sets the _FOUND variable correctly. >> Source/cmake/FindLIBCXXABI.cmake:8 >> + HINTS "/usr/include/libcxxabi/" > > Doesn't libcxxabi have .pc file ? Other FindFoo.cmake files have used ${PC_FOO_INCLUDEDIR}. > > Additionally, how to install libcxxabi library ? Should we install it manually ? This HINTS line is wrong. /usr/include is a path looked at by default in most Unix variants. You should probably use PATH_SUFFIXES instead.
Comment on attachment 231452 [details] Proposed patch r- for now based on previous comments.
Created attachment 231685 [details] Proposed patch This package is available in Ubuntu 13.10 and 14.04.
Comment on attachment 231685 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=231685&action=review > Source/cmake/FindLIBCXXABI.cmake:40 > +if (LIBCXXABI_INCLUDE_DIR) > + set(LIBCXXABI_FOUND 1) > + set(LIBCXXABI_INCLUDE_DIRS ${LIBCXXABI_INCLUDE_DIR}) > +else () > + set(LIBCXXABI_FOUND 0) > +endif () Please take a look at the other find files, you don't need to do this yourself, just use FindPackagesHandleStandardArgs.
Created attachment 231763 [details] Proposed patch Unfortunately, I created the previous patch based on FindICU.cmake, which does not use FindPackagesHandleStandardArgs(). The patch is updated.
Comment on attachment 231763 [details] Proposed patch Looks much better now, thanks. No more objections.
Comment on attachment 231763 [details] Proposed patch LGTM too.
Comment on attachment 231763 [details] Proposed patch Clearing flags on attachment: 231763 Committed r169181: <http://trac.webkit.org/changeset/169181>
All reviewed patches have been landed. Closing bug.