Patchwork RFC: PATCH to add abi_tag attribute

login
register
mail settings
Submitter Jason Merrill
Date Nov. 15, 2012, 1:51 a.m.
Message ID <50A44AB6.9060904@redhat.com>
Download mbox | patch
Permalink /patch/199102/
State New
Headers show

Comments

Jason Merrill - Nov. 15, 2012, 1:51 a.m.
On 11/11/2012 11:58 PM, Jason Merrill wrote:
> On 11/11/2012 08:01 AM, Florian Weimer wrote:
>> I'm not sure if this sufficiently far-reaching.  It seems that this
>> doesn't allow me to implement a virtual function which takes a
>> std::string parameter in new-ABI-mode when the base class is also used
>> in old-ABI-mode.
>
> Ah, yes; since virtual functions are called through the vtable the
> signatures of virtual functions are also part of the class ABI, so the
> base class also needs to be tagged.  I guess I should add that to
> -Wabi-tag.

Like so:

Tested x86_64-pc-linux-gnu, applying to trunk.
Florian Weimer - Nov. 23, 2012, 9:58 a.m.
On 11/15/2012 02:51 AM, Jason Merrill wrote:
> On 11/11/2012 11:58 PM, Jason Merrill wrote:
>> On 11/11/2012 08:01 AM, Florian Weimer wrote:
>>> I'm not sure if this sufficiently far-reaching.  It seems that this
>>> doesn't allow me to implement a virtual function which takes a
>>> std::string parameter in new-ABI-mode when the base class is also used
>>> in old-ABI-mode.
>>
>> Ah, yes; since virtual functions are called through the vtable the
>> signatures of virtual functions are also part of the class ABI, so the
>> base class also needs to be tagged.  I guess I should add that to
>> -Wabi-tag.
>
> Like so:

Okay, this might work in the sense that it flags the relevant cases. 
I'm still not convinced that this actually helps programmers that much 
because it pretty much separates the two worlds.  If this is the intend, 
surely there are simpler approaches (such as a command-line flag which 
changes the mangling for everything).
Jason Merrill - Nov. 23, 2012, 2:24 p.m.
On 11/23/2012 04:58 AM, Florian Weimer wrote:
> Okay, this might work in the sense that it flags the relevant cases. I'm
> still not convinced that this actually helps programmers that much
> because it pretty much separates the two worlds.  If this is the intend,
> surely there are simpler approaches (such as a command-line flag which
> changes the mangling for everything).

It only separates affected code; the idea is that code unaffected by ABI 
changes should be able to continue to interoperate normally.

Jason
Florian Weimer - Nov. 28, 2012, 1:12 p.m.
On 11/23/2012 03:24 PM, Jason Merrill wrote:
> On 11/23/2012 04:58 AM, Florian Weimer wrote:
>> Okay, this might work in the sense that it flags the relevant cases. I'm
>> still not convinced that this actually helps programmers that much
>> because it pretty much separates the two worlds.  If this is the intend,
>> surely there are simpler approaches (such as a command-line flag which
>> changes the mangling for everything).
>
> It only separates affected code; the idea is that code unaffected by ABI
> changes should be able to continue to interoperate normally.

What about code like this?

struct __attribute ((abi_tag ("foo"))) A { };

inline int foo()
{
   return sizeof(A);
}

Couldn't this lead to ODR violations?  Wouldn't we have to warn about 
this case as well?

Patch

commit 0c1d53def870aef5f785278eb447fceb2974df09
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Nov 13 12:23:52 2012 -0500

    	* class.c (finish_struct_1): Check virtual functions
    	for missing ABI tags.

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 0665e90..cdc02ae 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -6275,6 +6275,12 @@  finish_struct_1 (tree t)
 	/* Here we know enough to change the type of our virtual
 	   function table, but we will wait until later this function.  */
 	build_primary_vtable (CLASSTYPE_PRIMARY_BINFO (t), t);
+
+      /* If we're warning about ABI tags, check the types of the new
+	 virtual functions.  */
+      if (warn_abi_tag)
+	for (tree v = virtuals; v; v = TREE_CHAIN (v))
+	  check_abi_tags (t, TREE_VALUE (v));
     }
 
   if (TYPE_CONTAINS_VPTR_P (t))
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 25dd685..43b21c6 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -15622,7 +15622,7 @@  A redeclaration of a function or class must not add new ABI tags,
 since doing so would change the mangled name.
 
 The @option{-Wabi-tag} flag enables a warning about a class which does
-not have all the ABI tags used by its subobjects; for users with code
+not have all the ABI tags used by its subobjects and virtual functions; for users with code
 that needs to coexist with an earlier ABI, using this option can help
 to find all affected types that need to be tagged.
 
diff --git a/gcc/testsuite/g++.dg/abi/abi-tag4.C b/gcc/testsuite/g++.dg/abi/abi-tag4.C
new file mode 100644
index 0000000..3f8d7bf
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/abi-tag4.C
@@ -0,0 +1,8 @@ 
+// { dg-options "-Wabi-tag" }
+
+struct __attribute ((abi_tag ("X"))) A { };
+
+struct B			// { dg-warning "abi tag" }
+{
+  virtual void f(A);		// { dg-message "declared here" }
+};