205056 - add -Wwrite-strings to gcc compiler options

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should add -Wwrite-strings to the compiler options we use for gcc. 
The gcc info page describes it as:

`-Wwrite-strings'
     When compiling C, give string constants the type `const
     char[LENGTH]' so that copying the address of one into a
     non-`const' `char *' pointer will get a warning; when compiling
     C++, warn about the deprecated conversion from string constants to
     `char *'.  These warnings will help you find at compile time code
     that can try to write into a string constant, but only if you have
     been very careful about using `const' in declarations and
     prototypes.  Otherwise, it will just be a nuisance; this is why we
     did not make `-Wall' request these warnings.

However, I think we should try to reduce the number of warnings in current
builds *first*.  I've been building with it for a few weeks, and it's not too
noisy, but there are some files that yield a bunch of warnings.

We also need to check if this option exists on all the versions of gcc that we
support.

Comment 1

[Alec Flett]

I like this a lot. I personally would say we should just turn this on now - I
don't see a reason to wait - the more warnings we make visible, the sooner
people will fix them (and it distributes the load so myself or dbaron aren't
cleaning them all up)

Comment 2

[Bradley Baetz (:bbaetz)]

Note that anyone who actually writes to these strings is going to cause a
segfault anyway, so I'd be surprised if this was a big issue.

Comment 3

[tenthumbs]

I rebuilt my 5-10 tree with -Wwrite-strings and there were 2063 additional
warnings, 1494 from C++ files and the rest from C files. Many of the C++ files
had multiple warnings. Here are the top ten:

   20 check_off.xpm:22
   21 check_on.xpm:23
   40 gtkmozembed2.cpp:162
   49 nsXIContext.cpp:196
   58 nsXPLookAndFeel.cpp:189
   60 nsInstallResources.cpp:76
   96 nsObjectFrame.cpp:2729
  124 nsGtkMenu.cpp:164
  305 splash.xpm:307
  321 logo.xpm:323

That's an awful lot of noise. The C warnings are much more scattered.

It seems to me that you would get more useful results by adding -O so gcc will
do data flow analysis.

Comment 4

[Bradley Baetz (:bbaetz)]

Roughly a third (including the top two offenders) are from xpm files. Those are
generated by various programs, so we can't really do anything about them
directly. The next entry comes from GtkFactoryEntry, which is a struct we don't
define, with gchar* members, not const gchar*. GTK2 is the same. I don't know if
these are modifyable under some circumstances.

I guess that this is why the warning isn't on by default...

Comment 5

[Gregory Szorc [:gps]]

build/moz.configure/warnings.configure is setting -Wwrite-strings. Not sure when it was introduced. But this bug is definitely implemented.