* [PATCH, PR71602] Give error for invalid va_list argument to va_arg
@ 2016-06-23 10:27 Tom de Vries
2016-06-23 21:22 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2016-06-23 10:27 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]
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.
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.
Bootstrapped and reg-tested on x86_64 (-m64 and -m32).
OK for trunk?
Thanks,
- Tom
[-- Attachment #2: 0001-Give-error-for-invalid-va_list-argument-to-va_arg.patch --]
[-- Type: text/x-patch, Size: 5804 bytes --]
Give error for invalid va_list argument to va_arg
2016-06-22 Tom de Vries <tom@codesourcery.com>
PR c/71602
* c-common.c (build_va_arg): Add comp_types parameter. Give error for
invalid va_list argument.
* c-common.h (build_va_arg): Add comp_types parameter.
* c-typeck.c (va_list_comptypes): New function.
(c_build_va_arg): Add argument to build_va_arg call.
* call.c (build_x_va_arg): Add argument to build_va_arg call.
* c-c++-common/va-arg-va-list-type.c: New test.
---
gcc/c-family/c-common.c | 34 ++++++++++++++----------
gcc/c-family/c-common.h | 2 +-
gcc/c/c-typeck.c | 11 +++++++-
gcc/cp/call.c | 6 +++--
gcc/testsuite/c-c++-common/va-arg-va-list-type.c | 9 +++++++
5 files changed, 44 insertions(+), 18 deletions(-)
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 85f3a03..8d0f335 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5696,7 +5696,8 @@ build_va_arg_1 (location_t loc, tree type, tree op)
va_arg (EXPR, TYPE) at source location LOC. */
tree
-build_va_arg (location_t loc, tree expr, tree type)
+build_va_arg (location_t loc, tree expr, tree type,
+ bool (*comp_types) (tree, tree))
{
tree va_type = TREE_TYPE (expr);
tree canon_va_type = (va_type == error_mark_node
@@ -5712,6 +5713,14 @@ build_va_arg (location_t loc, tree expr, tree type)
return build_va_arg_1 (loc, type, expr);
}
+ bool valid_va_list
+ = ((TREE_CODE (canon_va_type) == ARRAY_TYPE
+ && TREE_CODE (va_type) == POINTER_TYPE)
+ ? comp_types (TREE_TYPE (canon_va_type), TREE_TYPE (va_type))
+ : comp_types (canon_va_type, va_type));
+ if (!valid_va_list)
+ goto fail_first_arg_type;
+
if (TREE_CODE (canon_va_type) != ARRAY_TYPE)
{
/* Case 1: Not an array type. */
@@ -5724,11 +5733,7 @@ build_va_arg (location_t loc, tree expr, tree type)
tree canon_expr_type
= targetm.canonical_va_list_type (TREE_TYPE (expr));
if (canon_expr_type == NULL_TREE)
- {
- error_at (loc,
- "first argument to %<va_arg%> not of type %<va_list%>");
- return error_mark_node;
- }
+ goto fail_first_arg_type;
return build_va_arg_1 (loc, type, expr);
}
@@ -5797,23 +5802,24 @@ build_va_arg (location_t loc, tree expr, tree type)
tree canon_expr_type
= targetm.canonical_va_list_type (TREE_TYPE (expr));
if (canon_expr_type == NULL_TREE)
- {
- error_at (loc,
- "first argument to %<va_arg%> not of type %<va_list%>");
- return error_mark_node;
- }
+ goto fail_first_arg_type;
}
- else
+ else if (TREE_CODE (va_type) == POINTER_TYPE)
{
/* 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'. */
;
}
+ else
+ goto fail_first_arg_type;
return build_va_arg_1 (loc, type, expr);
+
+ fail_first_arg_type:
+ error_at (loc,
+ "first argument to %<va_arg%> not of type %<va_list%>");
+ return error_mark_node;
}
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 4e6aa00..37bfd07 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -878,7 +878,7 @@ extern void disable_builtin_function (const char *);
extern void set_compound_literal_name (tree decl);
-extern tree build_va_arg (location_t, tree, tree);
+extern tree build_va_arg (location_t, tree, tree, bool (*) (tree, tree));
extern const unsigned int c_family_lang_mask;
extern unsigned int c_common_option_lang_mask (void);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 7c6241c..e96e31c 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -13674,6 +13674,15 @@ c_build_qualified_type (tree type, int type_quals, tree orig_qual_type,
return var_type;
}
+/* Callback function for c_build_va_arg to compare
+ va_list types. */
+
+static bool
+va_list_comptypes (tree type1, tree type2)
+{
+ return comptypes (type1, type2) == 1;
+}
+
/* Build a VA_ARG_EXPR for the C parser. */
tree
@@ -13698,7 +13707,7 @@ c_build_va_arg (location_t loc1, tree expr, location_t loc2, tree type)
else if (warn_cxx_compat && TREE_CODE (type) == ENUMERAL_TYPE)
warning_at (loc2, OPT_Wc___compat,
"C++ requires promoted type, not enum type, in %<va_arg%>");
- return build_va_arg (loc2, expr, type);
+ return build_va_arg (loc2, expr, type, va_list_comptypes);
}
/* Return truthvalue of whether T1 is the same tree structure as T2.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b739fa0..5e38326 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6956,11 +6956,13 @@ build_x_va_arg (source_location loc, tree expr, tree type)
"through %<...%> is conditionally-supported", type);
tree ref = cp_build_reference_type (type, false);
- expr = build_va_arg (loc, expr, ref);
+ expr = build_va_arg (loc, expr, ref,
+ same_type_ignoring_top_level_qualifiers_p);
return convert_from_reference (expr);
}
- return build_va_arg (loc, expr, type);
+ return build_va_arg (loc, expr, type,
+ same_type_ignoring_top_level_qualifiers_p);
}
/* TYPE has been given to va_arg. Apply the default conversions which
diff --git a/gcc/testsuite/c-c++-common/va-arg-va-list-type.c b/gcc/testsuite/c-c++-common/va-arg-va-list-type.c
new file mode 100644
index 0000000..cdd97cf
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/va-arg-va-list-type.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+
+__builtin_va_list *pap;
+
+void
+fn1 (void)
+{
+ __builtin_va_arg (pap, double); /* { dg-error "first argument to 'va_arg' not of type 'va_list'" } */
+}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602] Give error for invalid va_list argument to va_arg
2016-06-23 10:27 [PATCH, PR71602] Give error for invalid va_list argument to va_arg Tom de Vries
@ 2016-06-23 21:22 ` Jason Merrill
2016-08-24 6:22 ` [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict Tom de Vries
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2016-06-23 21:22 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-06-23 21:22 ` Jason Merrill
@ 2016-08-24 6:22 ` Tom de Vries
2016-08-29 15:51 ` Joseph Myers
0 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2016-08-24 6:22 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
[ 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-08-24 6:22 ` [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict Tom de Vries
@ 2016-08-29 15:51 ` Joseph Myers
2016-08-29 16:12 ` Tom de Vries
0 siblings, 1 reply; 9+ messages in thread
From: Joseph Myers @ 2016-08-29 15:51 UTC (permalink / raw)
To: Tom de Vries; +Cc: Jason Merrill, GCC Patches
On Wed, 24 Aug 2016, Tom de Vries wrote:
> This patch fixes PR71602 by making canonical_va_list_type more strict.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk, 6-branch?
ENOPATCH
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-08-29 15:51 ` Joseph Myers
@ 2016-08-29 16:12 ` Tom de Vries
2016-09-09 22:12 ` Jeff Law
2016-09-19 11:20 ` Szabolcs Nagy
0 siblings, 2 replies; 9+ messages in thread
From: Tom de Vries @ 2016-08-29 16:12 UTC (permalink / raw)
To: Joseph Myers; +Cc: Jason Merrill, GCC Patches
[-- Attachment #1: Type: text/plain, Size: 290 bytes --]
On 29/08/16 17:51, Joseph Myers wrote:
> On Wed, 24 Aug 2016, Tom de Vries wrote:
>
>> This patch fixes PR71602 by making canonical_va_list_type more strict.
>>
>> Bootstrapped and reg-tested on x86_64.
>>
>> OK for trunk, 6-branch?
>
> ENOPATCH
>
Patch attached this time.
Thanks,
- Tom
[-- Attachment #2: 0004-Make-canonical_va_list_type-more-strict.patch --]
[-- Type: text/x-patch, Size: 3842 bytes --]
Make canonical_va_list_type more strict
2016-08-22 Tom de Vries <tom@codesourcery.com>
PR C/71602
* builtins.c (std_canonical_va_list_type): Strictly return non-null for
va_list type only.
* config/i386/i386.c (ix86_canonical_va_list_type): Same.
* gimplify.c (gimplify_va_arg_expr): Handle &va_list.
* c-common.c (build_va_arg): Handle more strict
targetm.canonical_va_list_type.
* c-c++-common/va-arg-va-list-type.c: New test.
---
gcc/builtins.c | 4 ----
gcc/c-family/c-common.c | 8 ++------
gcc/config/i386/i386.c | 8 --------
gcc/gimplify.c | 5 +++++
gcc/testsuite/c-c++-common/va-arg-va-list-type.c | 9 +++++++++
5 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/gcc/builtins.c b/gcc/builtins.c
index abc934b..101b1e3 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4089,10 +4089,6 @@ std_canonical_va_list_type (tree type)
{
tree wtype, htype;
- if (INDIRECT_REF_P (type))
- type = TREE_TYPE (type);
- else if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE (type)))
- type = TREE_TYPE (type);
wtype = va_list_type_node;
htype = type;
/* Treat structure va_list types. */
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3b61e64..6cf2ffc 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5813,15 +5813,11 @@ build_va_arg (location_t loc, tree expr, tree type)
{
/* Case 1: Not an array type. */
- /* Take the address, to get '&ap'. */
+ /* Take the address, to get '&ap'. Note that &ap is not a va_list
+ type. */
mark_addressable (expr);
expr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (expr)), expr);
- /* Verify that &ap is still recognized as having va_list type. */
- tree canon_expr_type
- = targetm.canonical_va_list_type (TREE_TYPE (expr));
- gcc_assert (canon_expr_type != NULL_TREE);
-
return build_va_arg_1 (loc, type, expr);
}
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 2639c8c..343efa0 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -48565,14 +48565,6 @@ ix86_canonical_va_list_type (tree type)
{
tree wtype, htype;
- /* Resolve references and pointers to va_list type. */
- if (TREE_CODE (type) == MEM_REF)
- type = TREE_TYPE (type);
- else if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE(type)))
- type = TREE_TYPE (type);
- else if (POINTER_TYPE_P (type) && TREE_CODE (TREE_TYPE (type)) == ARRAY_TYPE)
- type = TREE_TYPE (type);
-
if (TARGET_64BIT && va_list_type_node != NULL_TREE)
{
wtype = va_list_type_node;
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 288b472..6ce516a 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -11959,6 +11959,11 @@ 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)
+ /* 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)));
gcc_assert (have_va_type != NULL_TREE);
/* Generate a diagnostic for requesting data of a type that cannot
diff --git a/gcc/testsuite/c-c++-common/va-arg-va-list-type.c b/gcc/testsuite/c-c++-common/va-arg-va-list-type.c
new file mode 100644
index 0000000..cdd97cf
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/va-arg-va-list-type.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+
+__builtin_va_list *pap;
+
+void
+fn1 (void)
+{
+ __builtin_va_arg (pap, double); /* { dg-error "first argument to 'va_arg' not of type 'va_list'" } */
+}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-08-29 16:12 ` Tom de Vries
@ 2016-09-09 22:12 ` Jeff Law
2016-09-11 9:39 ` Christophe Lyon
2016-09-19 11:20 ` Szabolcs Nagy
1 sibling, 1 reply; 9+ messages in thread
From: Jeff Law @ 2016-09-09 22:12 UTC (permalink / raw)
To: Tom de Vries, Joseph Myers; +Cc: Jason Merrill, GCC Patches
On 08/29/2016 10:12 AM, Tom de Vries wrote:
> On 29/08/16 17:51, Joseph Myers wrote:
>> On Wed, 24 Aug 2016, Tom de Vries wrote:
>>
>>> This patch fixes PR71602 by making canonical_va_list_type more strict.
>>>
>>> Bootstrapped and reg-tested on x86_64.
>>>
>>> OK for trunk, 6-branch?
>>
>> ENOPATCH
>>
>
> Patch attached this time.
>
> Thanks,
> - Tom
>
> 0004-Make-canonical_va_list_type-more-strict.patch
>
>
> Make canonical_va_list_type more strict
>
> 2016-08-22 Tom de Vries <tom@codesourcery.com>
>
> PR C/71602
> * builtins.c (std_canonical_va_list_type): Strictly return non-null for
> va_list type only.
> * config/i386/i386.c (ix86_canonical_va_list_type): Same.
> * gimplify.c (gimplify_va_arg_expr): Handle &va_list.
>
> * c-common.c (build_va_arg): Handle more strict
> targetm.canonical_va_list_type.
>
> * c-c++-common/va-arg-va-list-type.c: New test.
>
> ---
Happy to see the _REF nodes disappearing from
std_canonical_va_list_type. If I understand the code correctly its
argument should always be a type node, so handling an _REF node makes no
sense.
OK for the trunk.
jeff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-09-09 22:12 ` Jeff Law
@ 2016-09-11 9:39 ` Christophe Lyon
0 siblings, 0 replies; 9+ messages in thread
From: Christophe Lyon @ 2016-09-11 9:39 UTC (permalink / raw)
To: Jeff Law; +Cc: Tom de Vries, Joseph Myers, Jason Merrill, GCC Patches
On 10 September 2016 at 00:04, Jeff Law <law@redhat.com> wrote:
> On 08/29/2016 10:12 AM, Tom de Vries wrote:
>>
>> On 29/08/16 17:51, Joseph Myers wrote:
>>>
>>> On Wed, 24 Aug 2016, Tom de Vries wrote:
>>>
>>>> This patch fixes PR71602 by making canonical_va_list_type more strict.
>>>>
>>>> Bootstrapped and reg-tested on x86_64.
>>>>
>>>> OK for trunk, 6-branch?
>>>
>>>
>>> ENOPATCH
>>>
>>
>> Patch attached this time.
>>
>> Thanks,
>> - Tom
>>
>> 0004-Make-canonical_va_list_type-more-strict.patch
>>
>>
>> Make canonical_va_list_type more strict
>>
>> 2016-08-22 Tom de Vries <tom@codesourcery.com>
>>
>> PR C/71602
>> * builtins.c (std_canonical_va_list_type): Strictly return
>> non-null for
>> va_list type only.
>> * config/i386/i386.c (ix86_canonical_va_list_type): Same.
>> * gimplify.c (gimplify_va_arg_expr): Handle &va_list.
>>
>> * c-common.c (build_va_arg): Handle more strict
>> targetm.canonical_va_list_type.
>>
>> * c-c++-common/va-arg-va-list-type.c: New test.
>>
>> ---
>
> Happy to see the _REF nodes disappearing from std_canonical_va_list_type.
> If I understand the code correctly its argument should always be a type
> node, so handling an _REF node makes no sense.
>
> OK for the trunk.
>
> jeff
Hi Tom,
I've noticed that the new testcase (va-arg-va-list-type.c) fails on
arm and aarch64 targets.
On aarch64, the compiler emits no error/warning message, hence the test failure.
On arm, the compiler actually ICEs:
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/c-c++-common/va-arg-va-list-type.c:
In function 'fn1':
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/c-c++-common/va-arg-va-list-type.c:8:26:
internal compiler error: tree check: expected record_type or
union_type or qual_union_type, have pointer_type in
arm_extract_valist_ptr, at config/arm/arm.c:2767
0xde4cda tree_check_failed(tree_node const*, char const*, int, char const*, ...)
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree.c:9742
0xe8c28a tree_check3
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree.h:3066
0xe8c28a arm_extract_valist_ptr
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:2767
0xe8c2b0 arm_gimplify_va_arg_expr
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:2788
0xd566fa expand_ifn_va_arg_1
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-stdarg.c:1043
0xd566fa expand_ifn_va_arg
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-stdarg.c:1111
0xd5697b execute
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-stdarg.c:1217
Let me know if you need more details.
Thanks,
Christophe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-08-29 16:12 ` Tom de Vries
2016-09-09 22:12 ` Jeff Law
@ 2016-09-19 11:20 ` Szabolcs Nagy
2016-09-19 11:25 ` Tom de Vries
1 sibling, 1 reply; 9+ messages in thread
From: Szabolcs Nagy @ 2016-09-19 11:20 UTC (permalink / raw)
To: Tom de Vries, Joseph Myers; +Cc: nd, Jason Merrill, GCC Patches
On 29/08/16 17:12, Tom de Vries wrote:
> On 29/08/16 17:51, Joseph Myers wrote:
>> On Wed, 24 Aug 2016, Tom de Vries wrote:
>>
>>> This patch fixes PR71602 by making canonical_va_list_type more strict.
> 2016-08-22 Tom de Vries <tom@codesourcery.com>
>
> PR C/71602
> * builtins.c (std_canonical_va_list_type): Strictly return non-null for
> va_list type only.
this does not seem to be true.
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index abc934b..101b1e3 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -4089,10 +4089,6 @@ std_canonical_va_list_type (tree type)
> {
> tree wtype, htype;
>
> - if (INDIRECT_REF_P (type))
> - type = TREE_TYPE (type);
> - else if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE (type)))
> - type = TREE_TYPE (type);
> wtype = va_list_type_node;
> htype = type;
> /* Treat structure va_list types. */
here the code follows as
if (TREE_CODE (wtype) == RECORD_TYPE && POINTER_TYPE_P (htype))
htype = TREE_TYPE (htype);
else if (TREE_CODE (wtype) == ARRAY_TYPE)
[...]
if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
return va_list_type_node;
return NULL_TREE;
i think va_list* is accepted as va_list in the
first check if it is a struct type.
i think this check is not needed and causes
problems on arm and aarch64 (where va_list is
a struct type).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict
2016-09-19 11:20 ` Szabolcs Nagy
@ 2016-09-19 11:25 ` Tom de Vries
0 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2016-09-19 11:25 UTC (permalink / raw)
To: Szabolcs Nagy, Joseph Myers; +Cc: nd, Jason Merrill, GCC Patches
On 19/09/16 13:02, Szabolcs Nagy wrote:
> On 29/08/16 17:12, Tom de Vries wrote:
>> > On 29/08/16 17:51, Joseph Myers wrote:
>>> >> On Wed, 24 Aug 2016, Tom de Vries wrote:
>>> >>
>>>> >>> This patch fixes PR71602 by making canonical_va_list_type more strict.
>> > 2016-08-22 Tom de Vries <tom@codesourcery.com>
>> >
>> > PR C/71602
>> > * builtins.c (std_canonical_va_list_type): Strictly return non-null for
>> > va_list type only.
> this does not seem to be true.
>
>> > diff --git a/gcc/builtins.c b/gcc/builtins.c
>> > index abc934b..101b1e3 100644
>> > --- a/gcc/builtins.c
>> > +++ b/gcc/builtins.c
>> > @@ -4089,10 +4089,6 @@ std_canonical_va_list_type (tree type)
>> > {
>> > tree wtype, htype;
>> >
>> > - if (INDIRECT_REF_P (type))
>> > - type = TREE_TYPE (type);
>> > - else if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE (type)))
>> > - type = TREE_TYPE (type);
>> > wtype = va_list_type_node;
>> > htype = type;
>> > /* Treat structure va_list types. */
> here the code follows as
>
> if (TREE_CODE (wtype) == RECORD_TYPE && POINTER_TYPE_P (htype))
> htype = TREE_TYPE (htype);
> else if (TREE_CODE (wtype) == ARRAY_TYPE)
> [...]
> if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
> return va_list_type_node;
>
> return NULL_TREE;
>
> i think va_list* is accepted as va_list in the
> first check if it is a struct type.
>
> i think this check is not needed and causes
> problems on arm and aarch64 (where va_list is
> a struct type).
>
Yes, that's a known issue: PR77558 - [6/7 regression]
c-c++-common/va-arg-va-list-type.c fails for arm/aarch64.
Thanks,
- Tom
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77558
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-19 11:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-23 10:27 [PATCH, PR71602] Give error for invalid va_list argument to va_arg Tom de Vries
2016-06-23 21:22 ` Jason Merrill
2016-08-24 6:22 ` [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict Tom de Vries
2016-08-29 15:51 ` Joseph Myers
2016-08-29 16:12 ` Tom de Vries
2016-09-09 22:12 ` Jeff Law
2016-09-11 9:39 ` Christophe Lyon
2016-09-19 11:20 ` Szabolcs Nagy
2016-09-19 11:25 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).