Bug 127909 - [GTK] Disable optimizations for JSC that turned out malignant after jsCStack branch merge
Summary: [GTK] Disable optimizations for JSC that turned out malignant after jsCStack ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 127777
  Show dependency treegraph
 
Reported: 2014-01-30 07:45 PST by Zan Dobersek
Modified: 2014-01-31 10:58 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.33 KB, patch)
2014-01-30 07:51 PST, Zan Dobersek
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-01-30 07:45:35 PST
[GTK] Disable optimizations for JSC that turned out malignant after jsCStack branch merge
Comment 1 Zan Dobersek 2014-01-30 07:51:30 PST
Created attachment 222668 [details]
Patch
Comment 2 Csaba Osztrogonác 2014-01-30 08:11:07 PST
-fno-omit-frame-pointer is reasonable, but I'm a little bit sceptic about
-fno-tree-dce . Of course it can be good as a workaround, but it would be
great to find the root of this problem. Could you possibly add a detailed 
comment with bug URL to the GNUmakefile.am too?

I'm not so familiar with autotools files, so I'd like 
to let the final review for the GTK maintainers.
Comment 3 Carlos Garcia Campos 2014-01-30 08:14:28 PST
(In reply to comment #2)
> -fno-omit-frame-pointer is reasonable, but I'm a little bit sceptic about
> -fno-tree-dce . Of course it can be good as a workaround, but it would be
> great to find the root of this problem. Could you possibly add a detailed 
> comment with bug URL to the GNUmakefile.am too?
> 
> I'm not so familiar with autotools files, so I'd like 
> to let the final review for the GTK maintainers.

I agree, let's land this just as a workaround, but we should re-enable the optimizations at some point. Maybe we could file bug reports for that.
Comment 4 Alberto Garcia 2014-01-30 08:16:42 PST
I think that should go to CFLAGS (compiler flags), not CPPFLAGS
(preprocessor flags).

--- Source/JavaScriptCore/GNUmakefile.am	(revision 163081)
+++ Source/JavaScriptCore/GNUmakefile.am	(working copy)
@@ -70,6 +70,8 @@ javascriptcore_cppflags += \
 	-I$(srcdir)/Source/JavaScriptCore/yarr \
 	-I$(top_builddir)/DerivedSources/JavaScriptCore \
 	-I$(srcdir)/Source/WTF \
+	-fno-omit-frame-pointer \
+	-fno-tree-dce \
 	$(LLVM_CFLAGS)
 
 javascriptcore_cflags += \
Comment 5 Alberto Garcia 2014-01-30 08:19:52 PST
(In reply to comment #4)
> I think that should go to CFLAGS (compiler flags), not CPPFLAGS
> (preprocessor flags).

CXXFLAGS actually, or are we using CPPFLAGS to make sure that they're
included in both cases? :)
Comment 6 Zan Dobersek 2014-01-30 08:28:19 PST
(In reply to comment #5)
> (In reply to comment #4)
> > I think that should go to CFLAGS (compiler flags), not CPPFLAGS
> > (preprocessor flags).
> 
> CXXFLAGS actually, or are we using CPPFLAGS to make sure that they're
> included in both cases? :)

Yes, we're trying to cover both cases.
Comment 7 Alberto Garcia 2014-01-30 08:48:20 PST
My clang installation (3.4) doesn't seem to like this flag:

clang: error: unknown argument: '-fno-tree-dce'
Comment 8 Geoffrey Garen 2014-01-30 11:01:09 PST
> -fno-omit-frame-pointer is reasonable, but I'm a little bit sceptic about
> -fno-tree-dce . 

Me too.
Comment 9 Zan Dobersek 2014-01-30 11:18:00 PST
(In reply to comment #7)
> My clang installation (3.4) doesn't seem to like this flag:
> 
> clang: error: unknown argument: '-fno-tree-dce'

Already addressed in bug #127911.
Comment 10 Zan Dobersek 2014-01-30 11:18:45 PST
(In reply to comment #8)
> > -fno-omit-frame-pointer is reasonable, but I'm a little bit sceptic about
> > -fno-tree-dce . 
> 
> Me too.

The optimization is only problematic with GCC 4.8 but not with GCC 4.7, so this might be a bug introduced in the compiler.
Comment 11 Zan Dobersek 2014-01-31 10:58:30 PST
Landed in r163083.
http://trac.webkit.org/changeset/163083