Fix the B3 build with GCC 4.9.3

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

../../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 ?

[Csaba Osztrogonác]

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]

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 on attachment 266188 [details]
Patch

I'm strongly opposed to this fix. std::function requires extra heap allocation in many cases.

[Csaba Osztrogonác]

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

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

Created attachment 267107 [details]
Patch

updated the same workaround to ToT

[WebKit Commit Bot]

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]

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

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

Created attachment 268537 [details]
Patch

updated the same workaround to ToT

[Csaba Osztrogonác]

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

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]

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]

Created attachment 270149 [details]
Patch

[Yusuke Suzuki]

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

[Csaba Osztrogonác]

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