[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

--- Comment #15 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The testcase has an ODR violation that makes comdat groups go out of sync. So I
guess it is just about finding way to not make verifier to ICE.
With release settings the testcase will however quietly compile this I do not
think this is release blocker (P1).

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marxin at gcc dot gnu.org

--- Comment #16 from Martin Liška <marxin at gcc dot gnu.org> ---
I see the ODR warning being properly printed:

g++ pr64076_0.o pr64076_1.o
pr64076.H:10:8: warning: type ‘struct S’ violates the C++ One Definition Rule
[-Wodr]
   10 | struct S : public X, public Y, public Z
      |        ^
pr64076.H:10:8: note: a type with the same name but different number of
polymorphic bases is defined in another translation unit
   10 | struct S : public X, public Y, public Z
      |        ^
pr64076.H:8:8: note: the extra base is defined here
    8 | struct T : public Base { };
      |        ^
pr64076.H:15:8: warning: type of ‘f’ does not match original declaration
[-Wlto-type-mismatch]
   15 |   void f()
      |        ^
pr64076_1.C:5:6: note: ‘f’ was previously declared here
    5 | void S::f() { }
      |      ^
pr64076_1.C:5:6: note: code may be misoptimized unless ‘-fno-strict-aliasing’
is used

I agree with Honza that it's not P1.

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

--- Comment #17 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Note that to fully fix the problem we need to resolve the way aliases works.
In this case ODR violation makes one COMDAT section to contain only ctor, while
other contains ctor and its thunk.  The first COMDAT wins which makes the thunk
to call alias of a symbol prevailed by different COMDAT.
This still work w/o LTO and to immitate what happens in linker correctly
we need ability to load both constructors

https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542733.html

For invalid code like this that does not matter much, but the patch has also a
valid testcase.

I can also however patch around and silence the verifier ICE, but it would be
just symptomatic workaround

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #18 from Richard Biener <rguenth at gcc dot gnu.org> ---
So the resolution for PR64076 the testcase was added for was "similar" as in
short-cut an assert with !in_lto_p.(In reply to Jan Hubicka from comment #15)
> The testcase has an ODR violation that makes comdat groups go out of sync.
> So I guess it is just about finding way to not make verifier to ICE.
> With release settings the testcase will however quietly compile this I do
> not think this is release blocker (P1).

Huh, the testcase failure mode is a link-fail - remember the testcase
has one object compiled with -fno-lto.  It might be there's another bug
in us ICEing when you LTO the whole thing and I'd agree that's more an
ice-after-error and we should eventually make ODR violations error and
simply give up after WPA.

So - can you please see why the testcase fails to _link_?  For reference
this is what my testsuite log says:

spawn -ignore SIGHUP /home/rguenther/obj/gcc/testsuite/g++/../../xg++
-B/home/rguenther/obj/gcc/testsuite/g++/../../ cp_lto_pr64076_0.o
cp_lto_pr64076_1.o -fno-diagnostics-show-caret
-fno-diagnostics-show-line-numbers -fdiagnostics-color=never
-fdiagnostics-urls=never -nostdinc++
-I/home/rguenther/obj/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu
-I/home/rguenther/obj/x86_64-pc-linux-gnu/libstdc++-v3/include
-I/home/rguenther/src/trunk/libstdc++-v3/libsupc++
-I/home/rguenther/src/trunk/libstdc++-v3/include/backward
-I/home/rguenther/src/trunk/libstdc++-v3/testsuite/util -fmessage-length=0 -O0
-flto -flto-partition=none -fuse-linker-plugin
-L/home/rguenther/obj/x86_64-pc-linux-gnu/./libstdc++-v3/src/.libs
-B/home/rguenther/obj/x86_64-pc-linux-gnu/./libstdc++-v3/src/.libs
-L/home/rguenther/obj/x86_64-pc-linux-gnu/./libstdc++-v3/src/.libs
-B/home/rguenther/obj/x86_64-pc-linux-gnu/./libitm/
-L/home/rguenther/obj/x86_64-pc-linux-gnu/./libitm/.libs -o
g++-dg-lto-pr64076-01.exe^M
/usr/lib64/gcc/x86_64-suse-linux/4.8/../../../../x86_64-suse-linux/bin/ld:
g++-dg-lto-pr64076-01.exe.lto.o:(.rodata+0x58): undefined reference to
`non-virtual thunk to S::f()'^M
collect2: error: ld returned 1 exit status^M
compiler exited with status 1
FAIL: g++.dg/lto/pr64076 cp_lto_pr64076_0.o-cp_lto_pr64076_1.o link, -O0 -flto
-flto-partition=none -fuse-linker-plugin

I guess the explanation is similar in the end - the wrong comdat wins?

For a usual bugreport such report would be INVALID I guess.  Now the issue
is we have that testcase in our testsuite and the solution to PR64076.
I suppose PR64076 would have ICEd when producing a shared object as well
so that might be something we could do to alleivate the link failure?
The testcase unfortunately doesn't reproduce the old issue anymore
when reverting the fix.

The question is of course also why appearantly the causing rev caused
that symbol to be necessary.

diff --git a/gcc/testsuite/g++.dg/lto/pr64076_0.C
b/gcc/testsuite/g++.dg/lto/pr64076_0.C
index fb9b060e323..57d0fd6a1c3 100644
--- a/gcc/testsuite/g++.dg/lto/pr64076_0.C
+++ b/gcc/testsuite/g++.dg/lto/pr64076_0.C
@@ -1,4 +1,8 @@
 // { dg-lto-do link }
+// { dg-lto-options { { -O0 -flto -shared -fPIC } } }
+// { dg-require-effective-target fpic }
+// { dg-require-effective-target shared }
+// { dg-extra-ld-options "-shared" }

 #define XXX
 #include "pr64076.H"
diff --git a/gcc/testsuite/g++.dg/lto/pr64076_1.C
b/gcc/testsuite/g++.dg/lto/pr64076_1.C
index 4bd00817b1d..c9c58b798ec 100644
--- a/gcc/testsuite/g++.dg/lto/pr64076_1.C
+++ b/gcc/testsuite/g++.dg/lto/pr64076_1.C
@@ -1,4 +1,4 @@
-// { dg-options -fno-lto }
+// { dg-options "-fno-lto -fPIC" }

 #include "pr64076.H"


fixes the testcase for me (and would be OK to me to resolve this P1).
Other opinions?

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

--- Comment #19 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The reason why we get link failure is that we behave differently to mismatched
comdats.  While linker choose comdat that wins and eliminate other one we keep
the other symbol and end up compiling it which leads to interesting issues with
"half comdat" I am aiming to solve with the patch for proper handling of
aliases.

I think updating the testcase with -shared is a way to go for this P1 and I we
can discuss the alias issue (probably for 10.2, since it is bit involved and
very old)

Honza

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

--- Comment #20 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:bb40460646ce4e6ad27a2f6795106d004d405314

commit r10-7652-gbb40460646ce4e6ad27a2f6795106d004d405314
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Apr 9 13:54:01 2020 +0200

    testsuite/93369 - use -shared to avoid issue with ODR violation

    The testcase contains an ODR violation and thus the observed
    link failure is an accepted outcome (it originally was for
    an ICE during WPA).  Thus the following adds -shared to the link.

    2020-04-09  Richard Biener  <rguenther@suse.de>

            PR testsuite/93369
            * g++.dg/lto/pr64076_0.C: Add -shared -fPIC.
            * g++.dg/lto/pr64076_1.C: Add -fPIC.

[Bug ipa/93369] [10 regression] g++.dg/lto/pr64076 fails

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93369

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #21 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.