diff mbox

c++/58109 - alignas() fails to compile with constant expression

Message ID 567A07A0.1010008@gmail.com
State New
Headers show

Commit Message

Martin Sebor Dec. 23, 2015, 2:32 a.m. UTC 
The attached patch adds handling of dependent arguments to
attribute aligned and attribute vector_size, fixing c++/58109
and 69022 - attribute vector_size ignored with dependent bytes.

Tested on x86_64.

Martin

Comments

Marc Glisse Dec. 23, 2015, 7:53 a.m. UTC | #1 
On Tue, 22 Dec 2015, Martin Sebor wrote:

> The attached patch adds handling of dependent arguments to
> attribute aligned and attribute vector_size, fixing c++/58109
> and 69022 - attribute vector_size ignored with dependent bytes.

In the longer term, Gaby used to suggest that internally we should 
represent the vector_size attribute as some kind of template 
__vector<scalar,size> (half-way between a template class and a template 
alias). The goal would be to avoid duplicating all the logic from 
templates into attributes.

Of course that's much more work (even assuming that there isn't some big 
road-block, which there might be), and a little bit more code duplication 
as in your patch seems appropriate at this stage.
Martin Sebor Jan. 5, 2016, 4:49 a.m. UTC | #2 
Ping: looking for review/approval of the patch below:
   https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02074.html

Thanks
Martin

On 12/22/2015 07:32 PM, Martin Sebor wrote:
> The attached patch adds handling of dependent arguments to
> attribute aligned and attribute vector_size, fixing c++/58109
> and 69022 - attribute vector_size ignored with dependent bytes.
>
> Tested on x86_64.
>
> Martin
Martin Sebor Jan. 12, 2016, 12:46 a.m. UTC | #3 
Ping:
     https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02074.html

On 01/04/2016 09:49 PM, Martin Sebor wrote:
> Ping: looking for review/approval of the patch below:
>    https://gcc.gnu.org/ml/gcc-patches/2015-12/msg02074.html
>
> Thanks
> Martin
>
> On 12/22/2015 07:32 PM, Martin Sebor wrote:
>> The attached patch adds handling of dependent arguments to
>> attribute aligned and attribute vector_size, fixing c++/58109
>> and 69022 - attribute vector_size ignored with dependent bytes.
>>
>> Tested on x86_64.
>>
>> Martin
>
Jason Merrill Jan. 12, 2016, 5:20 a.m. UTC | #4 
On 12/22/2015 09:32 PM, Martin Sebor wrote:
> +  if (is_attribute_p ("aligned", name)
> +      || is_attribute_p ("vector_size", name))
> +    {
> +      /* Attribute argument may be a dependent indentifier.  */
> +      if (tree t = args ? TREE_VALUE (args) : NULL_TREE)
> +	if (value_dependent_expression_p (t)
> +	    || type_dependent_expression_p (t))
> +	  return true;
> +    }

Instead of this, is_late_template_attribute should be fixed to check 
attribute_takes_identifier_p.

Jason
Martin Sebor Jan. 12, 2016, 6:11 p.m. UTC | #5 
On 01/11/2016 10:20 PM, Jason Merrill wrote:
> On 12/22/2015 09:32 PM, Martin Sebor wrote:
>> +  if (is_attribute_p ("aligned", name)
>> +      || is_attribute_p ("vector_size", name))
>> +    {
>> +      /* Attribute argument may be a dependent indentifier.  */
>> +      if (tree t = args ? TREE_VALUE (args) : NULL_TREE)
>> +    if (value_dependent_expression_p (t)
>> +        || type_dependent_expression_p (t))
>> +      return true;
>> +    }
>
> Instead of this, is_late_template_attribute should be fixed to check
> attribute_takes_identifier_p.

attribute_takes_identifier_p() returns false for the aligned
attribute and for vector_size (it returns true only for
attributes cleanup, format, and mode, and none others).

Are you suggesting to also change attribute_takes_identifier_p
to return true for these attributes?  That would likely mean
changes to the C front end as well.)

Thanks
Martin
Martin Sebor Jan. 15, 2016, 5:39 p.m. UTC | #6 
On 01/12/2016 11:11 AM, Martin Sebor wrote:
> On 01/11/2016 10:20 PM, Jason Merrill wrote:
>> On 12/22/2015 09:32 PM, Martin Sebor wrote:
>>> +  if (is_attribute_p ("aligned", name)
>>> +      || is_attribute_p ("vector_size", name))
>>> +    {
>>> +      /* Attribute argument may be a dependent indentifier.  */
>>> +      if (tree t = args ? TREE_VALUE (args) : NULL_TREE)
>>> +    if (value_dependent_expression_p (t)
>>> +        || type_dependent_expression_p (t))
>>> +      return true;
>>> +    }
>>
>> Instead of this, is_late_template_attribute should be fixed to check
>> attribute_takes_identifier_p.
>
> attribute_takes_identifier_p() returns false for the aligned
> attribute and for vector_size (it returns true only for
> attributes cleanup, format, and mode, and none others).
>
> Are you suggesting to also change attribute_takes_identifier_p
> to return true for these attributes?  That would likely mean
> changes to the C front end as well.)

Jason, can you please clarify what you had in mind?  I realize this
isn't as severe as a codegen problem but I'd like to try to wrap it
up in between higher priority tasks.

>
> Thanks
> Martin
Jason Merrill Jan. 18, 2016, 4:11 p.m. UTC | #7 
On 01/12/2016 01:11 PM, Martin Sebor wrote:
> On 01/11/2016 10:20 PM, Jason Merrill wrote:
>> On 12/22/2015 09:32 PM, Martin Sebor wrote:
>>> +  if (is_attribute_p ("aligned", name)
>>> +      || is_attribute_p ("vector_size", name))
>>> +    {
>>> +      /* Attribute argument may be a dependent indentifier.  */
>>> +      if (tree t = args ? TREE_VALUE (args) : NULL_TREE)
>>> +    if (value_dependent_expression_p (t)
>>> +        || type_dependent_expression_p (t))
>>> +      return true;
>>> +    }
>>
>> Instead of this, is_late_template_attribute should be fixed to check
>> attribute_takes_identifier_p.
>
> attribute_takes_identifier_p() returns false for the aligned
> attribute and for vector_size (it returns true only for
> attributes cleanup, format, and mode, and none others).

Right.  The problem is this code in is_late_template_attribute:

>       /* If the first attribute argument is an identifier, only consider
>          second and following arguments.  Attributes like mode, format,
>          cleanup and several target specific attributes aren't late
>          just because they have an IDENTIFIER_NODE as first argument.  */
>       if (arg == args && identifier_p (t))
>         continue;

It shouldn't skip an initial identifier if !attribute_takes_identifier_p.

Jason
diff mbox

Patch

gcc/testsuite/ChangeLog:
2015-12-22  Martin Sebor  <msebor@redhat.com>

	PR c++/58109
	PR c++/69022
	* g++.dg/cpp0x/alignas5.C: New test.
	* g++.dg/ext/vector29.C: Same.

gcc/cp/ChangeLog:
2015-12-22  Martin Sebor  <msebor@redhat.com>

	PR c++/58109
	PR c++/69022
	* decl2.c (is_late_template_attribute): Handle dependent argument
	to attribute align and attribute vector_size.

Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 231903)
+++ gcc/cp/decl2.c	(working copy)
@@ -1183,6 +1183,16 @@ 
   if (args && PACK_EXPANSION_P (args))
     return true;
 
+  if (is_attribute_p ("aligned", name)
+      || is_attribute_p ("vector_size", name))
+    {
+      /* Attribute argument may be a dependent indentifier.  */
+      if (tree t = args ? TREE_VALUE (args) : NULL_TREE)
+	if (value_dependent_expression_p (t)
+	    || type_dependent_expression_p (t))
+	  return true;
+    }
+  
   /* If any of the arguments are dependent expressions, we can't evaluate
      the attribute until instantiation time.  */
   for (arg = args; arg; arg = TREE_CHAIN (arg))
Index: gcc/testsuite/g++.dg/cpp0x/alignas5.C
===================================================================
--- gcc/testsuite/g++.dg/cpp0x/alignas5.C	(revision 0)
+++ gcc/testsuite/g++.dg/cpp0x/alignas5.C	(working copy)
@@ -0,0 +1,45 @@ 
+// PR c++/58109  - alignas() fails to compile with constant expression
+// { dg-do compile }
+
+template <typename T>
+struct Base {
+  static const int Align = sizeof (T);
+};
+
+// Never instantiated.
+template <typename T>
+struct Derived: Base<T>
+{
+#if __cplusplus >= 201102L
+  // This is the meat of the (simplified) regression test for c++/58109.
+  using B = Base<T>;
+  using B::Align;
+
+  alignas (Align) char a [1];
+  alignas (Align) T b [1];
+#else
+  // Fake the test for C++ 98.
+#  define Align Base<T>::Align
+#endif
+
+  char __attribute__ ((aligned (Align))) c [1];
+  T __attribute__ ((aligned (Align))) d [1];
+};
+
+// Instantiated to verify that the code is accepted even when instantiated.
+template <typename T>
+struct InstDerived: Base<T>
+{
+#if __cplusplus >= 201102L
+  using B = Base<T>;
+  using B::Align;
+
+  alignas (Align) char a [1];
+  alignas (Align) T b [1];
+#endif
+
+  char __attribute__ ((aligned (Align))) c [1];
+  T __attribute__ ((aligned (Align))) d [1];
+};
+
+InstDerived<int> dx;
Index: gcc/testsuite/g++.dg/ext/vector29.C
===================================================================
--- gcc/testsuite/g++.dg/ext/vector29.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/vector29.C	(working copy)
@@ -0,0 +1,53 @@ 
+// PR c++/69022 - attribute vector_size ignored with dependent bytes
+// { dg-do compile }
+
+template <int N>
+struct A { static const int X = N; };
+
+#if __cplusplus >= 201202L
+#  define ASSERT(e) static_assert (e, #e)
+#else
+#  define ASSERT(e)   \
+  do { struct S { bool: !!(e); } asrt; (void)&asrt; } while (0)
+#endif
+
+template <class T, int N>
+struct B: A<N>
+{
+#if __cplusplus >= 201202L
+  using A<N>::X;
+#  define VecSize X
+#else
+#  define VecSize A<N>::X
+#endif
+
+    static void foo ()
+    {
+        char a __attribute__ ((vector_size (N)));
+        ASSERT (sizeof a == N);
+
+        T b __attribute__ ((vector_size (N)));
+        ASSERT (sizeof b == N);
+    }
+
+    static void bar ()
+    {
+        char c1 __attribute__ ((vector_size (VecSize)));
+        ASSERT (sizeof c1 == VecSize);
+
+        char c2 __attribute__ ((vector_size (A<N>::X)));
+        ASSERT (sizeof c2 == A<N>::X);
+
+        T d1 __attribute__ ((vector_size (VecSize)));
+        ASSERT (sizeof d1 == VecSize);
+
+        T d2 __attribute__ ((vector_size (A<N>::X)));
+        ASSERT (sizeof d2 == A<N>::X);
+    }
+};
+
+void bar ()
+{
+    B<int, 16>::foo ();
+    B<int, 16>::bar ();
+}