Bug 140609

Summary: [EFL][GTK] Stop compiling with fno-omit-frame-pointer, -fno-tree-dce
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: akiss, ggaren, gyuyoung.kim, msaboff, ossy, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142042    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch ossy: review+

Description Zan Dobersek 2015-01-19 01:49:00 PST
[EFL][GTK] Stop compiling with -fno-tree-dce
Comment 1 Zan Dobersek 2015-01-19 01:52:53 PST
Created attachment 244883 [details]
Patch
Comment 2 Zan Dobersek 2015-01-19 01:55:34 PST
The main implication is that we insist on using the latest releases from the last three stable series of the GCC compiler. I tested all three, and there were no problems.

-fno-omit-frame-pointer could perhaps be removed as well, since without it there are still no problems. I'd like to check it on ARM before that, though. (-fno-tree-dce was specific to x86-64.)

The origin of all this is bug #127777.
Comment 3 Csaba Osztrogonác 2015-01-19 01:57:42 PST
Great news, let me check it on EFL.
Comment 4 Zan Dobersek 2015-01-19 02:26:02 PST
The tested versions of GCC were:
- 4.7.4
- 4.8.4
- 4.9.2
Comment 5 Csaba Osztrogonác 2015-01-19 02:29:18 PST
(In reply to comment #2)
> -fno-omit-frame-pointer could perhaps be removed as well, since without it
> there are still no problems. I'd like to check it on ARM before that,
> though. (-fno-tree-dce was specific to x86-64.)
> 
> The origin of all this is bug #127777.

Can we really remove -fno-omit-frame-pointer? I thought JSC still 
relies on it. ( https://bugs.webkit.org/show_bug.cgi?id=127777#c15 ) 

Was something changed in JSC related to this issue?
Comment 6 Csaba Osztrogonác 2015-01-19 03:09:21 PST
I tested EFL on Ubuntu 14.04 X86_64 with the stock GCC 4.8.2, all 
tests pass after removing -fno-omit-frame-pointer and -fno-tree-dce .

ARM and Thumb2 testing are still in progress.
Comment 7 Csaba Osztrogonác 2015-01-19 04:30:27 PST
(In reply to comment #6)
> ARM and Thumb2 testing are still in progress.

Tests are finished, all tests passed on ARM and Thumb2,
(except the flakey timeouts on Thumb2, it wasn't changed)
we can remove -fno-omit-frame-pointer and -fno-tree-dce too.
Comment 8 Zan Dobersek 2015-01-19 05:12:38 PST
Awesome, thanks for helping out!

In the future if problems arise in this area, we would ideally disable optimizations only for the affected architectures.
Comment 9 Zan Dobersek 2015-01-19 05:19:00 PST
Created attachment 244898 [details]
Patch
Comment 10 Csaba Osztrogonác 2015-01-19 05:37:39 PST
Comment on attachment 244898 [details]
Patch

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

r=me

> Source/cmake/OptionsEfl.cmake:263
>  # Push of rbp is needed after JSC JIT uses CStack.

Please remove this comment before landing.
Comment 11 Sergio Villar Senin 2015-01-19 05:41:38 PST
(In reply to comment #2)
> The main implication is that we insist on using the latest releases from the
> last three stable series of the GCC compiler. I tested all three, and there
> were no problems.
> 
> -fno-omit-frame-pointer could perhaps be removed as well, since without it
> there are still no problems. I'd like to check it on ARM before that,
> though. (-fno-tree-dce was specific to x86-64.)

This is important enough to be sent to the list IMO.
Comment 12 Csaba Osztrogonác 2015-01-19 06:55:59 PST
I played a little bit with bisecting which revisions fixed the issue.

Without -fno-omit-frame-pointer and -fno-tree-dce options, there were
~ 1800-2000 failures previously, but https://trac.webkit.org/changeset/173282
fixed most of the failures, and https://trac.webkit.org/changeset/173298 fixed
the remaining 78 failures on x86_64. I think these changes fixed the issues
on ARM and Thumb2 too, but I haven't checked yet, only the ToT revision.

It would be better to reference to this changesets in the changelog 
instead of unlikely GCC changes/fixed.
Comment 13 Csaba Osztrogonác 2015-01-19 10:16:17 PST
(In reply to comment #12)
> I played a little bit with bisecting which revisions fixed the issue.
> 
> Without -fno-omit-frame-pointer and -fno-tree-dce options, there were
> ~ 1800-2000 failures previously, but https://trac.webkit.org/changeset/173282
> fixed most of the failures, and https://trac.webkit.org/changeset/173298
> fixed
> the remaining 78 failures on x86_64. I think these changes fixed the issues
> on ARM and Thumb2 too, but I haven't checked yet, only the ToT revision.
> 
> It would be better to reference to this changesets in the changelog 
> instead of unlikely GCC changes/fixed.

I validated, r173282 fixed most of the failures on ARM and Thumb2 too,
and r173298 fixed the remaining failures.
Comment 14 Csaba Osztrogonác 2015-01-22 01:42:50 PST
Can we remove the -Wno-error=uninitialized workaround too from here?

http://trac.webkit.org/browser/trunk/Source/cmake/WebKitHelpers.cmake?rev=172894#L31
Comment 15 Csaba Osztrogonác 2015-01-22 02:19:13 PST
(In reply to comment #14)
> Can we remove the -Wno-error=uninitialized workaround too from here?
> 
> http://trac.webkit.org/browser/trunk/Source/cmake/WebKitHelpers.
> cmake?rev=172894#L31

I checked, we can remove -Wno-error=uninitialized and -Wno-error=literal-suffix too.
Comment 16 Csaba Osztrogonác 2015-01-23 08:49:03 PST
Zan, could you land it in the near future?
Comment 17 Zan Dobersek 2015-01-26 01:36:59 PST
(In reply to comment #15)
> (In reply to comment #14)
> > Can we remove the -Wno-error=uninitialized workaround too from here?
> > 
> > http://trac.webkit.org/browser/trunk/Source/cmake/WebKitHelpers.
> > cmake?rev=172894#L31
> 
> I checked, we can remove -Wno-error=uninitialized and
> -Wno-error=literal-suffix too.

In a separate bug, please.
Comment 18 Csaba Osztrogonác 2015-01-26 01:44:35 PST
(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Can we remove the -Wno-error=uninitialized workaround too from here?
> > > 
> > > http://trac.webkit.org/browser/trunk/Source/cmake/WebKitHelpers.
> > > cmake?rev=172894#L31
> > 
> > I checked, we can remove -Wno-error=uninitialized and
> > -Wno-error=literal-suffix too.
> 
> In a separate bug, please.

It's ok for me to do it in a separated bug.
Comment 19 Csaba Osztrogonác 2015-02-26 04:58:28 PST
Already landed in http://trac.webkit.org/changeset/179110