Bug 132907

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 EFLAssignee: 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 Flags
Proposed patch
ossy: review-, ossy: commit-queue-
Proposed patch
none
Proposed patch none

Description Zsolt Borbely 2014-05-14 07:34:30 PDT
Add include path of cxxabi.h if FTL JIT is enabled.
Comment 1 Zsolt Borbely 2014-05-14 07:39:56 PDT
Created attachment 231452 [details]
Proposed patch
Comment 2 Gyuyoung Kim 2014-05-14 16:45:27 PDT
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 ?
Comment 3 László Langó 2014-05-14 23:53:28 PDT
(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
Comment 4 László Langó 2014-05-15 00:00:32 PDT
(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 5 Gyuyoung Kim 2014-05-15 00:05:32 PDT
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 ?
Comment 6 Gyuyoung Kim 2014-05-15 00:16:16 PDT
(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 7 Raphael Kubo da Costa (:rakuco) 2014-05-16 13:01:17 PDT
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 8 Csaba Osztrogonác 2014-05-16 23:37:51 PDT
Comment on attachment 231452 [details]
Proposed patch

r- for now based on previous comments.
Comment 9 Zsolt Borbely 2014-05-19 06:58:17 PDT
Created attachment 231685 [details]
Proposed patch

This package is available in Ubuntu 13.10 and 14.04.
Comment 10 Raphael Kubo da Costa (:rakuco) 2014-05-20 01:59:01 PDT
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.
Comment 11 Zsolt Borbely 2014-05-20 05:56:33 PDT
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 12 Raphael Kubo da Costa (:rakuco) 2014-05-20 06:51:24 PDT
Comment on attachment 231763 [details]
Proposed patch

Looks much better now, thanks. No more objections.
Comment 13 Gyuyoung Kim 2014-05-21 16:43:04 PDT
Comment on attachment 231763 [details]
Proposed patch

LGTM too.
Comment 14 WebKit Commit Bot 2014-05-21 17:13:16 PDT
Comment on attachment 231763 [details]
Proposed patch

Clearing flags on attachment: 231763

Committed r169181: <http://trac.webkit.org/changeset/169181>
Comment 15 WebKit Commit Bot 2014-05-21 17:13:25 PDT
All reviewed patches have been landed.  Closing bug.