Chris Peterson [:cpeterson] Assignee
	Description • 12 years ago

To make gcc's NULL/nullptr warnings fatal like clang's nullptr errors, we may want to add -Werror=conversion-null to Linux's CFLAGS/CXXFLAGS for gcc >= 4.5. (The -Wconversion-null flag was new in gcc 4.5.)

If a developer overlooks these gcc warnings, then they may not realize their code change will break the OS X's clang build until they push to the tryserver. This a waste of developer time and tryserver resources.

	Chris Peterson [:cpeterson] Assignee
	Comment 1 • 12 years ago

For example, given the following code:

  char a = nullptr;
  int  b = nullptr;

* gcc 4.5 emits these warnings on Linux:

  warning: converting to non-pointer type 'char' from NULL
  warning: converting to non-pointer type 'int' from NULL

* clangs emits these nullptr errors on OS X:

  error: assigning to 'char' from incompatible type 'nullptr_t'
  error: assigning to 'int' from incompatible type 'nullptr_t'

	SADINENI RAVI CHANDRA
	Comment 2 • 12 years ago

hii i would like ot work on this .. can you tell me what shld i do...

	Chris Peterson [:cpeterson] Assignee
	Comment 3 • 12 years ago

hi Sadineni, this code change should be pretty straightforward. The most time-consuming part will be configuring the build and fixing any NULL warnings that become compile errors. The only Firefox platforms that use gcc today are Linux and Android.

gcc's -Wconversion-null flag is only available in recent versions of gcc, so you will need to add a "configure" check that tests for this compiler flag at build time. We would like gcc to treat these warnings as compile errors, so we need to add "-Werror=conversion-null" to our CXXFLAGS.

The "configure" check should be added to configure.in and js/src/configure.in. It will look something like this:

--- a/configure.in
+++ b/configure.in
@@ -1477,21 +1477,23 @@ if test "$GNU_CXX"; then
     # Turn on GNU-specific warnings:
     # -Wall - turn on a lot of warnings
     # -pedantic - this is turned on below
     # -Wpointer-arith - enabled with -pedantic, but good to have even if not
     # -Woverloaded-virtual - ???
     # -Werror=return-type - catches missing returns, zero false positives
     # -Wtype-limits - catches overflow bugs, few false positives
     # -Wempty-body - catches bugs, e.g. "if (c); foo();", few false positives
+    # -Werror=conversion-null - catches conversions between NULL and other types
     #
     _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wall -Wpointer-arith -Woverloaded-virtual"
     MOZ_CXX_SUPPORTS_WARNING(-W, error=return-type, ac_cxx_has_werror_return_type)
     MOZ_CXX_SUPPORTS_WARNING(-W, type-limits, ac_cxx_has_wtype_limits)
     MOZ_CXX_SUPPORTS_WARNING(-W, empty-body, ac_cxx_has_wempty_body)
+    MOZ_CXX_SUPPORTS_WARNING(-W, error=conversion-null, ac_cxx_has_werror_conversion_null)
 
     # Turn off the following warnings that -Wall/-pedantic turn on:
     # -Wno-ctor-dtor-privacy - ???
     # -Wno-overlength-strings - we exceed the minimum maximum length frequently 
     # -Wno-invalid-offsetof - we use offsetof on non-POD types frequently
     # -Wno-variadic-macros - ???
     #
     _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-ctor-dtor-privacy"

	Chris Peterson [:cpeterson] Assignee
	Comment 4 • 12 years ago

Sadineni, have you had a chance to look at this bug? If you have any questions, just let me know.

	Chris Peterson [:cpeterson] Assignee
	Comment 5 • 12 years ago

Part 1: Fix gcc -Wconversion-null warnings in gfx/gl

Fix some gcc warnings about NULL conversions that clang treats as errors.

	Chris Peterson [:cpeterson] Assignee
	Comment 6 • 12 years ago

Part 2: Fix gcc -Wconversion-null warnings in js

Fix some gcc warnings about NULL conversions that clang treats as errors.

	Chris Peterson [:cpeterson] Assignee
	Comment 7 • 12 years ago

Part 3: Fix gcc -Wconversion-null warnings in widget/android

	Chris Peterson [:cpeterson] Assignee
	Comment 8 • 12 years ago

Part 4: Fix gcc -Wconversion-null warnings in ipc/chromium and toolkit/crashreporter

Fix some gcc warnings about NULL conversions that clang treats as errors.

	Chris Peterson [:cpeterson] Assignee
	Comment 9 • 12 years ago

Part 6: Treat gcc -Wconversion-null warnings as errors (like clang)

Here is a green try build:
https://tbpl.mozilla.org/?tree=Try&rev=cd2f6a06f90d

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 10 • 12 years ago

Comment on attachment 661419 [details] [diff] [review]
part-2-fix-js-warnings.patch

Review of attachment 661419 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +3809,5 @@
>   */
>  #ifdef JS_GC_ZEAL
>  # define JS_SET_TRACING_LOCATION(trc, location)                               \
>      JS_BEGIN_MACRO                                                            \
> +        if (!(trc)->realLocation || (location) == NULL)                       \

What's this supposed to fix?

	Chris Peterson [:cpeterson] Assignee
	Comment 11 • 12 years ago

(In reply to :Ms2ger from comment #10)
> Comment on attachment 661419 [details] [diff] [review]
> part-2-fix-js-warnings.patch
> 
> Review of attachment 661419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.h
> @@ +3809,5 @@
> >   */
> >  #ifdef JS_GC_ZEAL
> >  # define JS_SET_TRACING_LOCATION(trc, location)                               \
> >      JS_BEGIN_MACRO                                                            \
> > +        if (!(trc)->realLocation || (location) == NULL)                       \
> 
> What's this supposed to fix?

The JS_SET_TRACING_LOCATION() macro had been called with a NULL constant and, after the macro was expanded, gcc -Wconversion-null did not like |!(NULL)|.

However, it looks like this change is no longer necessary because cset 8464ed98f78b introduced a new JS_UNSET_TRACING_LOCATION() macro to fix clang's NULL-to-bool warnings:

https://hg.mozilla.org/mozilla-central/rev/8464ed98f78b

	Benjamin Smedberg
	Comment 12 • 12 years ago

Why are these patches using NULL instead of nullptr? I thought we standardized on nullptr.

	Chris Peterson [:cpeterson] Assignee
	Comment 13 • 12 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Why are these patches using NULL instead of nullptr? I thought we
> standardized on nullptr.

nsnulls had been replaced with nullptrs, but existing NULLs had not been replaced with nullptrs. In my patches that introduce NULLs, I was just following the existing coding style in each file.

	Chris Peterson [:cpeterson] Assignee
	Comment 14 • 12 years ago

Patch 2 v2 no longer changes JS_SET_TRACING_LOCATION() because cset 8464ed98f78b already fixed this issue for clang's NULL-to-bool warnings.

	Chris Peterson [:cpeterson] Assignee
	Comment 15 • 12 years ago

Landed patch 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d06085ab429

Landed patch 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49bc46738815

	Graeme McCutcheon [:graememcc]
	Comment 16 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4d06085ab429
https://hg.mozilla.org/mozilla-central/rev/49bc46738815

	Benjamin Smedberg
	Comment 17 • 12 years ago

Comment on attachment 661422 [details] [diff] [review]
part-4-fix-ipc-and-toolkit-warnings.patch

r=me for the chromium bits. Please do not check in the google-breakpad bit: we import that directly from upstream and it needs to be fixed there.

	Chris Peterson [:cpeterson] Assignee
	Comment 18 • 12 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Comment on attachment 661422 [details] [diff] [review]
> part-4-fix-ipc-and-toolkit-warnings.patch
> 
> r=me for the chromium bits. Please do not check in the google-breakpad bit:
> we import that directly from upstream and it needs to be fixed there.

I just went to submit my google-breakpad fix upstream, but they have already fixed the warning in SVN r939. <:)

Bug 791775 tried to merge breakpad SVN r1044, but it was backed out. Once bug 791775 lands, I can land patch 5 (treat-warning-as-error).

	Chris Peterson [:cpeterson] Assignee
	Comment 19 • 12 years ago

Landed patches 2 and 4a (patch 4 without breakpad change):
https://hg.mozilla.org/integration/mozilla-inbound/rev/224ba5e9fb70
https://hg.mozilla.org/integration/mozilla-inbound/rev/081b9207fe4e

Once breakpad bug 791775 lands, I can land patch 5 and close this bug.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/224ba5e9fb70
https://hg.mozilla.org/mozilla-central/rev/081b9207fe4e

	Chris Peterson [:cpeterson] Assignee
	Comment 21 • 12 years ago

Green try builds with patch 5:
https://tbpl.mozilla.org/?tree=Try&rev=445de5e1e12a

	Chris Peterson [:cpeterson] Assignee
	Comment 22 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc505740ad56

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/cc505740ad56

	:aceman
	Comment 24 • 12 years ago

Does this break comm-central by itself or do we need to port it to c-c makefiles?

	Chris Peterson [:cpeterson] Assignee
	Comment 25 • 12 years ago

(In reply to :aceman from comment #24)
> Does this break comm-central by itself or do we need to port it to c-c
> makefiles?

aceman, the configure.in change enables some gcc warnings-as-errors. comm-central shouldn't break if this patch is not applied, though changes merged from comm-central to mozilla-central may hit these new warnings-as-errors.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 12 years ago

Part 5 of this seems to break compilation with clang on Mac for me.  I can't tell how our builders are managing to build, since I'm using the same clang revision.  :(

	Daniel Holbert [:dholbert]
	Comment 27 • 11 years ago

Following up on comment 26, for archeological purposes: Bug 798800 backed out part 5, reverting the "-Werror=conversion-null" in configure.in.

We still have -Werror=conversion-null in js/src/configure.in, though, FWIW. (That surprised me -- I thought we had a build-time check that those two configure.in files were identical in order to build, but apparently this section can differ without triggering that check.)

	(not currently active) Ted Mielczarek
	Comment 28 • 11 years ago

The configure.in files are not checked, they're very different. It's only the build/ and config/ dirs that are checked:
http://mxr.mozilla.org/mozilla-central/source/client.mk#418