Bug 132907 – [EFL] Add include path of compact_unwind_encoding.h if FTL JIT is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Zsolt Borbely]

Add include path of cxxabi.h if FTL JIT is enabled.

[Zsolt Borbely]

Created attachment 231452 [details]
Proposed patch

[Gyuyoung Kim]

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 ?

[László Langó]

(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

[László Langó]

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

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Csaba Osztrogonác]

Comment on attachment 231452 [details]
Proposed patch

r- for now based on previous comments.

[Zsolt Borbely]

Created attachment 231685 [details]
Proposed patch

This package is available in Ubuntu 13.10 and 14.04.

[Raphael Kubo da Costa (:rakuco)]

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.

[Zsolt Borbely]

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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 231763 [details]
Proposed patch

Looks much better now, thanks. No more objections.

[Gyuyoung Kim]

Comment on attachment 231763 [details]
Proposed patch

LGTM too.

[WebKit Commit Bot]

Comment on attachment 231763 [details]
Proposed patch

Clearing flags on attachment: 231763

Committed r169181: <http://trac.webkit.org/changeset/169181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.