Bug 17175

Summary: Use of C++ compiler flags in CFLAGS
Product: WebKit Reporter: Rodney Dawes <dobey>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mrowe
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch to resolve CFLAGS vs. CXXFLAGS issues
dobey: review-
Updated patch that adds and uses global_cxxflags
alp: review-
Fix typo mrowe: review-

Description Rodney Dawes 2008-02-04 08:26:02 PST
Currently, the autotools build adds -fno-rtti and -fvisibility-inlines-hidden to CFLAGS, but they are only valid for C++ as gcc warns during the build. We also need to add -I$(srcdir)/WebCore to the global_cppflags so that the proper config.h gets found by code under WebCore. The attached patch fixes these issues.
Comment 1 Rodney Dawes 2008-02-04 08:27:09 PST
Created attachment 18908 [details]
Patch to resolve CFLAGS vs. CXXFLAGS issues
Comment 2 Rodney Dawes 2008-02-04 08:42:29 PST
Comment on attachment 18908 [details]
Patch to resolve CFLAGS vs. CXXFLAGS issues

Doh. Was just pointed out that global_cppflags is for CPPFLAGS not CXXFLAGS, and the content of global_cppflags confused me.
Comment 3 Alp Toker 2008-02-04 08:43:49 PST
Comment on attachment 18908 [details]
Patch to resolve CFLAGS vs. CXXFLAGS issues

>Index: ChangeLog
>===================================================================
>--- ChangeLog	(revision 29961)
>+++ ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2008-02-04  Rodney Dawes  <dobey@wayofthemonkey.com>
>+
>+	Reviewed by NOBODY (OOPS!!)
>+
>+	Only use -fno-rtti and $(SYMBOL_VISIBILITY_INLINES) with
>+	global_cppflags	as gcc complains they aren't valid for C
>+	Add -I$(srcdir)/WebCore to global_cppflags to get the right config.h
>+
>+	* GNUmakefile.am:
>+
> 2008-02-04  Alp Toker  <alp@atoker.com>
> 
>         Rubber-stamped by Mark Rowe.
>Index: GNUmakefile.am
>===================================================================
>--- GNUmakefile.am	(revision 29961)
>+++ GNUmakefile.am	(working copy)
>@@ -47,9 +47,11 @@ EXTRA_DIST :=
> 
> #
> # Global flags to CPP
>-global_cppflags :=
>+global_cppflags := \
>+	$(SYMBOL_VISIBILITY_INLINES)

cppflags is for preprocessor options, not C++ flags. This won't help.

Better solution is to clean up the flags and layout of the build modules.
Comment 4 Rodney Dawes 2008-02-04 08:55:05 PST
Created attachment 18909 [details]
Updated patch that adds and uses global_cxxflags
Comment 5 Alp Toker 2008-02-04 09:01:08 PST
Comment on attachment 18909 [details]
Updated patch that adds and uses global_cxxflags

>Index: JavaScriptCore/GNUmakefile.am
>===================================================================
>--- JavaScriptCore/GNUmakefile.am	(revision 29961)
>+++ JavaScriptCore/GNUmakefile.am	(working copy)
>@@ -149,14 +149,14 @@ Programs_minidom_SOURCES = \
> 	JavaScriptCore/API/NodeList.c \
> 	JavaScriptCore/API/minidom.c
> Programs_minidom_CPPFLAGS = $(global_cppflags)
>-Programs_minidom_CXXFLAGS = $(global_cflags)
>+Programs_minidom_CXXFLAGS = $(globacl_cxxflags) $(global_cflags)

Typo. Please test before requesting review.
Comment 6 Rodney Dawes 2008-02-04 09:07:18 PST
Created attachment 18910 [details]
Fix typo
Comment 7 Alp Toker 2008-02-04 18:06:39 PST
Comment on attachment 18910 [details]
Fix typo

Looks good
Comment 8 Mark Rowe (bdash) 2008-02-04 18:08:57 PST
Comment on attachment 18910 [details]
Fix typo

It seems that this patch will cause JavaScriptCore to use WebCore's config.h.  Why is that any better than WebCore using JavaScriptCore's config.h?
Comment 9 Alp Toker 2008-02-04 18:11:34 PST
Comment on attachment 18910 [details]
Fix typo

Clearing r+, I missed the -I addition which doesn't look right.
Comment 10 Mark Rowe (bdash) 2008-02-04 18:26:55 PST
The -IWebCore and -I JavaScriptCore's in global_cppflags look incorrect. There are no headers in the top-level JavaScriptCore folder, and JavaScriptCore should definitely not be looking for any headers inside WebCore.  If there's an issue there that needs fixed it should be addressed separately.  A patch without that change would be great to get landed.
Comment 11 Mark Rowe (bdash) 2008-02-04 19:28:13 PST
I landed the relevant portion of this in r29986.