| Message ID | 20180606211835.GC28085@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/86063, ICE with attribute with pack expansion | expand |
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
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
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
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
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
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" } +};