Bug 17175 – Use of C++ compiler flags in CFLAGS

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Rodney Dawes]

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.

[Rodney Dawes]

Created attachment 18908 [details]
Patch to resolve CFLAGS vs. CXXFLAGS issues

[Rodney Dawes]

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.

[Alp Toker]

Comment on attachment 18908 [details]
Patch to resolve CFLAGS vs. CXXFLAGS issues

>Index: ChangeLog
>===================================================================
>--- ChangeLog	(revision 29961)
>+++ ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>[phone]  Rodney Dawes  <[e-mail]>
>+
>+	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  <[e-mail]>
> 
>         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.

[Rodney Dawes]

Created attachment 18909 [details]
Updated patch that adds and uses global_cxxflags

[Alp Toker]

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.

[Rodney Dawes]

Created attachment 18910 [details]
Fix typo

[Alp Toker]

Comment on attachment 18910 [details]
Fix typo

Looks good

[Mark Rowe (bdash)]

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?

[Alp Toker]

Comment on attachment 18910 [details]
Fix typo

Clearing r+, I missed the -I addition which doesn't look right.

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

I landed the relevant portion of this in r29986.