Bug 140609 – [EFL][GTK] Stop compiling with fno-omit-frame-pointer, -fno-tree-dce

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[EFL][GTK] Stop compiling with -fno-tree-dce

[Zan Dobersek]

Created attachment 244883 [details]
Patch

[Zan Dobersek]

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.

[Csaba Osztrogonác]

Great news, let me check it on EFL.

[Zan Dobersek]

The tested versions of GCC were:
- 4.7.4
- 4.8.4
- 4.9.2

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Zan Dobersek]

Awesome, thanks for helping out!

In the future if problems arise in this area, we would ideally disable optimizations only for the affected architectures.

[Zan Dobersek]

Created attachment 244898 [details]
Patch

[Csaba Osztrogonác]

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.

[Sergio Villar Senin]

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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Can we remove the -Wno-error=uninitialized workaround too from here?

http://trac.webkit.org/browser/trunk/Source/cmake/WebKitHelpers.cmake?rev=172894#L31

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Zan, could you land it in the near future?

[Zan Dobersek]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Already landed in http://trac.webkit.org/changeset/179110