Message ID | 20161208210759.GL2337@redhat.com |
---|---|
State | New |
Headers | show |
On 08/12/16 22:07, Marek Polacek wrote: > This test ICEs since r240072 where Tom disallowed using va_list * as a first > argument to va_arg. I think the problem is in the ADDR_EXPR check in > gimplify_va_arg_expr. It's meant to handle Case 1, but the valist doesn't > always have to be ADDR_EXPR. E.g. for this testcase build_va_arg creates > VA_ARG_EXPR <&*s> > but during clone_body this is transformed into > VA_ARG_EXPR <s> > so we have a valid valist, but it's not an ADDR_EXPR, so we don't call > targetm.canonical_va_list_type and crash on the subsequent assert. > > Tom, does this make sense to you? Hi, duing the compilation resulting in the ICE, the "Case 1" is triggered in build_va_arg, so we need the targetm.canonical_va_list_type retry in gimplify_va_arg_expr. Hence, the patch looks good to me. [ FTR, it would be cleaner to add an encoding of the case to VA_ARG_EXPR and call either: ... have_va_type = targetm.canonical_va_list_type (TREE_TYPE (TREE_TYPE (valist))); ... or: ... have_va_type = targetm.canonical_va_list_type (TREE_TYPE (valist)); ... in gimplify_va_arg_expr based on that encoding, but that's probably overkill. ] Thanks, - Tom
On Mon, Dec 12, 2016 at 11:10:31AM +0100, Tom de Vries wrote: > On 08/12/16 22:07, Marek Polacek wrote: > > This test ICEs since r240072 where Tom disallowed using va_list * as a first > > argument to va_arg. I think the problem is in the ADDR_EXPR check in > > gimplify_va_arg_expr. It's meant to handle Case 1, but the valist doesn't > > always have to be ADDR_EXPR. E.g. for this testcase build_va_arg creates > > VA_ARG_EXPR <&*s> > > but during clone_body this is transformed into > > VA_ARG_EXPR <s> > > so we have a valid valist, but it's not an ADDR_EXPR, so we don't call > > targetm.canonical_va_list_type and crash on the subsequent assert. > > > > Tom, does this make sense to you? > > Hi, > > duing the compilation resulting in the ICE, the "Case 1" is triggered in > build_va_arg, so we need the targetm.canonical_va_list_type retry in > gimplify_va_arg_expr. Hence, the patch looks good to me. Thanks. Ok to commit, then? > [ FTR, it would be cleaner to add an encoding of the case to VA_ARG_EXPR and > call either: > ... > have_va_type > = targetm.canonical_va_list_type (TREE_TYPE (TREE_TYPE (valist))); > ... > > or: > ... > have_va_type > = targetm.canonical_va_list_type (TREE_TYPE (valist)); > ... > in gimplify_va_arg_expr based on that encoding, but that's probably > overkill. ] Marek
On Thu, Dec 08, 2016 at 10:07:59PM +0100, Marek Polacek wrote: > 2016-12-08 Marek Polacek <polacek@redhat.com> > > PR middle-end/78716 > * gimplify.c (gimplify_va_arg_expr): Don't require ADDR_EXPR for > Case 1. > > * g++.dg/other/vararg-5.C: New. > > diff --git gcc/gimplify.c gcc/gimplify.c > index 8611060..08c4faa 100644 > --- gcc/gimplify.c > +++ gcc/gimplify.c > @@ -12642,8 +12642,7 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, > if (have_va_type == error_mark_node) > return GS_ERROR; > have_va_type = targetm.canonical_va_list_type (have_va_type); > - if (have_va_type == NULL_TREE > - && TREE_CODE (valist) == ADDR_EXPR) > + if (have_va_type == NULL_TREE) > /* Handle 'Case 1: Not an array type' from c-common.c/build_va_arg. */ > have_va_type > = targetm.canonical_va_list_type (TREE_TYPE (TREE_TYPE (valist))); This assumes that TREE_TYPE (TREE_TYPE (valist)) is meaningful, which isn't always the case. So, shall it check instead that POINTER_TYPE_P (TREE_TYPE (valist)) instead, or do we have any guarantees valist must be a pointer at this point? > diff --git gcc/testsuite/g++.dg/other/vararg-5.C gcc/testsuite/g++.dg/other/vararg-5.C > index e69de29..9327bd6 100644 > --- gcc/testsuite/g++.dg/other/vararg-5.C > +++ gcc/testsuite/g++.dg/other/vararg-5.C > @@ -0,0 +1,24 @@ > +// PR middle-end/78716 > +// { dg-do compile } > + > +template <typename = int, typename = int, typename = int, typename = int, > + typename = int> > + struct a; > + template <typename> struct b; > + template <typename = int, typename d = void> class e : b<d>::c { > + public: > + typedef e f; > + typedef typename b<d>::c g; > + e(__builtin_va_list *s) : g(__builtin_va_arg(*s, int)) {} > + }; > +template <> struct b<void> { typedef e<> c; }; > +template <> struct e<> { template <typename h> e(h); }; > +template <typename i> class a<i> : public e<i> {}; > +template <typename i, typename j, typename k, typename l> > +class a<i, j, k, l> : e<typename a<j>::f> { > + public: > + template <typename m, typename n, typename o, typename p> > + a(a<m, n, o, p>) : a::f(0) {} > +}; > +template <typename i, typename j, typename k, typename l> a<> r(i, j, k, l); > +void q() { a<float, float>(r(4, 6, 9, 7)); } > > Marek Jakub
On December 12, 2016 11:56:36 AM GMT+01:00, Marek Polacek <polacek@redhat.com> wrote: >On Mon, Dec 12, 2016 at 11:10:31AM +0100, Tom de Vries wrote: >> On 08/12/16 22:07, Marek Polacek wrote: >> > This test ICEs since r240072 where Tom disallowed using va_list * >as a first >> > argument to va_arg. I think the problem is in the ADDR_EXPR check >in >> > gimplify_va_arg_expr. It's meant to handle Case 1, but the valist >doesn't >> > always have to be ADDR_EXPR. E.g. for this testcase build_va_arg >creates >> > VA_ARG_EXPR <&*s> >> > but during clone_body this is transformed into >> > VA_ARG_EXPR <s> >> > so we have a valid valist, but it's not an ADDR_EXPR, so we don't >call >> > targetm.canonical_va_list_type and crash on the subsequent assert. >> > >> > Tom, does this make sense to you? >> >> Hi, >> >> duing the compilation resulting in the ICE, the "Case 1" is triggered >in >> build_va_arg, so we need the targetm.canonical_va_list_type retry in >> gimplify_va_arg_expr. Hence, the patch looks good to me. > >Thanks. > >Ok to commit, then? > OK. Richard. >> [ FTR, it would be cleaner to add an encoding of the case to >VA_ARG_EXPR and >> call either: >> ... >> have_va_type >> = targetm.canonical_va_list_type (TREE_TYPE (TREE_TYPE >(valist))); >> ... >> >> or: >> ... >> have_va_type >> = targetm.canonical_va_list_type (TREE_TYPE (valist)); >> ... >> in gimplify_va_arg_expr based on that encoding, but that's probably >> overkill. ] > > Marek
diff --git gcc/gimplify.c gcc/gimplify.c index 8611060..08c4faa 100644 --- gcc/gimplify.c +++ gcc/gimplify.c @@ -12642,8 +12642,7 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, if (have_va_type == error_mark_node) return GS_ERROR; have_va_type = targetm.canonical_va_list_type (have_va_type); - if (have_va_type == NULL_TREE - && TREE_CODE (valist) == ADDR_EXPR) + if (have_va_type == NULL_TREE) /* Handle 'Case 1: Not an array type' from c-common.c/build_va_arg. */ have_va_type = targetm.canonical_va_list_type (TREE_TYPE (TREE_TYPE (valist))); diff --git gcc/testsuite/g++.dg/other/vararg-5.C gcc/testsuite/g++.dg/other/vararg-5.C index e69de29..9327bd6 100644 --- gcc/testsuite/g++.dg/other/vararg-5.C +++ gcc/testsuite/g++.dg/other/vararg-5.C @@ -0,0 +1,24 @@ +// PR middle-end/78716 +// { dg-do compile } + +template <typename = int, typename = int, typename = int, typename = int, + typename = int> + struct a; + template <typename> struct b; + template <typename = int, typename d = void> class e : b<d>::c { + public: + typedef e f; + typedef typename b<d>::c g; + e(__builtin_va_list *s) : g(__builtin_va_arg(*s, int)) {} + }; +template <> struct b<void> { typedef e<> c; }; +template <> struct e<> { template <typename h> e(h); }; +template <typename i> class a<i> : public e<i> {}; +template <typename i, typename j, typename k, typename l> +class a<i, j, k, l> : e<typename a<j>::f> { + public: + template <typename m, typename n, typename o, typename p> + a(a<m, n, o, p>) : a::f(0) {} +}; +template <typename i, typename j, typename k, typename l> a<> r(i, j, k, l); +void q() { a<float, float>(r(4, 6, 9, 7)); }