diff mbox

libstdc++ PATCH to add abi tag to complex::real/imag

Message ID 509F0D27.8080408@redhat.com
State New
Headers show

Commit Message

Jason Merrill Nov. 11, 2012, 2:27 a.m. UTC
As mentioned in http://gcc.gnu.org/wiki/Cxx11AbiCompatibility, C++11 
changes the return type of complex::real and imag, leading to a binary 
incompatibility between C++98 and C++11 code if the functions are used 
without inlining.  This patch adds an ABI tag to the C++11 variants so 
that they have different mangled names.  This does not change the 
exports from libstdc++.

For the map void->iterator change, I think it would make sense to just 
unconditionally return an iterator; there's no binary compatibility 
issue with older code that expects it to return void, the problem is 
only with C++11 code calling a C++98 instantiation that returns void.

For the other cases mentioned on that page, I think either we want to 
unify the two implementations (because we think they're compatible) or 
add ABI tags to the C++11 implementations.

Is this patch OK for trunk?  Does someone on the library team want to 
look at the other cases?

Comments

Marc Glisse Nov. 11, 2012, 8:08 a.m. UTC | #1
On Sat, 10 Nov 2012, Jason Merrill wrote:

> As mentioned in http://gcc.gnu.org/wiki/Cxx11AbiCompatibility, C++11 changes 
> the return type of complex::real and imag, leading to a binary 
> incompatibility between C++98 and C++11 code if the functions are used 
> without inlining.  This patch adds an ABI tag to the C++11 variants so that 
> they have different mangled names.  This does not change the exports from 
> libstdc++.

It looks like it won't prevent from using ref-qualifiers when they are 
implemented if we want to (PR 51013), which is good.

> For the map void->iterator change, I think it would make sense to just 
> unconditionally return an iterator;

It does break C++98 code that does "return f();" in a function returning 
void, but that's negligible.

> there's no binary compatibility issue 
> with older code that expects it to return void, the problem is only with 
> C++11 code calling a C++98 instantiation that returns void.

That's going to happen in practice, isn't it? So do you mean to add the 
abi tag to this unified function, for safety?
Jonathan Wakely Nov. 11, 2012, 11:55 a.m. UTC | #2
On 11 November 2012 02:27, Jason Merrill wrote:
>
> Is this patch OK for trunk?  Does someone on the library team want to look
> at the other cases?

The patch looks good to me. Thanks for implementing the attribute.

I'll start looking at some of the other cases.

For std::list I assume we want the attribute on the type itself, not
just member functions, since its size changes.

For vector::data() we should be able to make the attribute conditional
on is_same<_Tp*,allocator_type::pointer> so it doesn't change mangling
unless necessary.

As well as the existing incompatibilities, I need to add a new virtual
function to support returning future_status::deferred from
std::future::wait_{for,until}.  Is it appropriate to use the same
"cxx11" tag for that too, even though the existing implementation is
also "cxx11" because the type wasn't in C++98?   Or do we want to use
a different tag, such as "cxx11v2" or just "v2"?
Paolo Carlini Nov. 11, 2012, 12:17 p.m. UTC | #3
Hi,

On 11/11/2012 12:55 PM, Jonathan Wakely wrote:
> On 11 November 2012 02:27, Jason Merrill wrote:
>> Is this patch OK for trunk?  Does someone on the library team want to look
>> at the other cases?
> The patch looks good to me. Thanks for implementing the attribute.
>
> I'll start looking at some of the other cases.
>
> For std::list I assume we want the attribute on the type itself, not
> just member functions, since its size changes.
If it's that easy, I can do this bit, ie, simply restore my old patch, 
which was already complete and tested.

Of course thanks a lot from me too!

Paolo.
Jason Merrill Nov. 12, 2012, 4:54 a.m. UTC | #4
On 11/11/2012 06:55 AM, Jonathan Wakely wrote:
> For std::list I assume we want the attribute on the type itself, not
> just member functions, since its size changes.

Yes.  Note that I believe for the list and string changes we want to 
change the default ABI for C++98 and C++11, not just C++11.  And provide 
a way for users to select the old ABI if they need to pass string/list 
objects to and from previously compiled code.

> As well as the existing incompatibilities, I need to add a new virtual
> function to support returning future_status::deferred from
> std::future::wait_{for,until}.  Is it appropriate to use the same
> "cxx11" tag for that too, even though the existing implementation is
> also "cxx11" because the type wasn't in C++98?   Or do we want to use
> a different tag, such as "cxx11v2" or just "v2"?

If this is a C++11-only class, then I don't think we need a tag at all. 
  I don't think we've been trying to maintain binary compatibility 
between major releases for C++11 code.  The goal of all of this is to 
allow old C++98 code to coexist in the same running program with new 
C++11 code so long as they aren't trying to use the same string/list 
objects.

Jason
Marc Glisse Nov. 12, 2012, 6:50 a.m. UTC | #5
On Sun, 11 Nov 2012, Jason Merrill wrote:

> On 11/11/2012 06:55 AM, Jonathan Wakely wrote:
>> For std::list I assume we want the attribute on the type itself, not
>> just member functions, since its size changes.

When we are changing the whole type, I assume the point of using this
attribute instead of the standard solutions (move it to some other
(inline) namespace, for instance) is the -Wabi-tag warning?

> Yes.  Note that I believe for the list and string changes we want to change 
> the default ABI for C++98 and C++11, not just C++11.  And provide a way for 
> users to select the old ABI if they need to pass string/list objects to and 
> from previously compiled code.

That way is already needed while building libstdc++, and we need to 
compile all files that use string twice during the build, exporting only 
the symbols that contain "cxx11" from one of the builds.

Changing string/list is still an ABI break. Is it for 4.8, or should it 
wait until 4.9?
Jason Merrill Nov. 12, 2012, 2:02 p.m. UTC | #6
On 11/12/2012 01:50 AM, Marc Glisse wrote:
> When we are changing the whole type, I assume the point of using this
> attribute instead of the standard solutions (move it to some other
> (inline) namespace, for instance) is the -Wabi-tag warning?

Right.

Jason
diff mbox

Patch

commit be79353c10252bc99cac5f9d9ce045207e665238
Author: Jason Merrill <jason@redhat.com>
Date:   Sat Nov 10 13:10:15 2012 -0500

    	* include/std/complex (real, imag): Add ABI tag in C++11 mode.

diff --git a/libstdc++-v3/include/std/complex b/libstdc++-v3/include/std/complex
index f9221a8..24fa414 100644
--- a/libstdc++-v3/include/std/complex
+++ b/libstdc++-v3/include/std/complex
@@ -141,9 +141,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if __cplusplus >= 201103L
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // DR 387. std::complex over-encapsulated.
+      __attribute ((abi_tag ("cxx11")))
       constexpr _Tp 
       real() { return _M_real; }
 
+      __attribute ((abi_tag ("cxx11")))
       constexpr _Tp 
       imag() { return _M_imag; }
 #else
@@ -1061,9 +1063,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if __cplusplus >= 201103L
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // DR 387. std::complex over-encapsulated.
+      __attribute ((abi_tag ("cxx11")))
       constexpr float 
       real() { return __real__ _M_value; }
 
+      __attribute ((abi_tag ("cxx11")))
       constexpr float 
       imag() { return __imag__ _M_value; }
 #else
@@ -1210,9 +1214,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if __cplusplus >= 201103L
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // DR 387. std::complex over-encapsulated.
+      __attribute ((abi_tag ("cxx11")))
       constexpr double 
       real() { return __real__ _M_value; }
 
+      __attribute ((abi_tag ("cxx11")))
       constexpr double 
       imag() { return __imag__ _M_value; }
 #else
@@ -1360,9 +1366,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if __cplusplus >= 201103L
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // DR 387. std::complex over-encapsulated.
+      __attribute ((abi_tag ("cxx11")))
       constexpr long double 
       real() { return __real__ _M_value; }
 
+      __attribute ((abi_tag ("cxx11")))
       constexpr long double 
       imag() { return __imag__ _M_value; }
 #else
diff --git a/libstdc++-v3/testsuite/26_numerics/complex/abi_tag.cc b/libstdc++-v3/testsuite/26_numerics/complex/abi_tag.cc
new file mode 100644
index 0000000..a845466
--- /dev/null
+++ b/libstdc++-v3/testsuite/26_numerics/complex/abi_tag.cc
@@ -0,0 +1,25 @@ 
+// Test that the C++11 variants of real/imag have an ABI tag
+// { dg-do compile }
+// { dg-options -std=c++11 }
+
+#include <complex>
+
+// { dg-final { scan-assembler "_ZNKSt7complexIfE4realB5cxx11Ev" } }
+float (std::complex<float>::*p1)() const = &std::complex<float>::real;
+// { dg-final { scan-assembler "_ZNKSt7complexIdE4realB5cxx11Ev" } }
+double (std::complex<double>::*p2)() const = &std::complex<double>::real;
+// { dg-final { scan-assembler "_ZNKSt7complexIeE4realB5cxx11Ev" } }
+long double (std::complex<long double>::*p3)() const
+  = &std::complex<long double>::real;
+// { dg-final { scan-assembler "_ZNKSt7complexIiE4realB5cxx11Ev" } }
+int (std::complex<int>::*p4)() const = &std::complex<int>::real;
+
+// { dg-final { scan-assembler "_ZNKSt7complexIfE4imagB5cxx11Ev" } }
+float (std::complex<float>::*p5)() const = &std::complex<float>::imag;
+// { dg-final { scan-assembler "_ZNKSt7complexIdE4imagB5cxx11Ev" } }
+double (std::complex<double>::*p6)() const = &std::complex<double>::imag;
+// { dg-final { scan-assembler "_ZNKSt7complexIeE4imagB5cxx11Ev" } }
+long double (std::complex<long double>::*p7)() const
+  = &std::complex<long double>::imag;
+// { dg-final { scan-assembler "_ZNKSt7complexIiE4imagB5cxx11Ev" } }
+int (std::complex<int>::*p8)() const = &std::complex<int>::imag;