Summary: | [EFL] Add include path of compact_unwind_encoding.h if FTL JIT is enabled | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zsolt Borbely <zsborbely.u-szeged> | ||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bunhere, commit-queue, gyuyoung.kim, gyuyoung.kim, llango.u-szeged, lucas.de.marchi, rakuco, sergio | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Zsolt Borbely
2014-05-14 07:34:30 PDT
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. |