On December 12, 2016 11:56:36 AM GMT+01:00, Marek Polacek <pola...@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
