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

login
register
mail settings
Submitter Jason Merrill
Date Nov. 11, 2012, 2:27 a.m.
Message ID <509F0D27.8080408@redhat.com>
Download mbox | patch
Permalink /patch/198231/
State New
Headers show

Comments

Jason Merrill - Nov. 11, 2012, 2:27 a.m.
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?
Marc Glisse - Nov. 11, 2012, 8:08 a.m.
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.
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.
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.
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.
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.
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

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;