Bug 135565 - [JSC] Build fix for FTL on EFL after ftlopt merge
Summary: [JSC] Build fix for FTL on EFL after ftlopt merge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-04 07:16 PDT by Dániel Bátyai
Modified: 2014-08-06 08:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.57 KB, patch)
2014-08-04 07:18 PDT, Dániel Bátyai
no flags Details | Formatted Diff | Diff
Patch (5.95 KB, patch)
2014-08-06 06:38 PDT, Dániel Bátyai
mark.lam: review-
Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2014-08-06 07:55 PDT, Dániel Bátyai
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2014-08-06 07:57 PDT, Dániel Bátyai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dániel Bátyai 2014-08-04 07:16:38 PDT
Currently we don't have a good way of generating the required files for native call inlining.
Comment 1 Dániel Bátyai 2014-08-04 07:18:04 PDT
Created attachment 235973 [details]
Patch
Comment 2 Mark Lam 2014-08-05 10:39:03 PDT
Comment on attachment 235973 [details]
Patch

Can you please add a comment in the ChangeLog to indicate why this fix is needed?  For example, say something about the feature of inlining native functions id dependent on features of the Clang compiler.
Comment 3 Mark Hahnenberg 2014-08-05 10:46:42 PDT
Maybe we should create a feature flag for the native inlining stuff? That might be a little clearer than just slapping COMPILER(CLANG) in a bunch of places.
Comment 4 Mark Lam 2014-08-05 10:48:27 PDT
(In reply to comment #3)
> Maybe we should create a feature flag for the native inlining stuff? That might be a little clearer than just slapping COMPILER(CLANG) in a bunch of places.

Yes, I agree. That is a better plan.
Comment 5 Dániel Bátyai 2014-08-06 06:38:30 PDT
Created attachment 236097 [details]
Patch
Comment 6 Mark Lam 2014-08-06 07:15:59 PDT
Comment on attachment 236097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236097&action=review

Also please rebase to ToT so that it’ll be easier to review this in context.  Thanks.

> Source/WTF/wtf/Platform.h:740
> +#if PLATFORM(EFL)
> +#define ENABLE_FTL_NATIVE_CALL_INLINING 0
> +#else
> +#define ENABLE_FTL_NATIVE_CALL_INLINING 1
> +#endif // PLATFORM(EFL)

Please change this to be dependent on COMPILER(CLANG) instead of PLATFORM(EFL) since that is the true nature of the dependency.  You could disable it for !COMPILER(CLANG) || PLATFORM(EFL) if you also don’t want to use it for EFL regardless of the compiler.
Comment 7 Dániel Bátyai 2014-08-06 07:55:53 PDT
Created attachment 236103 [details]
Patch
Comment 8 Dániel Bátyai 2014-08-06 07:57:40 PDT
Created attachment 236104 [details]
Patch

Fixed a typo
Comment 9 Mark Lam 2014-08-06 08:02:50 PDT
Comment on attachment 236104 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2014-08-06 08:45:19 PDT
Comment on attachment 236104 [details]
Patch

Clearing flags on attachment: 236104

Committed r172149: <http://trac.webkit.org/changeset/172149>
Comment 11 WebKit Commit Bot 2014-08-06 08:45:23 PDT
All reviewed patches have been landed.  Closing bug.