Bug 67056 - [5 regression] Wrong code generated
Summary: [5 regression] Wrong code generated
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: ipa (show other bugs)
Version: 5.2.0
: P2 normal
Target Milestone: 5.3
Assignee: Jan Hubicka
URL:
Keywords: wrong-code
: 66738 (view as bug list)
Depends on: 68057
Blocks:
  Show dependency treegraph
 
Reported: 2015-07-29 14:24 UTC by Henning Baldersheim
Modified: 2023-08-20 20:50 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 5.2.0
Last reconfirmed: 2015-11-02 00:00:00


Attachments
Minimal failing example (233 bytes, text/x-csrc)
2015-08-04 13:06 UTC, Vegard Sjonfjell
Details
Preprocessed file (83.71 KB, text/plain)
2015-08-04 13:09 UTC, Vegard Sjonfjell
Details
Somewhat reduced testcase (729 bytes, text/plain)
2015-08-04 14:43 UTC, Markus Trippelsdorf
Details
Fix I am teching. (1.04 KB, patch)
2015-10-16 04:46 UTC, Jan Hubicka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Henning Baldersheim 2015-07-29 14:24:04 UTC
Yes, I know a very bad summary. I need some help to narrow it down.

When upgrading compile from 4.9.3 to 5.2 1 of our unit tests started failing with segmentation fault. It compiles fine, but generates bad code. There are no warning neither with 4.9.3 nor with 5.2. We are also running the unit tests with latest valgrind. With 5.2 valgrind complains about accessing deleted memory.

I am not sure how I best can provide you with what you need in order to investigate this as it fails at runtime and the unit test uses other shared libraries. Any suggestions ?

I have made the following observations.

1 - We are building with -O3. The issue is also present at -O2. However if I add -fno-tree-vrp it improves. Then there is no segmentation fault, but valgrind complains about a memory leak. It fails to run a destructor.

2 - I can fix the issue by reordering members in one class from
    ConfigSnapshot snap;
    std::atomic<bool> configured;
    std::atomic<bool> throwException;
to
    std::atomic<bool> configured;
    std::atomic<bool> throwException;
    ConfigSnapshot snap;
Then valgrind is happy to. No need for tinkering with the compile flags.

3 - I move the implementation of the destructor and the constructor out of the class definition and add __attribute__((noinline)) to them.
Then everything works perfectly too.

What can I provide that can help you investigate this ?

This is the compile command for the unit test. The code that I mention in 2 and 3 are both contained in the file compiled here.
g++ -g -O3 -Wuninitialized -Werror -Wall -W -Wchar-subscripts -Wcomment -Wformat -Wparentheses -Wreturn-type -Wswitch -Wtrigraphs -Wunused -Wshadow -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -DGCC_X86_64 -fPIC -D_GLIBCXX_USE_CXX11_ABI=0 -DBOOST_DISABLE_ASSERTS -march=westmere -mtune=intel -Wnon-virtual-dtor -std=c++14 -fvisibility-inlines-hidden             -DNOT_BOOST_SPIRIT_THREADSAFE=1 -DNOT_PHOENIX_THREADSAFE=1 -DBOOST_NO_AUTO_PTR=1        -I../.. -I"/home/balder/build/lz4-PREINST/lz4-install/include"  -I/home/balder/build/staging_vespalib-HEAD/staging_vespalib-install/include -I"/home/balder/build/ytracelib-PREINST/ytracelib-install/include"  -I/home/balder/build/fastlib-HEAD/fastlib-install/include -I/home/balder/build/fnet-HEAD/fnet-install/include -I/home/balder/build/vespalib-HEAD/vespalib-install/include -isystem /home/balder/build/llvm-PREINST/llvm-install/include -isystem /home/balder/build/boost-PREINST/boost-install/include/ -I/home/balder/build/vespalog-HEAD/vespalog-install/include -I/home/balder/build/fastos-HEAD/fastos-install/include   -DV_TAG_DATE='"20150729.142010"'  -DV_TAG_YINST='"5.81.0.20150729.142010"'  -DV_TAG_COMPONENT='"5.81.0"'  -DV_TAG_ARCH='"x86_64"'  -DV_TAG_SYSTEM='"Linux"'  -DV_TAG_SYSTEM_REV='"rhel-6.6"'  -DV_TAG_BUILDER='"balder@honda.trondheim.corp"'  -DV_TAG='"CURRENT"'  -c configretriever.cpp -o configretriever.o

g++ -v
[balder@honda configretriever]$ g++ -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/home/y/bin64/../libexec/gcc/x86_64-redhat-linux/5.2.0/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ./configure --prefix=/home/y --enable-languages=c,c++ --host=x86_64-redhat-linux --build=x86_64-redhat-linux --target=x86_64-redhat-linux --disable-bootstrap LDFLAGS='-Wl,-rpath,/home/y/lib64 -L/home/y/lib64' --with-mpc=/home/y --with-gmp=/home/y --with-mpfr=/home/y
Thread model: posix
gcc version 5.2.0 (GCC)
Comment 1 ktkachov 2015-07-29 14:26:13 UTC
There are some tips and instructions at:
https://gcc.gnu.org/bugs/

A reduced, preprocessed testcase would be ideal.
Comment 2 Markus Trippelsdorf 2015-07-29 14:53:54 UTC
Also try to build your project with -fsanitize=undefined and see
if any issues pop up while running the unit tests.
Comment 3 Henning Baldersheim 2015-07-29 14:59:46 UTC
valgrind does not complain about anything undefined, but will try the sanitize option too.

Will also try to isolate it as much as possible.
Comment 4 Henning Baldersheim 2015-07-29 15:39:38 UTC
Using the -fsanitize-undefined caused this error.
configretriever.cpp: info:  running test suite 'configretriever.cpp'
/home/y/include/c++/5.2.0/bits/unique_ptr.h:76:2: runtime error: execution reached a __builtin_unreachable() call
make: *** [test] Error 1

While it works fine when not inlining the constructor.
Comment 5 Vegard Sjonfjell 2015-08-04 13:06:18 UTC
Created attachment 36122 [details]
Minimal failing example

We've tried to strip the code down to a minimal failing example. Compile with -O3 -fPIC -std=c++11 (or std=c++14).
Comment 6 Vegard Sjonfjell 2015-08-04 13:09:08 UTC
Created attachment 36123 [details]
Preprocessed file

Also adding the preprocessed file.
Comment 7 Markus Trippelsdorf 2015-08-04 14:43:21 UTC
Created attachment 36126 [details]
Somewhat reduced testcase
Comment 8 Markus Trippelsdorf 2015-08-04 14:45:34 UTC
markus@x4 tmp % g++  -fsanitize=undefined -std=c++14 -O2 -fPIC mem.ii
markus@x4 tmp % ./a.out
mem.ii:72:5: runtime error: execution reached a __builtin_unreachable() call
markus@x4 tmp % g++ -fno-devirtualize -fsanitize=undefined  -std=c++14 -O2 -fPIC mem.ii
markus@x4 tmp % ./a.out
markus@x4 tmp %
Comment 9 Markus Trippelsdorf 2015-08-04 15:47:33 UTC
Started with r215902.
Comment 10 Henning Baldersheim 2015-08-05 07:30:46 UTC
Wrong code generation on valid code does sound like a P2 to me.
Comment 11 Markus Trippelsdorf 2015-08-05 08:07:12 UTC
(In reply to Henning Baldersheim from comment #10)
> Wrong code generation on valid code does sound like a P2 to me.

The release manager sets the importance.

Another question is if the transition from "requests valid" to
"wrong code" is a regression at all.
Comment 12 Markus Trippelsdorf 2015-08-05 10:02:22 UTC
(In reply to Markus Trippelsdorf from comment #11)
> Another question is if the transition from "requests valid" to
> "wrong code" is a regression at all.

Apart from the typo (s/requests/rejects) please ignore the comment
above, it was meant for a different PR.
Comment 13 Jan Hubicka 2015-10-12 08:13:30 UTC
Will take a look.
Comment 14 Jan Hubicka 2015-10-16 04:45:39 UTC
OK, the unreachable is introduced here:
 - Creating a specialized node of bool staticBoolFunc(CompositeClass*)/414 for all known contexts.
     the new node is <built-in>/977.                                            
     known ctx 0 is     Outer type (dynamic):struct EmptyClass offset -64       
No devirtualization target in <built-in>/977                                    
ipa-prop: Discovered a virtual call to a known target (<built-in>/977 -> void __builtin_unreachable()/976), for stmt OBJ_TYPE_REF(_15;ptr_2(D)->1) (ptr_2(D));
/aux/hubicka/trunk-install/include/c++/6.0.0/bits/unique_ptr.h:76:2: note: converting indirect call in <built-in> to direct call to void __builtin_unreachable()
No devirtualization target in <built-in>/977                                    
ipa-prop: Discovered a virtual call to a known target (<built-in>/977 -> void __builtin_unreachable()/976), for stmt OBJ_TYPE_REF(_27;ptr_2(D)->1) (ptr_2(D));
/aux/hubicka/trunk-install/include/c++/6.0.0/bits/unique_ptr.h:76:2: note: converting indirect call in <built-in> to direct call to void __builtin_unreachable()

So ipa-CP thinks that staticBoolFunc is called on EmptyClass instead of CompositeClass:

Jump functions:                                                                 
  Jump functions of caller  long unsigned int __builtin_object_size(const void*, int)/967:
  Jump functions of caller  void operator delete(void*, long unsigned int)/964: 
  Jump functions of caller  void* operator new(std::size_t)/963:                
  Jump functions of caller  int main(int, char**)/415:                          
    callsite  int main(int, char**)/415 -> void operator delete(void*, long unsigned int)/964 :
    callsite  int main(int, char**)/415 -> bool staticBoolFunc(CompositeClass*)/414 :
       param 0: UNKNOWN                                                         
         Context:     Outer type (dynamic):struct EmptyClass offset -64         
         Unknown alignment                                                      
    callsite  int main(int, char**)/415 -> EmptyClass::EmptyClass()/404 :       
       param 0: UNKNOWN                                                         
         Context:     Outer type (dynamic): (or a derived type) (maybe in construction) offset 64 Speculative outer type:struct CompositeClass (or a derived type) at offset 64
         Unknown alignment                                                      

This is indeed wrong. Jump function analysis seems to confuse constructors:

Modification phase of node int main(int, char**)/402
int main(int, char**) (int D.39529, char * * D.39530)
{
  void * _3;
  struct EmptyClass * _7;

  <bb 2>:
  _3 = operator new (16);
  MEM[(struct  &)_3] ={v} {CLOBBER};
  MEM[(struct CompositeClass *)_3]._vptr.CompositeClass = &MEM[(void *)&_ZTV14CompositeClass + 16B];
  _7 = &MEM[(struct CompositeClass *)_3].object;
  EmptyClass::EmptyClass (_7);

  <bb 3>:
  staticBoolFunc (_3);
  return 0;

<L1>:
  operator delete (_3, 16);
  resx 1

EmptyClass ctor is called, but it should not type the object.

Determining dynamic type for call: staticBoolFunc (_3);
  Starting walk at: staticBoolFunc (_3);
  instance pointer: _3  Outer instance pointer: _3 offset: 0 (bits) vtbl reference: 
  Checking constructor call: EmptyClass::EmptyClass (_7);
  Recording type: struct EmptyClass at offset -64
  Determined dynamic type.

This is quite a nonsense, because EmptyClass is not even. So there are two bugs.
First is that we determine useless outer type. This should be just missed
optimization. But we also manage to consider to miss the case in placement_new checking where we are completely off the structure....
Comment 15 Jan Hubicka 2015-10-16 04:46:45 UTC
Created attachment 36520 [details]
Fix I am teching.
Comment 16 Jan Hubicka 2015-10-16 06:23:29 UTC
*** Bug 66738 has been marked as a duplicate of this bug. ***
Comment 17 Jan Hubicka 2015-10-21 21:14:38 UTC
Author: hubicka
Date: Wed Oct 21 21:14:06 2015
New Revision: 229148

URL: https://gcc.gnu.org/viewcvs?rev=229148&root=gcc&view=rev
Log:
	PR ipa/67056
	* ipa-polymorphic-call.c (possible_placement_new): If cur_offset
	is negative we don't know the type.
	(check_stmt_for_type_change): Skip constructors of non-polymorphic
	types as those won't help devirutalization.
	* g++.dg/ipa/pr67056.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/ipa/pr67056.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-polymorphic-call.c
    trunk/gcc/testsuite/ChangeLog
Comment 18 Jan Hubicka 2015-10-22 03:19:21 UTC
Fixed on trunk so far; fix is backportable.
Comment 19 Richard Biener 2015-11-02 10:29:20 UTC
*** Bug 68175 has been marked as a duplicate of this bug. ***
Comment 20 Richard Biener 2015-11-18 15:24:20 UTC
Author: rguenth
Date: Wed Nov 18 15:23:48 2015
New Revision: 230550

URL: https://gcc.gnu.org/viewcvs?rev=230550&root=gcc&view=rev
Log:
2015-11-18  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2015-11-07  Jan Hubicka  <hubicka@ucw.cz>

	PR ipa/68057
	PR ipa/68220
	* ipa-polymorphic-call.c
	(ipa_polymorphic_call_context::restrict_to_inner_type): Fix ordering
	issue when offset is out of range.
	(contains_type_p): Fix out of range check, clear dynamic flag.

	* g++.dg/torture/pr68220.C: New testcase.
	* g++.dg/lto/pr68057_0.C: Likewise.
	* g++.dg/lto/pr68057_1.C: Likewise.

	2015-10-23  Jan Hubicka  <hubicka@ucw.cz>
 
	PR ipa/pr67600
	* ipa-polymorphic-call.c
	(ipa_polymorphic_call_context::get_dynamic_type): Do not confuse
	instance offset with offset of outer type.
 
	* g++.dg/torture/pr67600.C: New testcase.

	2015-10-12  Richard Biener  <rguenther@suse.de>

	PR ipa/67783
	* ipa-inline-analysis.c (estimate_function_body_sizes): Re-add
	code that analyzes IVs on each stmt but in a cheaper way avoiding
	quadratic behavior.

	2015-10-11  Jan Hubicka  <hubicka@ucw.cz>
 
	PR ipa/67056
	* ipa-polymorphic-call.c (possible_placement_new): If cur_offset
	is negative we don't know the type.
	(check_stmt_for_type_change): Skip constructors of non-polymorphic
	types as those won't help devirutalization.

	* g++.dg/ipa/pr67056.C: New testcase.

	2015-08-11  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c/66098
	PR c/66711
	* diagnostic.c (diagnostic_classify_diagnostic): Take -Werror into
	account when deciding what was the command-line status.

	* gcc.dg/pragma-diag-3.c: New test.
	* gcc.dg/pragma-diag-4.c: New test.

Added:
    branches/gcc-5-branch/gcc/testsuite/g++.dg/ipa/pr67056.C
    branches/gcc-5-branch/gcc/testsuite/g++.dg/lto/pr68057_0.C
    branches/gcc-5-branch/gcc/testsuite/g++.dg/lto/pr68057_1.C
    branches/gcc-5-branch/gcc/testsuite/g++.dg/torture/pr67600.C
    branches/gcc-5-branch/gcc/testsuite/g++.dg/torture/pr68220.C
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/pragma-diag-3.c
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/pragma-diag-4.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/diagnostic.c
    branches/gcc-5-branch/gcc/ipa-inline-analysis.c
    branches/gcc-5-branch/gcc/ipa-polymorphic-call.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 21 Richard Biener 2015-11-18 15:24:48 UTC
Fixed.