[Bug c++/62306] New: [4.9/5 Regression?] Change in the comdat used for constructors

[Bug c++/62306] New: [4.9/5 Regression?] Change in the comdat used for constructors

[rafael.espindola at gmail dot com]

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

            Bug ID: 62306
           Summary: [4.9/5 Regression?] Change in the comdat used for
                    constructors
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rafael.espindola at gmail dot com
                CC: jason at redhat dot com

Given

struct Option {
  virtual ~Option() {}
};
template <class DataType> class opt : public Option {};
template class opt<int>;

before r206182 

gcc would print

.section    .text._ZN3optIiED0Ev,"axG",@progbits,_ZN3optIiED0Ev,comdat

now it prints

.section    .text._ZN3optIiED0Ev,"axG",@progbits,_ZN3optIiED5Ev,comdat

It looks reasonable to use D5, but it is not clear if that was intentional and
there are no tests for this change in behaviour.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-08-29
                 CC|jason at redhat dot com            |jakub at gcc dot gnu.org,
                   |                            |jason at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from Jason Merrill <jason at gcc dot gnu.org> ---
So, the change to use D5 for the deleting dtor was deliberate when we started
using D5 for the base/complete dtor aliases in 4.5.  Then 4.7 started using D0
again for some reason, and then 4.9 went back to D5.

Jakub, do you remember why you wanted to put the deleting dtor in D5 in the
first place?  The gcc-patches threads don't seem to mention why.

https://gcc.gnu.org/ml/gcc-patches/2009-11/threads.html#00700
https://gcc.gnu.org/ml/gcc-patches/2009-11/threads.html#01768
https://gcc.gnu.org/ml/gcc-patches/2009-12/threads.html#00023

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[rafael.espindola at gmail dot com]

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

--- Comment #2 from Rafael Avila de Espindola <rafael.espindola at gmail dot com> ---
This is again visible on trunk now that pr62302 has been fixed (thanks!).

It doesn't seem profitable to ever put D0 in the D5 comdat. It cannot be equal
to D1 or D2, so putting it there just constrains the linker unnecessarily.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[rafael.espindola at gmail dot com]

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

--- Comment #4 from Rafael Avila de Espindola <rafael.espindola at gmail dot com> ---
So it looks like the reason was
----------------
<jason> My thinking was that the ABI change should also support implementations
that implement D0 as another entry point into the destructor

<jakub> jason: leaving D0 out of D5 would be easiest, but would perhaps be a
problem for other implementations
-----------------

But I am not sure what the extra entry point would look like. D0 has a call to
delete *after* the call to the other destructor, so there is no tail that could
be used as an alternative entry point.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
You could e.g. implement the dtors as:
_Z...D0...:
  set some hard reg to 0
  tail call some function
_Z...D1...:
  set some hard reg to 1
  tail call some function
_Z...D2...:
  set some hard reg to 2
  tail call some function
and that function would just have a magic extra argument, if the same in
between all levels would not use it at all, otherwise could e.g. have if (extra
== 0) delete ...; at the end, etc.

My memory is fuzzy on it after all the years, but as the comdat names are part
of the ABI, I think the idea was to allow implementations to choose how to
implement it while staying interoperable.

So, if g++ 4.[569]/5 emit it in D5, I think that is how it was meant, and what
should be done.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[rafael.espindola at gmail dot com]

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

--- Comment #6 from Rafael Avila de Espindola <rafael.espindola at gmail dot com> ---
OK, so should we declare r206182 an "unintentional bug fix" and mark this bug
wontfix?

To be clear, the ABI then is

For any class an implementation has the option of using one comdat per
constructor/destructor or using a C5/D5 comdat. I may make that decision based
on any profitability criterion. If using a C5/D5 comdat the rules are

* A C5 comdat must have C1 and C2.
* If a class has a virtual destructor, the D5 comdat must have D0, D1 and D2
* If a class has a non-virtual destructor, the D5 comdat must have only the D1
and D2 destructors. That is true even if the implementation uses D0 instead of
a call to D1 + _ZdlPv to implement "delete *x"

Should this be documented in

https://refspecs.linuxbase.org/cxxabi-1.86.html ?

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[rafael.espindola at gmail dot com]

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

--- Comment #7 from Rafael Avila de Espindola <rafael.espindola at gmail dot com> ---
(In reply to Rafael Avila de Espindola from comment #6)
> OK, so should we declare r206182 an "unintentional bug fix" and mark this
> bug wontfix?
> 

One thing to keep in mind. If r206182 was the bug fix, r176071 was the revision
that introduced the bug. That was when trunk was 4.7.0.

A quick test with the current branches shows that for the example in the bug
description

4.6: Puts D0 in D5
4.7: Puts D0 in D0
4.8: Puts D0 in D0
4.9: Puts D0 in D5
trunk: Puts D0 in D5

in the case of the 4.9 branch, 

Given that most linux distros are still with 4.8, the safest thing to do might
be to say that 4.7 changed the ABI and patch 4.9 and trunk to put D0 in its own
comdat.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[jason at gcc dot gnu.org]

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

--- Comment #8 from Jason Merrill <jason at gcc dot gnu.org> ---
So, yeah.

When we were originally developing the ABI, we wanted to allow implementations
to make all of the symbols entry points to the same function.  But this didn't
end up being reflected in the ABI document, which doesn't currently allow for
sharing a COMDAT group at all:

"Implicitly-defined or inline user-defined constructors and destructors are
emitted where referenced, each in its own COMDAT group identified by the
constructor or destructor name."

I started a discussion about this on the ABI list in 2009 but it didn't
conclude.  It sounds like most other compilers still put the destructors in
different groups.  clang avoids creating two identical clones by just omitting
the complete-object clone (and putting the base clone in the vtable), which
also seems nonconformant.  Apparently HP puts all the constructors in a C3
group but uses separate groups for the destructors.

So we're already incompatible with everyone else on this, though it's pretty
harmless because the worst that can happen is a bit of extra bloat.  So we
don't need to consider compatibility with other vendors in this decision and
can just do what's right for us.

I think I'm sympathetic to Rafael's argument that we should stick with the 4.7
behavior since that's what most deployed GCCs currently do.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jason Merrill from comment #8)
> I think I'm sympathetic to Rafael's argument that we should stick with the
> 4.7 behavior since that's what most deployed GCCs currently do.

4.5+4.6+4.9 is more released compilers than 4.7/4.8 though, and 4.9 is already
widely deployed too, IMHO it is worse to change this again mid-release.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[rafael.espindola at gmail dot com]

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

--- Comment #10 from Rafael Avila de Espindola <rafael.espindola at gmail dot com> ---
(In reply to Jakub Jelinek from comment #9)
> (In reply to Jason Merrill from comment #8)
> > I think I'm sympathetic to Rafael's argument that we should stick with the
> > 4.7 behavior since that's what most deployed GCCs currently do.
> 
> 4.5+4.6+4.9 is more released compilers than 4.7/4.8 though, and 4.9 is
> already widely deployed too, IMHO it is worse to change this again
> mid-release.

Another option to start using a D6 COMDAT that is defined to always contain D1
and D2 and never contain D0.

This would mean a small bloat when mixing some .o files compiled with 5 and
some with previous releases, but would also mean better codegen and a clean
state for 5 and newer.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[jason at gcc dot gnu.org]

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

--- Comment #11 from Jason Merrill <jason at gcc dot gnu.org> ---
I'm not too concerned about the change.(In reply to Jakub Jelinek from comment
#9)
> 4.5+4.6+4.9 is more released compilers than 4.7/4.8 though, and 4.9 is
> already widely deployed too, IMHO it is worse to change this again
> mid-release.

Fair enough.  I'm not too concerned about the mismatch either way; even in
4.7/4.8 any TU that emits D1/2 also emits D0, so we aren't going to have link
failures.

[Bug c++/62306] [4.9/5 Regression?] Change in the comdat used for constructors

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID
           Assignee|unassigned at gcc dot gnu.org      |jason at gcc dot gnu.org

--- Comment #12 from Jason Merrill <jason at gcc dot gnu.org> ---
So, intentional.  And the test added for 62302 covers this as well.