[PATCH, PR71602, 4/4] Make canonical_va_list_type more strict

[Tom de Vries <Tom_deVries@mentor.com>]

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

On 23/06/16 23:21, Jason Merrill wrote:
> 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:>> 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
>

>> ...
>> __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?
>

This patch fixes PR71602 by making canonical_va_list_type more strict.

Bootstrapped and reg-tested on x86_64.

OK for trunk, 6-branch?

Thanks,
- Tom