Bug 71892 - Recent optimization changes introduce bugs
Summary: Recent optimization changes introduce bugs
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-15 08:19 UTC by Kern Sibbald
Modified: 2016-08-27 15:13 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kern Sibbald 2016-07-15 08:19:09 UTC
The optimizations that you made to g++ 6.0 are not reasonable since they create programming bugs.  This applies to two optimizations you have made.  One disallowing this to be tested against NULL and the second elimination of what you determine to be unnecessary memory clearing with memset. 

In both case, the assumptions you make are not valid and thus these optimizations should not be made at low levels such as -O2 and below.

First the case of testing this == NULL.  You state: "When optimizing, GCC now assumes the this pointer can never be null, which is guaranteed by the language rules."

This seems to be incorrect to me.  "this" is nothing but a pointer to the class structure and at least in the case of non-virtual methods this can be zero, and it is often useful for it to be zero, and it is even more useful for a class implementer to be able to know if his method is being called with an uninitialized class.  For example, if one has an optional list implemented as a class(as is the case in Bacula), rather than testing everywhere in the code whether or not the list pointer is NULL, it is more efficient to do so in the class.  In our case, we return a zero in the size() method and thus there is no need in perhaps hundreds of places to test for a NULL pointer because it is done in the class.  Another more important use of testing for this == NULL is for a class author to be able to know if the class has been initialized.  If so, the method can continue, and if not the class writer can take appropriate action and avoid a seg fault.  With your optimization, you explicitly remove this feature from the C++ language.

The second case is you do not generate code in a overridden new operator when memset() is used to zero the memory.  You are assuming that memory has already been zeroed, but we are overridding new specifically to use our own memory allocator that guarantees that the memory is non-zero.  While your optimization may be appropriate at higher levels of optimization, it is not appropriate at -O2 which is used by many programs.

I maintain that it is irresponsible to implement the above two unsafe optimizations at levels -O2 and below.  I am fully aware that we could test for the compiler level and add new command line options, but that is a lot of unnecessary work, and in the mean time, g++ is not generating the code that corresponds to what we wrote (i.e. it is generating incorrect code and introducing seg faults in many programs).  Please implement these "optimizations" if you must at higher optimization levels and leave levels -O2 and -O1 with only optimizations that are safe and do not modify the behavior of the program as written.
Comment 1 Andrew Pinski 2016-07-15 08:26:10 UTC
There is an option to disable both of these. Also the null pointer one had always been there. Just it got smarter.
Comment 2 Kern Sibbald 2016-07-15 08:45:32 UTC
Yes, we are aware of the option and how to fix the problem.  The issue is that this optimization at low levels of -O1 and -O2 is not reasonable, and it is unreasonable and irresponsible to make such changes. Just search Internet to see what kinds of pains this creates -- all for nothing.  

Some years ago, we just accepted these kinds of crippling and costly changes. This incident has caused bad versions of Bacula to be distributed by at least two popular distros that seg fault because of g++.  We are not the only project to be affected. For me this comes down to the philosophy of how the gcc project treats its users. Do you force risky changes on your users or do you try to protect them. The gcc project has its view, but this result is not acceptable to me.  

Some years ago there was no alternative to g++, but faced with these kind of problems that take months to fix because of an "unstable" and "incompatible" new compiler releases, I for one will now take a careful look at the alternatives.

I suggest that you (the gcc project) carefully reconsider whether making such assumptions leading to risky optimizations is best practice.
Comment 3 Jonathan Wakely 2016-07-15 14:47:12 UTC
Both examples you give are undefined behaviour according to the C++ standard. You can claim the code is valid, but that doesn't make it true.

You seem to be confusing "it worked OK until now" with "this code is valid according to the language standard".
Comment 4 Andrew Pinski 2016-07-16 06:24:55 UTC
-fno-delete-null-pointer-checks

For the not deleting null pointer checks.  The other bug report has the other option specified already.

C++ and C have undefined behavior in them.  Learning this for the first time sometimes can be a shock but once you learn the optimizing compiler optimizes code better you will understand why.

Also For the null pointer check, you can use -fsantizer=undefined to find the undefined behavior in your code.
Comment 5 Manuel López-Ibáñez 2016-07-16 13:37:01 UTC
Everything that has been said here has been said before:

https://gcc.gnu.org/wiki/FAQ#misoptimization
https://gcc.gnu.org/wiki/FAQ#undefinedbut
Comment 6 Kern Sibbald 2016-07-16 14:06:01 UTC
As you say, everything has been said and in any case, it is clear that you are going to stick with the current compiler behavior. What you have failed to understand is that I do very well understand that certain things are "undefined". My complaint is that once you have decided that it is undefined, the compiler should at print a warning message and then leave the code alone.  This is what any reasonable compiler would do (IMO). Instead the compiler disregards what the programmer has written and elides code. I (personally) classify this this is bad decision making (not conservative) because it made the "undefined behavior" worse by introducing bugs. Clearly this is my opinion and not yours, so let's let it drop there because it is your project and your call.
Comment 7 Manuel López-Ibáñez 2016-07-16 15:40:38 UTC
(In reply to Kern Sibbald from comment #6)
> As you say, everything has been said and in any case, it is clear that you
> are going to stick with the current compiler behavior. What you have failed

There is no 'we'. Individuals work on GCC, they implement changes because they fix something that they consider broken or because they are paid for it. I don't have an opinion on this particular case. I do think GCC could produce better diagnostics. It could also be smarter about its optimizations. It could also provide a -fboring option (https://lwn.net/Articles/669004/). But currently I don't have the time nor the money to do any of those, so there is no point in complaining loudly that people don't work on what I want.

What I meant by posting those links is that almost every point in the discussion so far is already in the FAQ and you can find much better explanations in them than anyone can give you here (otherwise, we should extend the FAQs).

> to understand is that I do very well understand that certain things are
> "undefined". My complaint is that once you have decided that it is

From the FAQ: Undefined behavior is not decided nor defined by GCC, but by the committees that write the language standards. Quoting Steve Summit (maintainer of the C FAQ): "Perhaps we could, but not under the current Standard. [...] But the C Standard is under revision: perhaps, if this is important enough to you, you can convince the committee"

> undefined, the compiler should at print a warning message and then leave the
> code alone.  This is what any reasonable compiler would do (IMO). Instead

From the FAQ: GCC attempts to diagnose some undefined behaviours, but this is not possible in all cases. If GCC missed a warning and you know how to implement it, please send even a rough working patch. Otherwise, just assume there is no easy/efficient way to diagnose that particular case.

(That said, I think GCC should warn in this case, as it seems easy: https://gcc.gnu.org/bugzilla/PR71904. Clang does it. Probably the above paragraph should be rewritten: encourage users to open PRs, but cautioning them that diagnosing UB is hard/impossible in most cases)

> the compiler disregards what the programmer has written and elides code. I
> (personally) classify this this is bad decision making (not conservative)
> because it made the "undefined behavior" worse by introducing bugs. Clearly

From the FAQ: The reasons why the above are not reasonable requests are difficult to understand unless you are willing to learn '''in detail''' about how optimizing compilers work internally and the history of the programming languages. Steve Summit briefly discusses only part of the [[http://www.eskimo.com/~scs/readings/undef.950321.html|rationale behind undefined behaviors.]]

(I just added this now to further explain this point and because people do not seem to bother to read Summit's text ;-)

> this is my opinion and not yours, so let's let it drop there because it is
> your project and your call.

GCC belongs to whoever decides to contribute to it: https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps

If you think that better warnings are needed, you are free to contribute to that. If you have a clear idea how to make this UB behave consistently and do  not impact optimization, then propose a patch. If you think that this case is so important that any optimization gains should be ignored, then become a middle-end maintainer.

You said:
> In both case, the assumptions you make are not valid and thus these
> optimizations should not be made at low levels such as -O2 and below.

If the assumptions were invalid, they would be invalid at any optimization level (and without optimization).
Comment 8 anton 2016-08-27 14:29:25 UTC
My recommendation is to, by default, refuse to compile with newer (i.e., worse) gcc versions than you have tested, and tell the builders that overriding that has a significant chance to result in a broken build.  You might also choose not to use worse gcc versions for your own development, saving you the time to find and work around the new bugs that gcc's miscompilation introduces.
Comment 9 Kern Sibbald 2016-08-27 14:40:36 UTC
(In reply to Manuel López-Ibáñez from comment #7)
Your wipppesnapper comments that are personally insulting are not at all helpful.
Comment 10 Kern Sibbald 2016-08-27 14:46:12 UTC
(In reply to anton from comment #8)
It is not productive or conductive to good relations for me to tell packagers how to do their job.  The fact is that very few of them never test the packages they release not because they are bad packagers but because they do not have the time and perhaps the knowledge.

The kind of changes the gcc project is making these days are what gives open source (free source if you like) a bad reputation.
Comment 11 Kern Sibbald 2016-08-27 14:55:24 UTC
I recently discussed both of these "optimizations" with Bjarne Stroustrup and his comment about deleting the memset() when overriding the new() functions was:

   Looks like a bug to me

His comment about deleting the test for a NULL class pointer was:

  Yes. According to the standard no object can exists at address 0, so the optimization seems valid.


So, I repeat: please remove your "bug" that deletes memset() code and causes program failures.

Despite what he says about removing the NULL pointer, when you do so, you make it impossible with standard C++ to prevent a seg fault is someone calls a class function (by error or on purpose) with a NULL pointer.  Please rmove this useless optimization.

Both of your optimizations do not make sense, they give no practical improvement to the compiler and only serve to introduce failures to programs that otherwise would run correctly.
Comment 12 anton 2016-08-27 15:09:59 UTC
Given that they suffer from a lack of time, and have no way to know which gcc versions you tested with, I guess most packagers would prefer it if the configure script tells them the compiler version they should use rather than them having to find out with trial and error (or worse, them not finding out and releasing a broken package).  Telling them in the README or INSTALL files is another option, but may be missed given the lack of time.

Of course, it would be preferable if the gcc maintainers applied the criteria for production software (Linus Torvalds: "If a change results in user programs breaking, it's a bug in the kernel. We never EVER blame the user programs. How hard can this be to understand?") rather than those for research software, but since they don't, software developers and packagers have to deal with the wreckage as long as we use C and C++.
Comment 13 Manuel López-Ibáñez 2016-08-27 15:12:39 UTC
(In reply to Kern Sibbald from comment #9)
> (In reply to Manuel López-Ibáñez from comment #7)
> Your wipppesnapper comments that are personally insulting are not at all
> helpful.

I'm not sure which of my comments personally insulted you, but that was never my intention. The summary is that: I do understand your point of view and I agree with you that GCC should do better to warn about undefined behavior and I have done a fair amount of volunteer coding to improve that, however, you are confused about crucial points (such as who decides what is undefined, that GCC is creating bugs that were not there, or who is responsible for changes in GCC) and if your aim is to influence GCC development in any way, your way of approaching the discussion is not conducive to that aim. But since I failed to help you, I won't keep trying.
Comment 14 Manuel López-Ibáñez 2016-08-27 15:13:10 UTC
Unfollowing.