Re: [PATCH, PR71602] Give error for invalid va_list argument to va_arg

[Jason Merrill <jason at redhat dot com>]

On Thu, Jun 23, 2016 at 1:27 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> this patch fixes PR71602, a 6/7 regression.
>
> Consider this test-case:
> ...
> __builtin_va_list *pap;
>
> void
> fn1 (void)
> {
>  __builtin_va_arg(pap, double);
> }
> ...
>
> The testcase is invalid, because we're not passing a va_list as first
> argument of va_arg, but a va_list*.
>
> When compiling for x86_64 -m64, we run into the second assert in this
> snippet from build_va_arg:
> ...
>     {
>       /* Case 2b: va_list is pointer to array elem type.  */
>       gcc_assert (POINTER_TYPE_P (va_type));
>       gcc_assert (TREE_TYPE (va_type) == TREE_TYPE (canon_va_type));
>
>       /* Don't take the address.  We've already got '&ap'.  */
>       ;
>     }
> ...
>
> At that point, va_type and canon_va_type are:
> ...
> (gdb) call debug_generic_expr (va_type)
> struct [1] *
> (gdb) call debug_generic_expr (canon_va_type)
> struct [1]
> ...
>
> so TREE_TYPE (va_type) and TREE_TYPE (canon_va_type) are not equal:
> ...
> (gdb) call debug_generic_expr (va_type.typed.type)
> struct [1]
> (gdb) call debug_generic_expr (canon_va_type.typed.type)
> struct
> ...
>
> Given the semantics of the target hook:
> ...
> Target Hook: tree TARGET_CANONICAL_VA_LIST_TYPE (tree type)
>
>     This hook returns the va_list type of the calling convention specified
> by the type of type. If type is not a valid va_list type, it returns
> NULL_TREE.
> ...
> one could argue that canonical_va_list_type should return NULL_TREE for a
> va_list*, which would fix the ICE. But the current implementation seems to
> rely on canonical_va_list_type to return va_list for a va_list* argument.

It does seem like it's the job of canonical_va_list_type to detect an
invalid argument.  Why not fix that?

> The patch fixes the ICE by making the valid va_list check in build_va_arg
> more precise, by taking into account the non-strict behavior of
> canonical_va_list_type.

If you do need to check this here, is there a reason you need to pass
in a callback rather than use lang_hooks.types_compatible_p?

Jason