From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x743.google.com (mail-qk1-x743.google.com [IPv6:2607:f8b0:4864:20::743]) by sourceware.org (Postfix) with ESMTPS id E7AB2384402A for ; Thu, 9 Jul 2020 16:44:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E7AB2384402A Received: by mail-qk1-x743.google.com with SMTP id j80so2444494qke.0 for ; Thu, 09 Jul 2020 09:44:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=RJDb7yQI5e3kPxrvvmVFBOW/Zt3Q72jcZKrJvRhx9Ck=; b=eQPYLVHarn/ocNKzpllDSrnWFe1U9o5BWoBEYMIFK88Bfh/B13hG6lxyLcx9wLNKBP s3F2Vtx3/WbfSgXz0Zddj4B+cl/jkWqZC1uQOXtFk476A1t/xAvBzDcnpzTzo0l7yUOU g1xlF02QztV0a5OdSTakvjF1dmy8EtQ7ROXyF2N6Mb4gH1OuiWVG0onP+znka7oIdUra xLbODu0tK0PkVfqQBZBsDSr831hIRfWMI63fR+3fGK+l8ydcgvP4UQf8NF274PouEE+h ZA3gkVlONkeCMoFW3MME42SJDJmZOTcxwjX0uYCWIfszlmFuXbM5En3r3xjFcgOli13V KRjw== X-Gm-Message-State: AOAM532Pa4FNAAXY/lmNBKig5v+W/XtClEPhEA46grl4+D0zzZnhJpsH lfemqIxsZFUKfCp7lgFjGXCjO1gqgXA= X-Google-Smtp-Source: ABdhPJw7b3iYCEwsX4KVVNOJl75ZaY8U1yw7pklANbz4WIBr6R4EMhCHkXQPGn4q4t0wrepnAewBPw== X-Received: by 2002:a05:620a:1385:: with SMTP id k5mr61505109qki.148.1594313044882; Thu, 09 Jul 2020 09:44:04 -0700 (PDT) Received: from ?IPv6:2804:7f0:8283:20c3:e9c8:8185:17e5:d78b? ([2804:7f0:8283:20c3:e9c8:8185:17e5:d78b]) by smtp.gmail.com with ESMTPSA id e203sm4205957qkb.87.2020.07.09.09.44.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jul 2020 09:44:03 -0700 (PDT) Subject: Re: [PATCH v2] Skip VLA structure field tests when compiling with clang To: Gary Benson , gdb-patches@sourceware.org References: <1594223215-21455-1-git-send-email-gbenson@redhat.com> From: Luis Machado Message-ID: Date: Thu, 9 Jul 2020 13:44:01 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <1594223215-21455-1-git-send-email-gbenson@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Jul 2020 16:44:08 -0000 Hi Gary, On 7/8/20 12:46 PM, Gary Benson wrote: > Hi Luis, > > Luis Machado wrote: >> As a general comment (for this and other patches like this), I think >> the preprocessor blocks intermixed with the test code make it harder >> to follow and update the test. >> >> If clang claims it will never support a particular feature, then >> maybe we should ... separate the bits clang doesn't support and put >> it into a separate test that clang-compiled hosts should skip? > > Thanks for the review. I've made that change, the updated patch is > below. I checked it on Fedora 31 x86_64, GCC and clang. Is it ok to > commit? Thanks. That looks cleaner. Just a gotcha below... > > Cheers > Gary > > -- > Clang fails to compile gdb.base/vla-datatypes.c with the following > error: fields must have a constant size: 'variable length array in > structure' extension will never be supported. This patch factors > the affected tests out into a new testcase, vla-struct-fields.{exp,c}, > which is skipped when the testcase is compiled using clang, > > gdb/testsuite/ChangeLog: > > * gdb.base/vla-datatypes.c (vla_factory): Factor out sections > defining and using VLA structure fields into... > * gdb.base/vla-struct-fields.c: New file. > * gdb.base/vla-datatypes.exp: Factor out VLA structure field > tests into... > * gdb.base/vla-struct-fields.exp: New file. > --- > gdb/testsuite/ChangeLog | 9 +++ > gdb/testsuite/gdb.base/vla-datatypes.c | 53 -------------- > gdb/testsuite/gdb.base/vla-datatypes.exp | 24 ------- > gdb/testsuite/gdb.base/vla-struct-fields.c | 104 +++++++++++++++++++++++++++ > gdb/testsuite/gdb.base/vla-struct-fields.exp | 67 +++++++++++++++++ > 5 files changed, 180 insertions(+), 77 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/vla-struct-fields.c > create mode 100644 gdb/testsuite/gdb.base/vla-struct-fields.exp > > diff --git a/gdb/testsuite/gdb.base/vla-datatypes.c b/gdb/testsuite/gdb.base/vla-datatypes.c > index 682319f..d8b835a 100644 > --- a/gdb/testsuite/gdb.base/vla-datatypes.c > +++ b/gdb/testsuite/gdb.base/vla-datatypes.c > @@ -46,49 +46,6 @@ struct foo > BAR bar_vla[n]; > int i; > > - struct vla_struct > - { > - int something; > - int vla_field[n]; > - }; > - /* Define a typedef for a VLA structure. */ > - typedef struct vla_struct vla_struct_typedef; > - vla_struct_typedef vla_struct_object; > - > - struct inner_vla_struct > - { > - int something; > - int vla_field[n]; > - int after; > - } inner_vla_struct_object; > - > - /* Define a structure which uses a typedef for the VLA field > - to make sure that GDB creates the proper type for this field, > - preventing a possible assertion failure (see gdb/21356). */ > - struct vla_struct_typedef_struct_member > - { > - int something; > - vla_struct_typedef vla_object; > - } vla_struct_typedef_struct_member_object; > - > - union vla_union > - { > - int vla_field[n]; > - } vla_union_object; > - > - /* Like vla_struct_typedef_struct_member but a union type. */ > - union vla_struct_typedef_union_member > - { > - int something; > - vla_struct_typedef vla_object; > - } vla_struct_typedef_union_member_object; > - > - vla_struct_object.something = n; > - inner_vla_struct_object.something = n; > - inner_vla_struct_object.after = n; > - vla_struct_typedef_struct_member_object.something = n * 2; > - vla_struct_typedef_struct_member_object.vla_object.something = n * 3; > - vla_struct_typedef_union_member_object.vla_object.something = n + 1; > for (i = 0; i < n; i++) > { > int_vla[i] = i*2; > @@ -104,13 +61,6 @@ struct foo > foo_vla[i].a = i*2; > bar_vla[i].x = i*2; > bar_vla[i].y.a = i*2; > - vla_struct_object.vla_field[i] = i*2; > - vla_union_object.vla_field[i] = i*2; > - inner_vla_struct_object.vla_field[i] = i*2; > - vla_struct_typedef_struct_member_object.vla_object.vla_field[i] > - = i * 3; > - vla_struct_typedef_union_member_object.vla_object.vla_field[i] > - = i * 3 - 1; > } > > size_t int_size = sizeof(int_vla); /* vlas_filled */ > @@ -124,9 +74,6 @@ struct foo > size_t uchar_size = sizeof(unsigned_char_vla); > size_t foo_size = sizeof(foo_vla); > size_t bar_size = sizeof(bar_vla); > - size_t vla_struct_object_size = sizeof(vla_struct_object); > - size_t vla_union_object_size = sizeof(vla_union_object); > - size_t inner_vla_struct_object_size = sizeof(inner_vla_struct_object); > > return; /* break_end_of_vla_factory */ > } > diff --git a/gdb/testsuite/gdb.base/vla-datatypes.exp b/gdb/testsuite/gdb.base/vla-datatypes.exp > index e8d8457..201f051 100644 > --- a/gdb/testsuite/gdb.base/vla-datatypes.exp > +++ b/gdb/testsuite/gdb.base/vla-datatypes.exp > @@ -41,14 +41,6 @@ gdb_test "print foo_vla" \ > "\\\{\\\{a = 0\\\}, \\\{a = 2\\\}, \\\{a = 4\\\}, \\\{a = 6\\\}, \\\{a = 8\\\}\\\}" > gdb_test "print bar_vla" \ > "\\\{\\\{x = 0, y = \\\{a = 0\\\}\\\}, \\\{x = 2, y = \\\{a = 2\\\}\\\}, \\\{x = 4, y = \\\{a = 4\\\}\\\}, \\\{x = 6, y = \\\{a = 6\\\}\\\}, \\\{x = 8, y = \\\{a = 8\\\}\\\}\\\}" > -gdb_test "print vla_struct_object" \ > - "\\\{something = 5, vla_field = \\\{0, 2, 4, 6, 8\\\}\\\}" > -gdb_test "print vla_union_object" \ > - "\\\{vla_field = \\\{0, 2, 4, 6, 8\\\}\\\}" > -gdb_test "print vla_struct_typedef_struct_member_object" \ > - "\\\{something = 10, vla_object = \\\{something = 15, vla_field = \\\{0, 3, 6, 9, 12\\\}\\\}\\\}" > -gdb_test "print vla_struct_typedef_union_member_object" \ > - "\\\{something = 6, vla_object = \\\{something = 6, vla_field = \\\{-1, 2, 5, 8, 11\\\}\\\}\\\}" > > # Check whatis of VLA's. > gdb_test "whatis int_vla" "type = int \\\[5\\\]" > @@ -65,8 +57,6 @@ gdb_test "whatis unsigned_short_vla" \ > gdb_test "whatis unsigned_char_vla" "type = unsigned char \\\[5\\\]" > gdb_test "whatis foo_vla" "type = struct foo \\\[5\\\]" > gdb_test "whatis bar_vla" "type = BAR \\\[5\\\]" > -gdb_test "whatis vla_struct_object" "type = vla_struct_typedef" > -gdb_test "whatis vla_union_object" "type = union vla_union" > > # Check ptype of VLA's. > gdb_test "ptype int_vla" "type = int \\\[5\\\]" > @@ -82,10 +72,6 @@ gdb_test "ptype unsigned_char_vla" "type = unsigned char \\\[5\\\]" > gdb_test "ptype foo_vla" "type = struct foo {\r\n\\s+int a;\r\n} \\\[5\\\]" > gdb_test "ptype bar_vla" \ > "type = struct bar {\r\n\\s+int x;\r\n\\s+struct foo y;\r\n} \\\[5\\\]" > -gdb_test "ptype vla_struct_object" \ > - "type = struct vla_struct {\r\n\\s+int something;\r\n\\s+int vla_field\\\[5\\\];\r\n}" > -gdb_test "ptype vla_union_object" \ > - "type = union vla_union {\r\n\\s+int vla_field\\\[5\\\];\r\n}" > > # Check the size of the VLA's. > gdb_breakpoint [gdb_get_line_number "break_end_of_vla_factory"] > @@ -109,10 +95,6 @@ gdb_test "print uchar_size == sizeof(unsigned_char_vla)" " = 1" \ > "size of unsigned_char_vla" > gdb_test "print foo_size == sizeof(foo_vla)" " = 1" "size of foo_vla" > gdb_test "print bar_size == sizeof(bar_vla)" " = 1" "size of bar_vla" > -gdb_test "print vla_struct_object_size == sizeof(vla_struct_object)" \ > - " = 1" "size of vla_struct_object" > -gdb_test "print vla_union_object_size == sizeof(vla_union_object)" \ > - " = 1" "size of vla_union_object" > > # Check side effects for sizeof argument. > set sizeof_int [get_sizeof "int" 4] > @@ -130,9 +112,3 @@ gdb_test "print int_vla\[0\]" " = 42" \ > gdb_test "whatis ++int_vla\[0\]" "type = int" > gdb_test "print int_vla\[0\]" " = 42" \ > "print int_vla\[0\] - whatis no side effects" > - > -# Fails due to incorrect debugging information generated by GCC. > -setup_xfail "*-*-*" > -gdb_test \ > - "print inner_vla_struct_object_size == sizeof(inner_vla_struct_object)" \ > - " = 1" "size of inner_vla_struct_object" > diff --git a/gdb/testsuite/gdb.base/vla-struct-fields.c b/gdb/testsuite/gdb.base/vla-struct-fields.c > new file mode 100644 > index 0000000..ef236a7 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/vla-struct-fields.c > @@ -0,0 +1,104 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2014-2020 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > +#define SIZE 5 > + > +struct foo > +{ > + int a; > +}; > + > +typedef struct bar > +{ > + int x; > + struct foo y; > +} BAR; > + > +void > +vla_factory (int n) > +{ > + struct vla_struct > + { > + int something; > + int vla_field[n]; > + }; > + /* Define a typedef for a VLA structure. */ > + typedef struct vla_struct vla_struct_typedef; > + vla_struct_typedef vla_struct_object; > + > + struct inner_vla_struct > + { > + int something; > + int vla_field[n]; > + int after; > + } inner_vla_struct_object; > + > + /* Define a structure which uses a typedef for the VLA field > + to make sure that GDB creates the proper type for this field, > + preventing a possible assertion failure (see gdb/21356). */ > + struct vla_struct_typedef_struct_member > + { > + int something; > + vla_struct_typedef vla_object; > + } vla_struct_typedef_struct_member_object; > + > + union vla_union > + { > + int vla_field[n]; > + } vla_union_object; > + > + /* Like vla_struct_typedef_struct_member but a union type. */ > + union vla_struct_typedef_union_member > + { > + int something; > + vla_struct_typedef vla_object; > + } vla_struct_typedef_union_member_object; > + int i; > + > + vla_struct_object.something = n; > + inner_vla_struct_object.something = n; > + inner_vla_struct_object.after = n; > + vla_struct_typedef_struct_member_object.something = n * 2; > + vla_struct_typedef_struct_member_object.vla_object.something = n * 3; > + vla_struct_typedef_union_member_object.vla_object.something = n + 1; > + > + for (i = 0; i < n; i++) > + { > + vla_struct_object.vla_field[i] = i*2; > + vla_union_object.vla_field[i] = i*2; > + inner_vla_struct_object.vla_field[i] = i*2; > + vla_struct_typedef_struct_member_object.vla_object.vla_field[i] > + = i * 3; > + vla_struct_typedef_union_member_object.vla_object.vla_field[i] > + = i * 3 - 1; > + } > + > + size_t vla_struct_object_size > + = sizeof(vla_struct_object); /* vlas_filled */ > + size_t vla_union_object_size = sizeof(vla_union_object); > + size_t inner_vla_struct_object_size = sizeof(inner_vla_struct_object); > + > + return; /* break_end_of_vla_factory */ > +} > + > +int > +main (void) > +{ > + vla_factory(SIZE); > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/vla-struct-fields.exp b/gdb/testsuite/gdb.base/vla-struct-fields.exp > new file mode 100644 > index 0000000..150a328 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/vla-struct-fields.exp > @@ -0,0 +1,67 @@ > +# Copyright 2014-2020 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +standard_testfile > + > +# Clang says it will never support variable length arrays in structures. > +if {[test_compiler_info clang*]} { > + unsupported "compiler does not variable length arrays in structure" ^^^ support? Otherwise this looks good to me. Can't approve a commit though.