| Message ID | 61d0854f-16be-8bd6-a776-e9d51dd00739@mentor.com |
|---|---|
| State | New |
| Headers | show |
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
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
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).
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
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'" } */ +}