C++ PATCH for c++/86063, ICE with attribute with pack expansion

Message ID 20180606211835.GC28085@redhat.com
State New
Headers show
Series
  • C++ PATCH for c++/86063, ICE with attribute with pack expansion
Related show

Commit Message

Marek Polacek June 6, 2018, 9:18 p.m.
We crash on this testcase containing a bogus attribute, because
cp_check_const_attributes accessed TREE_VALUE of a tree that happened to be
expr_pack_expansion.  Since here we're merely trying to evaluate constexpr
arguments, I thought we could skip such bogus arguments.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-06-06  Marek Polacek  <polacek@redhat.com>

	PR c++/86063
	* decl2.c (cp_check_const_attributes): Skip trees that are not
	TREE_LISTs.

	* g++.dg/cpp0x/gen-attrs-65.C: New test.

Comments

Jason Merrill June 11, 2018, 7 p.m. | #1
On Wed, Jun 6, 2018 at 5:18 PM, Marek Polacek <polacek@redhat.com> wrote:
> We crash on this testcase containing a bogus attribute, because

> cp_check_const_attributes accessed TREE_VALUE of a tree that happened to be

> expr_pack_expansion.  Since here we're merely trying to evaluate constexpr

> arguments, I thought we could skip such bogus arguments.


Hmm, attributes should always be a TREE_LIST, lots of places assume
that.  Why isn't the pack expansion wrapped in a TREE_LIST?

Jason
Marek Polacek June 14, 2018, 4:51 p.m. | #2
On Mon, Jun 11, 2018 at 03:00:04PM -0400, Jason Merrill wrote:
> On Wed, Jun 6, 2018 at 5:18 PM, Marek Polacek <polacek@redhat.com> wrote:

> > We crash on this testcase containing a bogus attribute, because

> > cp_check_const_attributes accessed TREE_VALUE of a tree that happened to be

> > expr_pack_expansion.  Since here we're merely trying to evaluate constexpr

> > arguments, I thought we could skip such bogus arguments.

> 

> Hmm, attributes should always be a TREE_LIST, lots of places assume

> that.  Why isn't the pack expansion wrapped in a TREE_LIST?


I believe you did that on purpose.  There pack comes from
cp_parser_std_attribute_list.  We could wrap it into a TREE_LIST, but then
tsubst_attribute would have to be tweaked to handle the pack expansion
correctly.  Since this is invalid code, it didn't seem worth it.  Normally
we remove the attribute in save_template_attributes:

  if (processing_template_decl)
    {    
      if (check_for_bare_parameter_packs (attributes))
        return;

      save_template_attributes (&attributes, decl, flags);
    }    

  cp_check_const_attributes (attributes);

so attributes is null after calling cp_check_const_attributes.  But this test
is invalid so save_template_attributes doesn't do anything and then
cp_check_const_attributes crashes on the expr_pack_expansion.

	Marek
Jason Merrill June 14, 2018, 4:59 p.m. | #3
On Thu, Jun 14, 2018 at 12:51 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Mon, Jun 11, 2018 at 03:00:04PM -0400, Jason Merrill wrote:

>> On Wed, Jun 6, 2018 at 5:18 PM, Marek Polacek <polacek@redhat.com> wrote:

>> > We crash on this testcase containing a bogus attribute, because

>> > cp_check_const_attributes accessed TREE_VALUE of a tree that happened to be

>> > expr_pack_expansion.  Since here we're merely trying to evaluate constexpr

>> > arguments, I thought we could skip such bogus arguments.

>>

>> Hmm, attributes should always be a TREE_LIST, lots of places assume

>> that.  Why isn't the pack expansion wrapped in a TREE_LIST?

>

> I believe you did that on purpose.  There pack comes from

> cp_parser_std_attribute_list. We could wrap it into a TREE_LIST, but then

> tsubst_attribute would have to be tweaked to handle the pack expansion

> correctly.


How so?  tsubst_attribute expects to find a pack expansion in the
TREE_VALUE of a TREE_LIST.
And cp_parser_std_attribute_list puts the pack expansion in TREE_VALUE.

Jason
Marek Polacek June 14, 2018, 7:40 p.m. | #4
On Thu, Jun 14, 2018 at 12:59:00PM -0400, Jason Merrill wrote:
> On Thu, Jun 14, 2018 at 12:51 PM, Marek Polacek <polacek@redhat.com> wrote:

> > On Mon, Jun 11, 2018 at 03:00:04PM -0400, Jason Merrill wrote:

> >> On Wed, Jun 6, 2018 at 5:18 PM, Marek Polacek <polacek@redhat.com> wrote:

> >> > We crash on this testcase containing a bogus attribute, because

> >> > cp_check_const_attributes accessed TREE_VALUE of a tree that happened to be

> >> > expr_pack_expansion.  Since here we're merely trying to evaluate constexpr

> >> > arguments, I thought we could skip such bogus arguments.

> >>

> >> Hmm, attributes should always be a TREE_LIST, lots of places assume

> >> that.  Why isn't the pack expansion wrapped in a TREE_LIST?

> >

> > I believe you did that on purpose.  There pack comes from

> > cp_parser_std_attribute_list. We could wrap it into a TREE_LIST, but then

> > tsubst_attribute would have to be tweaked to handle the pack expansion

> > correctly.

> 

> How so?  tsubst_attribute expects to find a pack expansion in the

> TREE_VALUE of a TREE_LIST.

> And cp_parser_std_attribute_list puts the pack expansion in TREE_VALUE.


Exactly.  But what tsubst_attribute gets currently is

 <tree_list 0x7ffff001c280 tree_0
    purpose <tree_list 0x7ffff0002f00
        purpose <identifier_node 0x7fffefec7d80 gnu
            normal local bindings <(nil)>>
        value <identifier_node 0x7ffff0014f00 aligned
            normal local bindings <(nil)>>>
    value <expr_pack_expansion 0x7fffefeada20
        arg:0 <tree_list 0x7ffff0002f50
            value <alignof_expr 0x7ffff0015600 type <integer_type 0x7fffefecd7e0 long unsigned int>
                readonly tree_0 arg:0 <template_type_parm 0x7ffff001a1f8 T>
                alignas4.C:17:19 start: alignas4.C:17:19 finish: alignas4.C:17:29>>
        arg:1 <tree_list 0x7ffff0002f78 value <template_type_parm 0x7ffff001a1f8 T>>>>

so if I were to wrap the expr_pack_expansion in a TREE_LIST, I would have to adjust
tsubst_attribute.  But cp_check_const_attributes doesn't expect that the TREE_VALUE
of the above is a non-list.  Right?

	Marek
Jason Merrill June 14, 2018, 8:46 p.m. | #5
On Thu, Jun 14, 2018 at 3:40 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Jun 14, 2018 at 12:59:00PM -0400, Jason Merrill wrote:

>> On Thu, Jun 14, 2018 at 12:51 PM, Marek Polacek <polacek@redhat.com> wrote:

>> > On Mon, Jun 11, 2018 at 03:00:04PM -0400, Jason Merrill wrote:

>> >> On Wed, Jun 6, 2018 at 5:18 PM, Marek Polacek <polacek@redhat.com> wrote:

>> >> > We crash on this testcase containing a bogus attribute, because

>> >> > cp_check_const_attributes accessed TREE_VALUE of a tree that happened to be

>> >> > expr_pack_expansion.  Since here we're merely trying to evaluate constexpr

>> >> > arguments, I thought we could skip such bogus arguments.

>> >>

>> >> Hmm, attributes should always be a TREE_LIST, lots of places assume

>> >> that.  Why isn't the pack expansion wrapped in a TREE_LIST?

>> >

>> > I believe you did that on purpose.  There pack comes from

>> > cp_parser_std_attribute_list. We could wrap it into a TREE_LIST, but then

>> > tsubst_attribute would have to be tweaked to handle the pack expansion

>> > correctly.

>>

>> How so?  tsubst_attribute expects to find a pack expansion in the

>> TREE_VALUE of a TREE_LIST.

>> And cp_parser_std_attribute_list puts the pack expansion in TREE_VALUE.

>

> Exactly.  But what tsubst_attribute gets currently is

>

>  <tree_list 0x7ffff001c280 tree_0

>     purpose <tree_list 0x7ffff0002f00

>         purpose <identifier_node 0x7fffefec7d80 gnu

>             normal local bindings <(nil)>>

>         value <identifier_node 0x7ffff0014f00 aligned

>             normal local bindings <(nil)>>>

>     value <expr_pack_expansion 0x7fffefeada20

>         arg:0 <tree_list 0x7ffff0002f50

>             value <alignof_expr 0x7ffff0015600 type <integer_type 0x7fffefecd7e0 long unsigned int>

>                 readonly tree_0 arg:0 <template_type_parm 0x7ffff001a1f8 T>

>                 alignas4.C:17:19 start: alignas4.C:17:19 finish: alignas4.C:17:29>>

>         arg:1 <tree_list 0x7ffff0002f78 value <template_type_parm 0x7ffff001a1f8 T>>>>

>

> so if I were to wrap the expr_pack_expansion in a TREE_LIST, I would have to adjust

> tsubst_attribute.  But cp_check_const_attributes doesn't expect that the TREE_VALUE

> of the above is a non-list.  Right?


Ah, of course, you're already looking at the arguments, I wasn't
reading closely enough.  The patch is OK.

Jason

Patch

diff --git gcc/cp/decl2.c gcc/cp/decl2.c
index 2cef9c750ed..35d8e423397 100644
--- gcc/cp/decl2.c
+++ gcc/cp/decl2.c
@@ -1386,7 +1386,8 @@  cp_check_const_attributes (tree attributes)
   for (attr = attributes; attr; attr = TREE_CHAIN (attr))
     {
       tree arg;
-      for (arg = TREE_VALUE (attr); arg; arg = TREE_CHAIN (arg))
+      for (arg = TREE_VALUE (attr); arg && TREE_CODE (arg) == TREE_LIST;
+	   arg = TREE_CHAIN (arg))
 	{
 	  tree expr = TREE_VALUE (arg);
 	  if (EXPR_P (expr))
diff --git gcc/testsuite/g++.dg/cpp0x/gen-attrs-65.C gcc/testsuite/g++.dg/cpp0x/gen-attrs-65.C
index e69de29bb2d..1d2b2f04e7a 100644
--- gcc/testsuite/g++.dg/cpp0x/gen-attrs-65.C
+++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-65.C
@@ -0,0 +1,7 @@ 
+// PR c++/86063
+// { dg-do compile { target c++11 } }
+
+template <class... T>
+struct S {
+  [[foobar(alignof(T))...]] char t; // { dg-warning "attribute directive ignored" }
+};