Bug 71885 - Incorrect code generated with -01, memset() function call is missing
Summary: Incorrect code generated with -01, memset() function call is missing
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.1.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-14 16:54 UTC by Eric Bollengier
Modified: 2016-08-16 11:24 UTC (History)
7 users (show)

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


Attachments
.ii file generated with -O1 option (9.85 KB, text/plain)
2016-07-14 16:54 UTC, Eric Bollengier
Details
ii file generated with -O0 (8.96 KB, text/plain)
2016-07-14 16:56 UTC, Eric Bollengier
Details
c++ file that reproduces the issue (654 bytes, text/x-csrc)
2016-07-14 16:56 UTC, Eric Bollengier
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Bollengier 2016-07-14 16:54:50 UTC
Created attachment 38905 [details]
.ii file generated with -O1 option

I overload the new operator in a class (to initialize the memory to 0 and track memory leaks), and after the upgrade from GCC 5.x to 6.1, my program stopped to work correctly.

If I tweak a little bit the new operator (I add a printf() inside, or I do all operations in one line memset(malloc()) instead of malloc() ; memset();) the code is generated correctly.

I have a small test file, not sure this is the best example.

In my little program, I have two set of classes, one that is properly generated, and an other one that is buggy. The only difference between the two is a "printf()" in a function. The good result should be a == 0

# g++ -Wall -00 new.c
# ./a.out
new is called from new.c:81 and it works!
test_ok a == 0
test    a == 0


# g++ -Wall -01 new.c
# ./a.out
test_ok a == 0
test    a == 1431655765

I'm running Archlinux.

Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/6.1.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc-multilib/src/gcc/configure --prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ --enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++ --enable-shared --enable-threads=posix --enable-libmpx --with-system-zlib --with-isl --enable-__cxa_atexit --disable-libunwind-exceptions --enable-clocale=gnu --disable-libstdcxx-pch --disable-libssp --enable-gnu-unique-object --enable-linker-build-id --enable-lto --enable-plugin --enable-install-libiberty --with-linker-hash-style=gnu --enable-gnu-indirect-function --enable-multilib --disable-werror --enable-checking=release
Thread model: posix
gcc version 6.1.1 20160707 (GCC) 

Thanks
Comment 1 Eric Bollengier 2016-07-14 16:56:19 UTC
Created attachment 38906 [details]
ii file generated with -O0
Comment 2 Eric Bollengier 2016-07-14 16:56:44 UTC
Created attachment 38907 [details]
c++ file that reproduces the issue
Comment 3 Markus Trippelsdorf 2016-07-14 18:44:13 UTC
See https://gcc.gnu.org/gcc-6/porting_to.html (More aggressive optimization of -flifetime-dse).

You're invoking undefined behavior; -flifetime-dse=1 is a workaround.
Comment 4 Eric Bollengier 2016-07-15 05:39:30 UTC
I don't know exactly when someone decided that a doing memset(buf, 0, sizeof(buf)); leads to an "undefined behavior", but it's how C and C++ work since quite long time. And it's also why the operator new() is so useful.

I see the interest of the optimization, however I think that it's not correctly implemented.

If GCC can find out that the code is

void *operator new(size_t x) {
...
  a = malloc(x);
  memset(a, 0, x);
  return a;
}

Then, it is probably OK to strip out the memset() call. However, here, the optimization looks to be only about the memset followed by the return call in the new operator.

void *operator new(size_t x) {

   memset(a, 0, x);
   return a;
}

GCC doesn't check that the memory is coming from a malloc() call or if something was modified in the memory before striping out the memset automatically. I think that it is an incorrect behavior. The memory can come from various places (like a pool of existing buffers), it can be dirty due to some work done before the memset.

It looks that the optimizer thinks that the object returned by the function is a "dead object", but it is not the case, we return it to the caller.

The written code explicitly asks to set a value to a memory area (to avoid undefined behavior), and the compiler decided to not do what is written, and it leads to a situation where the memory doesn't contain what the program expects. 

Thanks,

Best Regards,
Eric
Comment 5 Kern Sibbald 2016-07-15 07:11:11 UTC
I would like to see a precise explanation of what "undefined behavior" is.  Bacula does zero memory that is passed as a replacement to a new.  This behavior is desirable and very well defined. I may be wrong, but it seems that you are assuming that the memory is already zero, which is not a good assumption. In the case of Bacula memory that is allocated from the heap is non-zero by explicit programming.  Consequently this seems to me to be an overly aggressive optimization that should be enabled at a much higher optimization level than -O2, generally used by Bacula and many other programs.

At some point g++'s tinkering with valid user's programs should stop.  Yes, we can probably work around the g++ generated bug by explicitly testing for the g++ compiler version and adding another command line option, but this should be unnecessary.

Changing g++ in this manner (rather cavalier in my opinion) can create lots of unnecessary problems for many programs. As you might image, I call this new g++ feature "unreasonable" and "dangerous".  Please remove it or at least make it a new option that the g++ user must explicitly add.
Comment 6 Markus Trippelsdorf 2016-07-15 08:06:28 UTC
For the record, I was against enabling this optimization by default.
(Because the potential gain doesn't justify the possible confusion it may cause.
And -fsanitize=undefined doesn't catch these issues yet.)

But in the end it is just another instance were the compiler correctly assumes
that undefined behavior will never happen and so optimizes accordingly.

The issue is that the object has an indeterminate value when the constructor starts, so stores to it before this point are lost.
Comment 7 Kern Sibbald 2016-07-15 08:29:18 UTC
Just to be clear:

- This change might be appropriate for higher level of optimization, but is not appropriate as a default for -O2 and -O1.

- There is no undefined behavior here. We override the new operator explicitly to be able to allocate and manage the memory the way we want.  The g++ compiler can make assumptions about what a new operator returns, but it should make any assumptions about what we are doing to initialize memory inside an overridden new operator particularly at low optimization levels.  The g++ behavior is incorrect in this case.  Please correct it.
Comment 8 Markus Trippelsdorf 2016-07-15 08:37:00 UTC
See Initializers 8.6.12:  

When storage for an object with automatic or dynamic storage duration is obtained, the object has an indeterminate value, and if no initialization is performed for the object, that object retains an indeterminate value until that value is replaced (5.17). [ Note: Objects with static or thread storage duration are zero-initialized, see 3.6.2. — end note ] If an indeterminate value is produced by an evaluation, the behavior is undefined...
Comment 9 Eric Bollengier 2016-07-15 08:45:32 UTC
Thanks for the "behavior is undefined" explanation. I understand a bit better why the GCC team did this choice.

However, here, we don't talk about such kind of objects. In my case for example, objects that uses this way of initializing the memory are always allocated with a new(). The programmer controls that side.

I would rather prefer see some warnings about uninitialized variables when the object allocation is automatic rather than stripping my initialization because it doesn't work in all cases.

I would also vote to move this optimization to some -O3 -O4 level instead of -O1.
Comment 10 Kern Sibbald 2016-07-15 08:53:15 UTC
Thanks for your definition of "undetermined" behavior.  The problem here is that at the time the compiler applied its optimization we were just in the process of creating the object as is permitted by the C++ language, and in that process the g++ compiler made what I consider to be a very risky assumption about the state of the object while it was being created. I maintain that our code is valid and should not be optimized away by the compiler. This, in my opinion, is a case of the compiler writer not realizing that his assumption was not valid in our particular case (object being created in an overridden new).  The problem would be corrected in general by ensuring that no "unsafe" optimizations are made at low levels of optimization.  At higher levels the developer knows he risks problems.
Comment 11 Markus Trippelsdorf 2016-07-15 09:02:44 UTC
Well, I'm really the wrong person to justify this optimization.
CCing the author...
Comment 12 Kern Sibbald 2016-07-15 09:27:14 UTC
Yes, I clearly understand your point. My responses were meant for the project were not directed at you. Hopefully someone will consider taking your advice of not making this the default.  It may be difficult to backpeddle, but it is better than breaking tens and possibly hundreds of projects.
Comment 13 Jakub Jelinek 2016-07-15 09:59:26 UTC
An optimizing compiler optimizes on the assumption that undefined behavior does not happen.  So, if you want to keep using code that invokes undefined behavior (which is a very bad idea), you need to be prepared to pass some extra options that tell the compiler not to do it.
-flitefime-dse=1 or -fno-lifetime-dse in this case (the latter if you also "assume" that object after its destruction will still contain the data you've stored in there), -fno-aggressive-loop-optimizations, -fno-strict-aliasing, -O0 in certain cases, in some cases there just isn't an option.
The -Wuninitialized* family of warnings is able to warn in certain cases about this problem, but you of course don't get a guarantee this will be warned about every time.
Comment 14 hyc 2016-08-14 11:06:45 UTC
(In reply to Markus Trippelsdorf from comment #8)
> See Initializers 8.6.12:  
> 
> When storage for an object with automatic or dynamic storage duration is
> obtained, the object has an indeterminate value, and if no initialization is
> performed for the object, that object retains an indeterminate value until
> that value is replaced (5.17).

"If no initialization is performed" - this is the constructor, whose job is to create and initialize the object, and you're preventing that initialization from happening. How is that logical?
Comment 15 hyc 2016-08-14 11:11:29 UTC
(In reply to Markus Trippelsdorf from comment #3)
> See https://gcc.gnu.org/gcc-6/porting_to.html (More aggressive optimization
> of -flifetime-dse).
> 
> You're invoking undefined behavior; -flifetime-dse=1 is a workaround.

"More aggressive optimization of -flifetime-dse

The C++ compiler (with enabled -flifetime-dse) is more aggressive in dead-store elimination in situations where a memory store to a location precedes a constructor to the memory location."

This bug report is talking about a store that occurs *during* the constructor. This does not *precede* the constructor.
Comment 16 Manuel López-Ibáñez 2016-08-14 15:40:51 UTC
I think the relevant part of the standard may be:

C++98 [class.cdtor]:

  For an object of non-POD class type ... before the constructor
  begins execution ... referring to any non-static member or base
  class of the object results in undefined behavior

https://gcc.gnu.org/ml/gcc/2016-02/msg00207.html

But neither the manual nor the porting_to guide are very clear regarding this. Would it be hard to diagnose such uses? (since any use is forbidden!).
Comment 17 Kern Sibbald 2016-08-14 16:52:45 UTC
It is pretty difficult to argue with the developers because they know the "rules", better than most programmers.  However, here in my opinion they used very poor judgement, by implementing a change that they were fully aware would break many programs (they documented it as such).  

It is one thing to "force" C++ programmers to do what the developers think is correct, but it is very bad thing to modify a compiler knowing that it will break a lot of code.  There are probably thousands of users out there who are now experiencing seg faults due to changes such as this.  

Many programmers such as myself rely on C++ but we prefer not to run on bleeding edge systems, and we do not have the means to test our software on all new systems and new compilers.  In this case, various bleeding edge distros switch to gcc 6.0 and compile and release programs that seg fault out of the box. These distros do not have the time or personnel to test every single program. The result is that bad code is released.  This is and was unnecessary.  Yes, we the programmers has options and can fix it.  What was unnecessary was to release a new compiler that the developers knew would produce code that fails.

The g++ developers could have realized that in especially in "undefined" territory where they knew they would break code the conservative way to do it without creating chaos is to add new strict warning message for a period of time (one to two years) prior to making their changes default.
Comment 18 Manuel López-Ibáñez 2016-08-15 13:51:19 UTC
(In reply to Kern Sibbald from comment #17)
> The g++ developers could have realized that in especially in "undefined"
> territory where they knew they would break code the conservative way to do
> it without creating chaos is to add new strict warning message for a period
> of time (one to two years) prior to making their changes default.

If someone developed the warning, it could stay forever. The problem is that someone needs to develop the warning. People working on GCC right now (including myself) are either not paid to do this or not personally interested in such a warning enough to dedicate their free time.

People interested in diagnostics (or a boringcc that never takes advantage of undefined behavior[*]) rather than in optimization should either join GCC or encourage others to join GCC development to the point that they can influence its development in the future.

[*] https://lwn.net/Articles/669004/
Comment 19 hyc 2016-08-15 14:21:47 UTC
(In reply to Manuel López-Ibáñez from comment #18)
> (In reply to Kern Sibbald from comment #17)
> > The g++ developers could have realized that in especially in "undefined"
> > territory where they knew they would break code the conservative way to do
> > it without creating chaos is to add new strict warning message for a period
> > of time (one to two years) prior to making their changes default.
> 
> If someone developed the warning, it could stay forever. The problem is that
> someone needs to develop the warning. People working on GCC right now
> (including myself) are either not paid to do this or not personally
> interested in such a warning enough to dedicate their free time.

That's all well and good. But, somebody had to go out of their way to develop the code to identify this case of new as being a dead store. Why was this worth anyone's time to do so? What performance benefit does this "optimization" bring, and is it really worth all of the obviously known breakage that it causes?

We all have important things to be doing. It doesn't appear that the time invested in this "feature" was time well spent.
Comment 20 Jonathan Wakely 2016-08-15 14:30:18 UTC
(In reply to Kern Sibbald from comment #17)
> It is pretty difficult to argue with the developers because they know the
> "rules", better than most programmers.  However, here in my opinion they
> used very poor judgement, by implementing a change that they were fully
> aware would break many programs (they documented it as such).  

You've got the timeline wrong. The documentation was written *after* the feature was implemented. The new version of GCC had been used to build and test lots of code, which revealed that there were a number of programs breaking the rules of the C++ language, so the documentation was added.

> The g++ developers could have realized that in especially in "undefined"
> territory where they knew they would break code the conservative way to do
> it without creating chaos is to add new strict warning message for a period
> of time (one to two years) prior to making their changes default.

That's been tried before. Nobody fixes the code until the default changes, so in practice your suggestion achieves nothing except postponing the problems by one or two years.
Comment 21 Manuel López-Ibáñez 2016-08-15 14:34:39 UTC
(In reply to hyc from comment #19)
> That's all well and good. But, somebody had to go out of their way to
> develop the code to identify this case of new as being a dead store. Why was
> this worth anyone's time to do so? What performance benefit does this
> "optimization" bring, and is it really worth all of the obviously known
> breakage that it causes?

I don't know the answers to those questions, but somebody did do the effort to implement it and test it: https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00782.html

and then add various options to control it: https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01651.html

so they probably have their own motivation to do this instead of something else. Nobody raised any objections at the time.

> We all have important things to be doing. It doesn't appear that the time
> invested in this "feature" was time well spent.

For better or worse, we don't get to decide on what other people spent their own time unless we pay them.
Comment 22 Jonathan Wakely 2016-08-15 14:38:21 UTC
(In reply to hyc from comment #14)
> "If no initialization is performed" - this is the constructor, whose job is
> to create and initialize the object, and you're preventing that
> initialization from happening. How is that logical?

The constructor doesn't do any initialization. That's the problem.

(In reply to hyc from comment #15)
> This bug report is talking about a store that occurs *during* the
> constructor. This does not *precede* the constructor.

That's entirely incorrect. The initialization in the buggy code is done in operator new, which runs *before* the constructor.

If the initialization was done in the constructor the code would not have undefined behaviour and would work correctly.

Please try to understand the issue before ranting about time well spent.
Comment 23 Kern Sibbald 2016-08-15 14:52:32 UTC
In response to the last post of Jonathan Wakely.

Thanks, that is the first time someone on the gcc side has said something that makes sense.  Of course, possibly I missed or misunderstood previous arguments.

That said, I still think the project made a bad decision on this one, and spent time implementing something that has broken a lot of code and caused users (not just developers) a lot of grief.
Comment 24 hyc 2016-08-15 15:06:37 UTC
(In reply to Manuel López-Ibáñez from comment #21)
> (In reply to hyc from comment #19)
> > That's all well and good. But, somebody had to go out of their way to
> > develop the code to identify this case of new as being a dead store. Why was
> > this worth anyone's time to do so? What performance benefit does this
> > "optimization" bring, and is it really worth all of the obviously known
> > breakage that it causes?
> 
> I don't know the answers to those questions, but somebody did do the effort
> to implement it and test it:
> https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00782.html

I see a patch, I don't see justification / motivation / profiling / benchmark results quantifying the impact of this patch.

> and then add various options to control it:
> https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01651.html
> 
> so they probably have their own motivation to do this instead of something
> else. Nobody raised any objections at the time.
> 
> > We all have important things to be doing. It doesn't appear that the time
> > invested in this "feature" was time well spent.
> 
> For better or worse, we don't get to decide on what other people spent their
> own time unless we pay them.

No, but you get to decide whether to accept the work or not based on whether it actually solved a problem that programmers care about. Or whether the added complexity was worth any added maintenance cost or compile time.

This feature is enabled at -O1, which is usually reserved for non-aggressive, benign optimizations. In what way is this feature an optimization? How does it qualify as benign? Where was *any* critical review of this thing?

When I was a gcc maintainer (1988-1996) a patch like this wouldn't have proceeded without answers to these questions.
Comment 25 Manuel López-Ibáñez 2016-08-15 15:09:49 UTC
(In reply to Kern Sibbald from comment #23)
> That said, I still think the project made a bad decision on this one, and

The "project" doesn't make decisions. Individuals submit patches and other individuals (or the same one) who have a long history of useful contributions approve them if nobody speaks out.

Markus, you and I (and perhaps others) may agree that the potential gains in optimization do not seem to justify the grief caused, but we (including you) are not bothered enough to do the work required to change the status-quo (for example, adding a warning to detect this case and only enabling this optimization for -Ofast would be a good way to convince maintainers to change the default).

Arguing from an assumption of bad faith ("g++'s tinkering with valid user's programs should stop", "g++ generated bug") is not going to convince anyone to do the work that you want to see done for you for free, only entrench them in their own opinion and stop listening to you.
Comment 26 Markus Trippelsdorf 2016-08-16 11:24:26 UTC
For future reference here is a nice, short example from Bernd Edlinger:

markus@x4 tmp % cat lifetime-dse.cpp
#include <assert.h>
#include <stdlib.h>
#include <string.h>

struct A {
  A() {}
  void *operator new(size_t s) {
    void *ptr = malloc(s);
    memset(ptr, 0xFF, s);
    return ptr;
  }
  int value;
};

int main() {
  A *a = new A;
  assert(a->value == -1); /* Use of uninitialized value */
}

markus@x4 tmp % g++ -O2 -flifetime-dse=1 lifetime-dse.cpp
markus@x4 tmp % ./a.out

markus@x4 tmp % g++ -O2 -flifetime-dse=2 lifetime-dse.cpp
markus@x4 tmp % ./a.out
a.out: lifetime-dse.cpp:17: int main(): Assertion `a->value == -1' failed.
[1]    21394 abort      ./a.out