diff mbox

[PR70955] Tag {ms,sysv}_va_list_type_node with {ms,sysv}_abi attribute

Message ID 774c26d6-f93a-c816-be78-4371c8af4708@mentor.com
State New
Headers show

Commit Message

Tom de Vries Aug. 25, 2016, 1:36 p.m. UTC 
On 25/08/16 13:48, Richard Biener wrote:
> On Wed, 24 Aug 2016, Tom de Vries wrote:
>
>> Hi,
>>
>> in PR70955, ix86_canonical_va_list_type fails to recognize a
>> __builtin_ms_va_list that was declared in a TU, because its type doesn't have
>> the same main variant as the ms_va_list_type_node initialized in lto1.
>>
>> This patch fixes the PR by tagging ms_va_list_type_node and
>> sysv_va_list_type_node with ms_abi/sysv_abi attributes.
>>
>> sysv_va_list_type_node is of type array of length one with elemtype record,
>> and I ran into trouble with both adding the attribute to the array type and
>> the record type, so I ended up adding it to the first field type.
>>
>> Bootstrapped and reg-tested on x86_64.
>>
>> OK for trunk, 6 branch?

> How did you build the sysv_abi tagged struct / array that ended up
> not working?

My first try to tag the struct was this:
...
  }
...

But we immediately run into:
...
<built-in>: warning: ignoring attributes applied to ‘__va_list_tag’ 
after definition [-Wattributes]
<built-in>: warning: ignoring attributes applied to ‘struct ’ after 
definition [-Wattributes]
<built-in>: warning: ignoring attributes applied to ‘struct ’ after 
definition [-Wattributes]
...


I tried to work around that by directly assigning to TYPE_ATTRIBUTES, as 
implemented in attached patch. But then I run into a libstdc++ build 
ICE, in mangle.c:write_type:
...
       tree t = TYPE_MAIN_VARIANT (type);
       if (TYPE_ATTRIBUTES (t) && !OVERLOAD_TYPE_P (t))
         {
           tree attrs = NULL_TREE;
           if (tx_safe_fn_type_p (type))
             attrs = tree_cons (get_identifier ("transaction_safe"),
                                NULL_TREE, attrs);
           t = cp_build_type_attribute_variant (t, attrs);
         }
       gcc_assert (t != type);
...

 > I suspect that in parameter passing the array somehow
 > decays to a pointer-to-element type

Yep, as mentioned in more detail at 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955#c23 . That's why I 
didn't try very hard to get tagging the array with the attribute to work.

 > so it is important that the
 > TYPE_MAIN_VARIANT of the record type already contains the attribute.

AFAIU, the record type is its own TYPE_MAIN_VARIANT, so it contains the 
attributes.

Thanks,
- Tom

Comments

Richard Biener Aug. 26, 2016, 8:40 a.m. UTC | #1 
On Thu, 25 Aug 2016, Tom de Vries wrote:

> On 25/08/16 13:48, Richard Biener wrote:
> > On Wed, 24 Aug 2016, Tom de Vries wrote:
> > 
> > > Hi,
> > > 
> > > in PR70955, ix86_canonical_va_list_type fails to recognize a
> > > __builtin_ms_va_list that was declared in a TU, because its type doesn't
> > > have
> > > the same main variant as the ms_va_list_type_node initialized in lto1.
> > > 
> > > This patch fixes the PR by tagging ms_va_list_type_node and
> > > sysv_va_list_type_node with ms_abi/sysv_abi attributes.
> > > 
> > > sysv_va_list_type_node is of type array of length one with elemtype
> > > record,
> > > and I ran into trouble with both adding the attribute to the array type
> > > and
> > > the record type, so I ended up adding it to the first field type.
> > > 
> > > Bootstrapped and reg-tested on x86_64.
> > > 
> > > OK for trunk, 6 branch?
> 
> > How did you build the sysv_abi tagged struct / array that ended up
> > not working?
> 
> My first try to tag the struct was this:
> ...
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 2639c8c..f07d9f2 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -10548,6 +10548,9 @@ ix86_build_builtin_va_list_64 (void)
> 
>    layout_type (record);
> 
> +  tree attr = tree_cons (get_identifier ("sysv_abi"), NULL_TREE, NULL_TREE);
> +  record = build_type_attribute_variant (record, attr);
> +
>    /* The correct type is an array type of one element.  */
>    return build_array_type (record, build_index_type (size_zero_node));
>  }
> ...
> 
> But we immediately run into:
> ...
> <built-in>: warning: ignoring attributes applied to ‘__va_list_tag’ after
> definition [-Wattributes]
> <built-in>: warning: ignoring attributes applied to ‘struct ’ after definition
> [-Wattributes]
> <built-in>: warning: ignoring attributes applied to ‘struct ’ after definition
> [-Wattributes]
> ...

Yeah, and this also builds a variant type.

> 
> I tried to work around that by directly assigning to TYPE_ATTRIBUTES, as
> implemented in attached patch.

I expected that this would work ...

> But then I run into a libstdc++ build ICE, in
> mangle.c:write_type:
> ...
>       tree t = TYPE_MAIN_VARIANT (type);
>       if (TYPE_ATTRIBUTES (t) && !OVERLOAD_TYPE_P (t))
>         {
>           tree attrs = NULL_TREE;
>           if (tx_safe_fn_type_p (type))
>             attrs = tree_cons (get_identifier ("transaction_safe"),
>                                NULL_TREE, attrs);
>           t = cp_build_type_attribute_variant (t, attrs);
>         }
>       gcc_assert (t != type);
> ...

Hmm, you shouldn't run into write_CV_qualifiers_for_type as the
type shouldn't be CV qualified (and any CV qualified record type
should be indeed a variant type of the unqualified record).

Ah, so write_CV_qualifiers_for_type looks at ABI changing attributes
and it seems yours counts as such (because it is a documented
attribute even only documented as function attribute).  I suppose
you could get away with using sth entirely private to the backend,
like "sysv abi valist", also not ever user-creatable.  Does this
side-step the FE issue?

Richard.
diff mbox

Patch

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 2639c8c..54a0ef9 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10548,6 +10548,8 @@  ix86_build_builtin_va_list_64 (void)
 
   layout_type (record);
 
+  TYPE_ATTRIBUTES (record) = tree_cons (get_identifier ("sysv_abi"), NULL_TREE, NULL_TREE);
+
   /* The correct type is an array type of one element.  */
   return build_array_type (record, build_index_type (size_zero_node));
 }
@@ -10561,16 +10563,16 @@  ix86_build_builtin_va_list (void)
   if (TARGET_64BIT)
     {
       /* Initialize ABI specific va_list builtin types.  */
-      tree sysv_va_list, ms_va_list;
-
-      sysv_va_list = ix86_build_builtin_va_list_64 ();
-      sysv_va_list_type_node = build_variant_type_copy (sysv_va_list);
+      sysv_va_list_type_node = ix86_build_builtin_va_list_64 ();
 	
       /* For MS_ABI we use plain pointer to argument area.  */
-      ms_va_list = build_pointer_type (char_type_node);
-      ms_va_list_type_node = build_variant_type_copy (ms_va_list);
+      tree char_ptr_type = build_pointer_type (char_type_node);
+      tree attr = tree_cons (get_identifier ("ms_abi"), NULL_TREE, NULL_TREE);
+      ms_va_list_type_node = build_type_attribute_variant (char_ptr_type, attr);
 
-      return (ix86_abi == MS_ABI) ? ms_va_list : sysv_va_list;
+      return ((ix86_abi == MS_ABI)
+	      ? ms_va_list_type_node
+	      : sysv_va_list_type_node);
     }
   else
     {
@@ -48563,8 +48565,6 @@  ix86_fn_abi_va_list (tree fndecl)
 static tree
 ix86_canonical_va_list_type (tree type)
 {
-  tree wtype, htype;
-
   /* Resolve references and pointers to va_list type.  */
   if (TREE_CODE (type) == MEM_REF)
     type = TREE_TYPE (type);
@@ -48573,64 +48573,24 @@  ix86_canonical_va_list_type (tree type)
   else if (POINTER_TYPE_P (type) && TREE_CODE (TREE_TYPE (type)) == ARRAY_TYPE)
     type = TREE_TYPE (type);
 
-  if (TARGET_64BIT && va_list_type_node != NULL_TREE)
+  if (TARGET_64BIT)
     {
-      wtype = va_list_type_node;
-	  gcc_assert (wtype != NULL_TREE);
-      htype = type;
-      if (TREE_CODE (wtype) == ARRAY_TYPE)
-	{
-	  /* If va_list is an array type, the argument may have decayed
-	     to a pointer type, e.g. by being passed to another function.
-	     In that case, unwrap both types so that we can compare the
-	     underlying records.  */
-	  if (TREE_CODE (htype) == ARRAY_TYPE
-	      || POINTER_TYPE_P (htype))
-	    {
-	      wtype = TREE_TYPE (wtype);
-	      htype = TREE_TYPE (htype);
-	    }
-	}
-      if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
-	return va_list_type_node;
-      wtype = sysv_va_list_type_node;
-	  gcc_assert (wtype != NULL_TREE);
-      htype = type;
-      if (TREE_CODE (wtype) == ARRAY_TYPE)
-	{
-	  /* If va_list is an array type, the argument may have decayed
-	     to a pointer type, e.g. by being passed to another function.
-	     In that case, unwrap both types so that we can compare the
-	     underlying records.  */
-	  if (TREE_CODE (htype) == ARRAY_TYPE
-	      || POINTER_TYPE_P (htype))
-	    {
-	      wtype = TREE_TYPE (wtype);
-	      htype = TREE_TYPE (htype);
-	    }
-	}
-      if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
-	return sysv_va_list_type_node;
-      wtype = ms_va_list_type_node;
-	  gcc_assert (wtype != NULL_TREE);
-      htype = type;
-      if (TREE_CODE (wtype) == ARRAY_TYPE)
+      if (lookup_attribute ("ms_abi", TYPE_ATTRIBUTES (type)))
+	return ms_va_list_type_node;
+
+      if ((TREE_CODE (type) == ARRAY_TYPE
+	   && integer_zerop (array_type_nelts (type)))
+	  || POINTER_TYPE_P (type))
 	{
-	  /* If va_list is an array type, the argument may have decayed
-	     to a pointer type, e.g. by being passed to another function.
-	     In that case, unwrap both types so that we can compare the
-	     underlying records.  */
-	  if (TREE_CODE (htype) == ARRAY_TYPE
-	      || POINTER_TYPE_P (htype))
-	    {
-	      wtype = TREE_TYPE (wtype);
-	      htype = TREE_TYPE (htype);
-	    }
+	  tree elem_type = TREE_TYPE (type);
+	  if (TREE_CODE (elem_type) == RECORD_TYPE
+	      && lookup_attribute ("sysv_abi", TYPE_ATTRIBUTES (elem_type)))
+	    return sysv_va_list_type_node;
 	}
-      if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
-	return ms_va_list_type_node;
+
       return NULL_TREE;
     }
+
   return std_canonical_va_list_type (type);
 }