Bug 151624

Summary:

Fix the B3 build with GCC 4.9.3

Product:

WebKit

Reporter:

Csaba Osztrogonác <ossy>

Component:

JavaScriptCore

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, mcatanzaro, msaboff, ossy, saam, ysuzuki, zan

Priority:

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

153422

Bug Blocks:

152248

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	fpizlo: review+

Csaba Osztrogonác

Reported 2015-11-26 06:15:03 PST

../../Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp: In lambda function: ../../Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:154:13: internal compiler error: in gimplify_var_or_parm_decl, at gimplify.c:1741 inst.forEachTmp([&] (Tmp& otherArg, Arg::Role role, Arg::Type otherArgType) { ^ Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-4.9/README.Bugs> for instructions. Preprocessed source stored into /tmp/ccVqyjJp.out file, please attach this to your bugreport. It is a reported and fixed GCC bug - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62272 But unfortunately it is fixed only in GCC 5.2 and the bug is still valid in GCC 4.9.3. Can we workaround this util we require at least GCC 5.2 version ?

Attachments
Patch (2.40 KB, patch) 2015-11-26 09:10 PST, Csaba Osztrogonác	no flags	Details Formatted Diff Diff
Patch (2.60 KB, patch) 2015-12-10 07:20 PST, Csaba Osztrogonác	no flags	Details Formatted Diff Diff
Patch (2.62 KB, patch) 2016-01-08 03:56 PST, Csaba Osztrogonác	no flags	Details Formatted Diff Diff
Patch (1.61 KB, patch) 2016-01-28 14:50 PST, Yusuke Suzuki	fpizlo: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Csaba Osztrogonác

Comment 1 2015-11-26 09:10:12 PST

Created attachment 266188 [details] Patch WIP workaround, don't use nested lambdas. Have you got a better idea or can we add a similar workaround?

WebKit Commit Bot

Comment 2 2015-11-26 09:11:21 PST

Attachment 266188 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:153: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 3 2015-11-26 09:34:07 PST

Comment on attachment 266188 [details] Patch I'm strongly opposed to this fix. std::function requires extra heap allocation in many cases.

Csaba Osztrogonác

Comment 4 2015-11-26 13:14:01 PST

(In reply to comment #3) > Comment on attachment 266188 [details] > Patch > > I'm strongly opposed to this fix. std::function requires extra heap > allocation in many cases. Could you suggest a better workaround? If no, can we add workaround for only GCC older than 5.2.1? Or should we wait some months, maybe a year to be able to enable B3 on Linux when we can drop supporting older compilers. Are you planning to maintain the llvm backend for a while or will you remove it immediately once B3 is stable enough?

Filip Pizlo

Comment 5 2015-11-26 16:30:46 PST

(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 266188 [details] > > Patch > > > > I'm strongly opposed to this fix. std::function requires extra heap > > allocation in many cases. > > Could you suggest a better workaround? You could use wtf/ScopedLambda instead of std::function, but I'm not sure how that interacts with bind and so forth. > > If no, can we add workaround for only GCC older than 5.2.1? That makes sense, though have you tried some finer grained work arounds? Is it really the case that you can only work around the bug by using a function rather than just trying to use lambdas in a slightly different way? > Or should we wait some months, maybe a year to be able to > enable B3 on Linux when we can drop supporting older compilers. We could do that, but... > > Are you planning to maintain the llvm backend for a while > or will you remove it immediately once B3 is stable enough? We will probably get rid of llvm soon.

Csaba Osztrogonác

Comment 6 2015-12-10 07:20:06 PST

Created attachment 267107 [details] Patch updated the same workaround to ToT

WebKit Commit Bot

Comment 7 2015-12-10 07:22:31 PST

Attachment 267107 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:185: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Csaba Osztrogonác

Comment 8 2015-12-10 07:23:24 PST

(In reply to comment #5) > You could use wtf/ScopedLambda instead of std::function, but I'm not sure > how that interacts with bind and so forth. Thanks, I'll check if we can use it. Or we might not enable B3 until we require the compiler which can complile it properly.

Filip Pizlo

Comment 9 2015-12-10 09:20:13 PST

(In reply to comment #8) > (In reply to comment #5) > > You could use wtf/ScopedLambda instead of std::function, but I'm not sure > > how that interacts with bind and so forth. > > Thanks, I'll check if we can use it. Or we might not enable B3 > until we require the compiler which can complile it properly. Note that when we enable B3, we will rip out LLVM and start deleting any code guarded by !FTL_USES_B3 until we no longer have the FTL_USES_B3 variable. It might be worth figuring out what a good workaround would look like. We use many lambdas in WebKit and none of them hit this bug. Do you know what, specifically, the bug is? I'd be happy with playing with performance-neutral restructurings of this code and I only oppose the use of std::function because I fear that it will really hurt performance.

Csaba Osztrogonác

Comment 10 2016-01-08 03:56:32 PST

Created attachment 268537 [details] Patch updated the same workaround to ToT

Csaba Osztrogonác

Comment 11 2016-01-08 04:18:59 PST

(In reply to comment #9) > Note that when we enable B3, we will rip out LLVM and start deleting any > code guarded by !FTL_USES_B3 until we no longer have the FTL_USES_B3 > variable. It isn't a blocker issue. If we won't be able to make B3-FTL build and as stable as LLVM-FTL is now on Linux, we can simply disable building FTL JIT until proper fix. > It might be worth figuring out what a good workaround would look like. We > use many lambdas in WebKit and none of them hit this bug. Do you know what, > specifically, the bug is? I'd be happy with playing with > performance-neutral restructurings of this code and I only oppose the use of > std::function because I fear that it will really hurt performance. I don't know the logic of this code at all and I'm not interested to try to find a workaround any more, because Ubuntu 15.04 will reach its end of life at the end of January, so we have to migrate to 15.10 soon which has newer GCC which doesn't have this issue. (Otherwise the issue can be found in the description of this bug report if somebody is still interested in workarounding it for older GCC.)

Michael Catanzaro

Comment 12 2016-01-11 07:23:13 PST

A GCC dependency bump would jeopardize our ability to get distributors to release our updates and consequentially the security of our users. But we can conditionally enable FTL depending on the version of GCC is use. Distros with the latest GCC will get the latest and greatest, but it will cause no real problems for distros that don't have it. This can be addressed easily when B3 is exposed to CMake.

Yusuke Suzuki

Comment 13 2016-01-28 13:48:38 PST

Seeing https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62272, I noticed that we have very very easy (and no performance regression!!!) fix for this issue. Just replacing `addEdge` in the nested lambda to `this->addEdge`. Filip, how about this?

Yusuke Suzuki

Comment 14 2016-01-28 14:50:49 PST

Created attachment 270149 [details] Patch

Yusuke Suzuki

Comment 15 2016-01-28 15:21:58 PST

Committed r195788: <http://trac.webkit.org/changeset/195788>

Csaba Osztrogonác

Comment 16 2016-01-29 01:47:32 PST

(In reply to comment #13) > Seeing https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62272, I noticed that we > have very very easy (and no performance regression!!!) fix for this issue. > Just replacing `addEdge` in the nested lambda to `this->addEdge`. Yay, we should have read this bug report in details. :) Thanks for the fix.

Note You need to log in before you can comment on or make changes to this bug.