LWN: Comments on "Wielaard: Looking forward to GCC6 – Many new warnings"

Wielaard: Looking forward to GCC6 – Many new warnings

bronson — Thu, 03 Mar 2016 23:00:51 +0000

Hyper-optimized codecs and graphics code... but that's still very specialized. Anyone who isn't sure if -Werror is necessary should ship with it off.

Wielaard: Looking forward to GCC6 – Many new warnings

nix — Thu, 03 Mar 2016 21:20:21 +0000

There is one special case where this doesn't apply: where the software is arch-dependent enough and delicate enough that it should only be compiled with specific compilers (or with compilers in a specific version range) and on supported arches. This is particularly true where the software is crucial and any new warnings are quite likely a sign of impending disaster. On such systems, -Werror is actually really useful.

glibc is the only such software I can think of right now, on Linux at least.

Wielaard: Looking forward to GCC6 – Many new warnings

jwakely — Thu, 25 Feb 2016 09:44:59 +0000

That used to be true, but isn't now, see http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.h...

> A null pointer constant is an integer literal (2.13.2 [lex.icon]) with value zero or a prvalue of type std::nullptr_t.

So false and (0+0) and (1-1) are no longer null pointer constants and cannot be implicitly converted to pointer types.

If you want a pointer then use a pointer, not a bool or some other type.

Wielaard: Looking forward to GCC6 – Many new warnings

HelloWorld — Thu, 25 Feb 2016 06:59:15 +0000

Yeah, so? Using nulptr here rather than false would lead to the exact same runtime behaviour, so how is this a genuine problem?

Wielaard: Looking forward to GCC6 – Many new warnings

epa — Mon, 22 Feb 2016 10:07:18 +0000

Oh I agree, I certainly see the value in making sure there are zero warnings during development - I just don't see why the source tarball given to distributions and end users needs to have fatal warnings set. There may well be warnings which pop up on some other architecture, but they are probably just warnings rather than hard errors, and it is the user's or packager's choice whether to treat them as serious enough to abandon the build.

Wielaard: Looking forward to GCC6 – Many new warnings

ms_43 — Sun, 21 Feb 2016 13:48:36 +0000

> aiui, LibreOffice actually has config options in the build system for "each" distro.

Where "distro" means "Linux", "MacOSX", "Win32" or "Android".

https://cgit.freedesktop.org/libreoffice/core/tree/distro...

Ceterum Censeo

ms_43 — Sun, 21 Feb 2016 13:34:14 +0000

> new[] initializes values to the default constructor of the array subscript type

yes (if an explicit initialization expression is omitted).

https://github.com/cplusplus/draft/blob/master/source/exp...

> (zeroes for integral and floating-point types;

no, that would be zero-initialization!

operator new performs default-initialization, which calls an applicable no-argument constructor for class types, otherwise does nothing.

https://github.com/cplusplus/draft/blob/01c0772e922e5edcd...

Ceterum Censeo

bronson — Fri, 19 Feb 2016 19:12:41 +0000

It was way better than the guesses itvirta posted though! And nice and succinct.

Ceterum Censeo

cortana — Fri, 19 Feb 2016 14:13:08 +0000

Your version of the story misses out several details that were covered in Debian, OpenSSL, and a lack of cooperation posted to a certain web site some time ago.

Wielaard: Looking forward to GCC6 – Many new warnings

dirtyepic — Fri, 19 Feb 2016 00:18:52 +0000

In half the cases it's not a new option throwing a warning at you, it's an old option that learned some new tricks (or likely, some new false positives).

Ceterum Censeo

Sesse — Thu, 18 Feb 2016 21:54:04 +0000

That's a false dichotomy. What's much more likely is that someone used a different compiler which has a different set of warnings. Not all the world is GCC.

Most warnings, especially in code that's been around for a while, are _not_ signs of actual bugs. (It's good style to fix them nevertheless, because some are, but that should not block the compile for an end user.)

/* Steinar */

Ceterum Censeo

mpr22 — Thu, 18 Feb 2016 21:48:06 +0000

My C and C++ code builds without warnings on my system, because I do all my development with -Werror enabled. Thus, if my C and C++ code produces warnings when compiled on someone else's system, then either their toolchain is (defective|deficient|misconfigured) or there is an issue with my code which needs to be properly examined by a human, and that someone else should probably not be distributing binaries to other people until it has been.

Wielaard: Looking forward to GCC6 – Many new warnings

mjw — Thu, 18 Feb 2016 17:58:23 +0000

But that is the whole point isn't it? If your setup is so different from the supported setups of the project as released that you get new warnings then you do need to carefully check them and ideally discuss with the project maintainers how to resolve them. In my experience that is exactly what happens. Distros that have architecture support not yet seen by the project contribute fixes back and we work together to make sure newer compiler warnings are fixed (which often means there was some subtle bug in the code in the first place). It grows the project to include these new environments as supported. And it makes sure no subtle bugs slip in because people ignore warnings. IMHO it is exactly as making sure your testsuite is zero fail and if not to abort the build so nothing is ever released that is known broken.

Wielaard: Looking forward to GCC6 – Many new warnings

pizza — Thu, 18 Feb 2016 13:44:26 +0000

> I am somewhat surprised by the hostility against -Werror.

It's awesome for the tail end of development work leading up to a release, but IMO it has no place in a release package if that package is intended to be compiled by random third parties. The same code on different versions of the compiler (or even the same compiler on different platforms) will result in different sets of warnings.

Wielaard: Looking forward to GCC6 – Many new warnings

mjw — Thu, 18 Feb 2016 10:02:24 +0000

I am somewhat surprised by the hostility against -Werror. Personally I use it religiously in my projects and take pride in having the build explicitly break if a warning slips through. I do select the set of warnings (normally at least -Wall and -Wextra and then some of the non-default ones after making sure they catch some real bugs, and then fix any false positive, like some of those in this blog post did). My experience with distros is actually pretty positive. Package maintainers do warn (pun intended!) us when the build breaks because a new warning is detected on some setup we didn't test yet ourselves and we work together to get the build clean again. In fact one real bug was found when Debian tried a mass rebuild with gcc6 because of -Wmisleading-indentation. It was an obscure bug that would probably not trigger in normal situations, but it was nice to find it because it would have been a pain to diagnose otherwise. You do have to have a good relationship with the package maintainers of your project to make -Werror work, but then it really is IMHO a pretty sweet deal.

Wielaard: Looking forward to GCC6 – Many new warnings

mjw — Thu, 18 Feb 2016 09:49:23 +0000

I agree that -Weverything is pretty silly. Not all warnings are equal. Some are like -Wtemplate or -Weffc++ are really about coding style (and don't normally signal a possible bug). And then there are warnings like -Wwrite-strings that change the type of some constructs effectively changing the code. Most people are probably not interested in such warnings in the first place.

But for other categories of warnings (where there is the potential that it does indeed signal a subtle bug that might need some analysis even if it could be a false positive) it would be really nice to have some way to enable them all and just see what warning results you get. Is it really too hard to define two or three categories of warnings that can easily be enabled all at once?

I certainly appreciate the GCC documentation. It is pretty good. And it was fun to compare the info pages of the current and new version of GCC to get an overview of all the new warning options to try out. I learned a lot! But an easier way to discover new warnings (especially those not enabled by default or through -Wall or -Wextra) would be appreciated.

Ceterum Censeo

tialaramex — Wed, 17 Feb 2016 23:58:06 +0000

Version of the story which is popular and you're repeating to us:

* OpenSSL is so crappy it relies on uninitialised memory reads to do random number generation even though, er, that can't work on Unix
* Hero Debian Guy fixes the uninitialised reads
* Somehow this breaks OpenSSL because it needed uninitialised reads, even though er, they didn't work so... um.

Actual reality of the story

* OpenSSL is so crappy it has several similar-looking C functions that actually do different stuff
* Debian Guy spots that function A does a harmless uninitialised read and fixes it, arguably making the code more "correct" but with no functional change.
* Idiot Debian Guy sees a similar function B and blindly applies the same "fix".
* The "fix" to this function B disables its entire purpose, effectively removing the PRNG feature almost entirely

I don't know if either story has anything to tell us about GCC6. But I do know that spreading the former story is harmful.

Ceterum Censeo

itvirta — Wed, 17 Feb 2016 21:20:29 +0000

Well, of course commenting out lines without understanding what they do is either silly or plain stupid.

But in that particular case, the ultimate reason seems to be that someone tried to be smarter than they should have
been, and did something that is both wrong (in the language sense) and not very useful either.

Wrong, since as far as I can find, the point was to read uninitialized memory, and that results in undefined behaviour
with the full demons-flying-from-ones-mouth possibility included.

Not very useful since the whole thing was to "safeguard" against not having a good RNG by creating a really shitty RNG.
Same goes for the fact that the library used the PID to seed the RNG. PIDs aren't really secret or random and make for
a rather shitty RNG. Had the library not used such a weak addition, the final screw-up might have been detected sooner,
since all the outputs would have been exactly the same, instead of having the 2^15 different possibilities (or whatever
the number was).

All of this goes with a big IIRC disclaimer, so feel free to point me wrong.
However, I do find the custom of solely blaming a silly mistake while ignoring the fucked-up design really tiring.

Wielaard: Looking forward to GCC6 – Many new warnings

Sesse — Wed, 17 Feb 2016 21:09:31 +0000

Sure, when all upstream app developers commit to making their own packages for the 10–15 architectures people care about on Linux, and keep them up-to-date wrt. security and such.

/* Steinar */

Re: In C++, you need type information as part of the ABI.

ldo — Wed, 17 Feb 2016 21:00:19 +0000

The ABI can only handle a small subset of the necessary type information.

Consider the Ada declaration

type B is new A;

which means that type B is essentially a copy of type A, inheriting all the functionality defined for it. But B must still be considered distinct from A, and incompatible with it.

Then you have subtypes, which are considered compatible with the type they derive from, but are subject to additional constraints.

There can be no code or runtime data associated with these definitions, which is why you cannot represent them in any ABI. Yet they need to be processed across compilation units. Isn’t that a natural job for the linker?

Ceterum Censeo

ksandstr — Wed, 17 Feb 2016 20:52:58 +0000

Whether it's using new[] or malloc is, in fact, very important here: new[] initializes values to the default constructor of the array subscript type (zeroes for integral and floating-point types; the same rule is applied to all members of structs to make up an implicit default constructor), while malloc() does no initialization at all.

So if you're seeing consistent results, you're either using new[] or malloc() in a process that hasn't freed much of the memory it's allocated and written yet. But sure enough, valgrind does complain about uninitialized values when reading from new[]'d arrays -- this should be reported and corrected.

Back in 2006 it was still realistic that valgrind wouldn't become yet another mindlessly-followed lint. It had innovations like data tracking instrumentation and what-not. Alas.

Ceterum Censeo

juliank — Wed, 17 Feb 2016 19:43:18 +0000

Thing is, we don't really mmap() for writing anymore (only for reading), but use new[] instead (or malloc, idk right now). We then do a nearly-atomic write to temporary cache & rename transaction.

Valgrind currently complains about that, but oh well, it's at least clearly identifiable. Should still get fixed at some point.

Many new warnings, but still not my pet peeve

ksandstr — Wed, 17 Feb 2016 19:31:17 +0000

Somewhat relatedly, this issue recently won the 2015 Underhanded C Competition.

Ceterum Censeo

ksandstr — Wed, 17 Feb 2016 19:20:32 +0000

However, POSIX initializes fresh mmap() memory to all zeroes, but uninitialized automatic arrays in a function are truly uninitialized in that they contain arbitrary detritus from the thread stack (whatever that was un-/initialized to). In essence, APT performs a well-defined function that yields consistent results despite "you" thinking it doesn't.

Ceterum Censeo

bronson — Wed, 17 Feb 2016 16:05:06 +0000

The direct reason the SSL bug entered Debian was to get rid of the warning.

Or, if you're saying that it's normal to go commenting out lines of code to eradicate warnings -- because it's ultimately the fault of the person who wrote the questionable code -- then I guess this is a fine example of the problem. Thank you!

Remember, warning-driven development only works when you're using a perfect compiler.

Wielaard: Looking forward to GCC6 – Many new warnings

andresfreund — Wed, 17 Feb 2016 15:45:32 +0000

Indeed. Similar false positives are common with -Wtype-limits as well. E.g. a somechar >= 0, where common platforms will spew warnings about the check being redundant, but for a signed char the check might be important. Dealing with warnings like these can be quite frustrating :(

Wielaard: Looking forward to GCC6 – Many new warnings

itvirta — Wed, 17 Feb 2016 14:13:48 +0000

Of course that particular check should probably be written as (value > min(limit1, limit2))
since that's what logically happens (and the optimiser will happily remove the non-dominating bound anyway).

Apparently a warning is not given if the limits are different, even though that might
catch the case where check was supposed to be (value > limit1 || value2 > limit2) but the other
variable name was mistyped.

Wielaard: Looking forward to GCC6 – Many new warnings

mathstuf — Wed, 17 Feb 2016 12:09:44 +0000

C++11 introduced nullptr which allows pointer logic with NULL to be much stricter. Relevant links:

https://stackoverflow.com/questions/32009694/why-implicit...
https://www.mail-archive.com/gcc-bugs@gcc.gnu.org/msg4645...

Ceterum Censeo

juliank — Wed, 17 Feb 2016 12:05:06 +0000

Fun fact: We use uninitialized memory in APT. That is, for the binary (mmap()able) cache, we allocate memory and initialise parts of that.

We store all of it though, and calculate a checksum of it all, including the not yet initialised parts.

Ceterum Censeo

itvirta — Wed, 17 Feb 2016 11:29:17 +0000

That would be the type of people who deliberately access uninitialized memory.
They are surely not to be trusted, but that has little to do with warnings.

Wielaard: Looking forward to GCC6 – Many new warnings

HelloWorld — Wed, 17 Feb 2016 10:36:57 +0000

> One was a compile error caused by returning "false" instead of a nullptr
Why would this be a problem? Every integral constant-expression with a value of 0 is a valid null pointer literal. It's certainly uncommon to use false for this purpose, but probably not a real problem.

Re: the linker doesn't care much about matching symbols

niner — Wed, 17 Feb 2016 08:06:38 +0000

"it's the tools' job to properly unmangle names."

That's the problem right there: the assumption, that there is "the" tool, rather than multiple tools. With multiple tools, they have to agree on how to mangle the names. If that's standardized (even if it's just de facto) like the bit of mangling that C does, interoperation is easy. That's why I can call an abritrary C library's functions from Perl 6 without even needing a C compiler on the machine[1]. Binding to C++ libraries on the other hand is really hard. It needs special code to support each compiler[2].

[1] http://doc.perl6.org/language/nativecall
[2] https://github.com/rakudo/rakudo/blob/nom/lib/NativeCall/...

Ceterum Censeo

nix — Wed, 17 Feb 2016 00:17:19 +0000

You mean the sort of person who makes compiler warnings fatal on every machine the thing is compiled on, even though many warnings vary per-architecture and almost all vary per-compiler-version?

Yeah, I'd trust that sort of person a whole lot -- to make mistakes through ignorance.

Wielaard: Looking forward to GCC6 – Many new warnings

rahvin — Tue, 16 Feb 2016 23:00:12 +0000

It didn't lead to misbehavior that you know of. My experience with computers and software in general is if you let the computer make a decision about something (such as exactly where the brace ending the If statement actually is) the computer will assume something completely random that doesn't make any sense. Those are the kind of errors that lead to untraceable bugs because the compiler made an assumption about where the IF statement ended that and you know you would never see that missing brace while troubleshooting.

Though I have to admit that error name isn't very descriptive.

Wielaard: Looking forward to GCC6 – Many new warnings

Wol — Tue, 16 Feb 2016 22:28:42 +0000

aiui, LibreOffice actually has config options in the build system for "each" distro.

So you can take an upstream git pull, and run make for your distro. So the distro can tweak things, and they go upstream so the user can see what's going on - that is, if they can understand the build system ... :-)

Cheers,
Wol

Wielaard: Looking forward to GCC6 – Many new warnings

Wol — Tue, 16 Feb 2016 22:21:11 +0000

> Personally I can't see the value in fatal warnings

Having had some warnings that I couldn't get rid of, it can be a real problem, but I'd rather have fatal warnings switched on if I can. That way, I know my code is as clean as possible, and having taken over code that originally had minimal warnings and lots of subtle bugs, that's the way I like it ... :-)

Cheers,
Wol

Wielaard: Looking forward to GCC6 – Many new warnings

fratti — Tue, 16 Feb 2016 21:35:09 +0000

I highly recommend giving the SVN version a spin - while the >1 hour it took me to build GCC, it found 3 actual problems in the first codebase I ran it on. One was a compile error caused by returning "false" instead of a nullptr, two others were caught by -Wmisleading-indentation and were cases of forgotten curly braces around if-conditions.

As far as I can tell, none of the uncovered issues had a real world impact; while the code was different than intended it did not lead to program misbehaviour. However, it'll probably save some headaches if the surrounding code were to be refactored.

Wielaard: Looking forward to GCC6 – Many new warnings

xtifr — Tue, 16 Feb 2016 21:07:46 +0000

Hmm, I can see the new meaning of -Wlogical-op causing some problems with platform-specific code. Imagine:

#if defined(PLATFORM1)
#define LIMIT1 64
#define LIMIT2 32
#elif defined(PLATFORM2)
#define LIMIT1 32
#define LIMIT2 64
#elif defined(PLATFORM3)
#define LIMIT1 32
#define LIMIT2 32
#endif
...
if (value > LIMIT1 || value > LIMIT2) ...

On Platform 3, this would generate a warning about a redundant comparison (error with -Werror). Of course, there are numerous ways to work around this problem, but, still...

Re: All Ada compilers I had worked with (around 20 years ago…) also used name mangling.

ldo — Tue, 16 Feb 2016 20:02:10 +0000

GNAT does not.

Re: the linker doesn't care much about matching symbols

darktjm — Tue, 16 Feb 2016 19:11:29 +0000

Yes, I mean type consistency between object modules for exported functions and data objects.

Ada was designed for type safety in this regard, and C wasn't. Simple as that. It's one of the many features that I love(d) about Ada. You could give C the benefit of the doubt, having been designed 10 years earlier than Ada. At least it's more type-safe than its predecessors. In any case, I'm not talking about doing this in the linker, although that is possible if debugging info with type information is used for that purpose (or if a GCC-specific section type were to be added for this purpose, I guess), and it would be better/more secure than the warnings I'm proposing. The warnings I'm proposing are simply to ensure that you declare all globals in a header file. That alone does not guarantee that the same header file is used to declare the globals the same way in multiple separate modules, but it does prevent people from sloppily declaring the same function/global data object in every C file and then forgetting to change it later on when the original changes (or doing it wrong in the first place). It also prevents library conflicts due to excessive exports, since you are more aware of what you're exporting. In other words, I'm proposing a warning enforcing good coding practice (which a linker fix would not do, by the way), similar to the indentation warning mentioned in the summary.