Message ID | 20150516132555.GB17266@host1.jankratochvil.net |
---|---|
State | New, archived |
Headers |
Received: (qmail 82386 invoked by alias); 16 May 2015 13:26:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 82376 invoked by uid 89); 16 May 2015 13:26:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sat, 16 May 2015 13:26:01 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 1E93A29320B for <gdb-patches@sourceware.org>; Sat, 16 May 2015 13:26:00 +0000 (UTC) Received: from host1.jankratochvil.net (ovpn-116-27.ams2.redhat.com [10.36.116.27]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4GDPuvB015290 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 16 May 2015 09:25:58 -0400 Date: Sat, 16 May 2015 15:25:55 +0200 From: Jan Kratochvil <jan.kratochvil@redhat.com> To: gdb-patches@sourceware.org Cc: Phil Muldoon <pmuldoon@redhat.com> Subject: [patchv2] compile: Fix crash on cv-qualified self-reference Message-ID: <20150516132555.GB17266@host1.jankratochvil.net> References: <20150418172843.GA17777@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="BOKacYhQ+x31HxR3" Content-Disposition: inline In-Reply-To: <20150418172843.GA17777@host1.jankratochvil.net> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes |
Commit Message
Jan Kratochvil
May 16, 2015, 1:25 p.m. UTC
On Sat, 18 Apr 2015 19:28:43 +0200, Jan Kratochvil wrote: Hi, with this modified testcase GDB would: compile code struct_object.selffield = &struct_object ./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error) While the insert_type() assertion looks unclear trying to fix it one ends up with either GCC crash [gcc libcc1] build_qualified_type for self-referencing/incomplete types https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html c_incomplete_type_error() or after fixing up the GCC type for proper error reporting one gets: gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’ which is the real culprit of this bug as explained in this patch. This patch is related to the XFAIL introduced by [PATCH v3 5/9] compile: Use -Wall, not -w https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html as for proper -Wall happiness the 'volatile' qualifier needs to be added there - but adding the qualifier has caused this crash. No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu. Thanks, Jan gdb/ChangeLog 2015-05-16 Jan Kratochvil <jan.kratochvil@redhat.com> compile: Fix crash on cv-qualified self-reference. * compile/compile-c-types.c (convert_struct_or_union): Apply build_qualified_type. (convert_type_basic): Do not apply build_qualified_type for TYPE_CODE_STRUCT and TYPE_CODE_UNION. gdb/testsuite/ChangeLog 2015-05-16 Jan Kratochvil <jan.kratochvil@redhat.com> compile: Fix crash on cv-qualified self-reference. * gdb.compile/compile.c (struct struct_type): Add volatile for selffield. * gdb.compile/compile.exp (compile code struct_object.selffield = &struct_object): Remove XFAIL.
Comments
Would anyone be in a better position to review this patch? It's one patch identified as a blocking issue before we create the GDB 7.10 branch. It looks reasonable to me, but I can only do a superficial review... Thanks! On Sat, May 16, 2015 at 03:25:55PM +0200, Jan Kratochvil wrote: > On Sat, 18 Apr 2015 19:28:43 +0200, Jan Kratochvil wrote: > Hi, > > with this modified testcase GDB would: > > compile code struct_object.selffield = &struct_object > ./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error) > > While the insert_type() assertion looks unclear trying to fix it one ends up > with either GCC crash > [gcc libcc1] build_qualified_type for self-referencing/incomplete types > https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html > c_incomplete_type_error() > or after fixing up the GCC type for proper error reporting one gets: > gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’ > which is the real culprit of this bug as explained in this patch. > > This patch is related to the XFAIL introduced by > [PATCH v3 5/9] compile: Use -Wall, not -w > https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html > as for proper -Wall happiness the 'volatile' qualifier needs to be added there > - but adding the qualifier has caused this crash. > > No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu. > > > Thanks, > Jan > gdb/ChangeLog > 2015-05-16 Jan Kratochvil <jan.kratochvil@redhat.com> > > compile: Fix crash on cv-qualified self-reference. > * compile/compile-c-types.c (convert_struct_or_union): Apply > build_qualified_type. > (convert_type_basic): Do not apply build_qualified_type for > TYPE_CODE_STRUCT and TYPE_CODE_UNION. > > gdb/testsuite/ChangeLog > 2015-05-16 Jan Kratochvil <jan.kratochvil@redhat.com> > > compile: Fix crash on cv-qualified self-reference. > * gdb.compile/compile.c (struct struct_type): Add volatile for > selffield. > * gdb.compile/compile.exp > (compile code struct_object.selffield = &struct_object): Remove XFAIL. > > diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c > index 2b521bc..420f61d 100644 > --- a/gdb/compile/compile-c-types.c > +++ b/gdb/compile/compile-c-types.c > @@ -166,9 +166,13 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type) > { > int i; > gcc_type result; > + int quals; > > /* First we create the resulting type and enter it into our hash > - table. This lets recursive types work. */ > + table. This lets recursive types work. We have to create gcc_type > + already with its qualifiers to prevent recursively calling > + build_qualified_type for unfinished TYPE as build_qualified_type > + creates a copy of the type, remaining unfinished forever. */ > if (TYPE_CODE (type) == TYPE_CODE_STRUCT) > result = C_CTX (context)->c_ops->build_record_type (C_CTX (context)); > else > @@ -176,6 +180,15 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type) > gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION); > result = C_CTX (context)->c_ops->build_union_type (C_CTX (context)); > } > + quals = 0; > + if (TYPE_CONST (type)) > + quals |= GCC_QUALIFIER_CONST; > + if (TYPE_VOLATILE (type)) > + quals |= GCC_QUALIFIER_VOLATILE; > + if (TYPE_RESTRICT (type)) > + quals |= GCC_QUALIFIER_RESTRICT; > + result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context), > + result, quals); > insert_type (context, type, result); > > for (i = 0; i < TYPE_NFIELDS (type); ++i) > @@ -329,10 +342,13 @@ static gcc_type > convert_type_basic (struct compile_c_instance *context, struct type *type) > { > /* If we are converting a qualified type, first convert the > - unqualified type and then apply the qualifiers. */ > + unqualified type and then apply the qualifiers, except for the > + types handling qualifiers on their own. */ > if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST > | TYPE_INSTANCE_FLAG_VOLATILE > - | TYPE_INSTANCE_FLAG_RESTRICT)) != 0) > + | TYPE_INSTANCE_FLAG_RESTRICT)) != 0 > + && (TYPE_CODE (type) != TYPE_CODE_STRUCT > + && TYPE_CODE (type) != TYPE_CODE_UNION)) > return convert_qualified (context, type); > > switch (TYPE_CODE (type)) > diff --git a/gdb/testsuite/gdb.compile/compile.c b/gdb/testsuite/gdb.compile/compile.c > index 3d5f20a..41ff087 100644 > --- a/gdb/testsuite/gdb.compile/compile.c > +++ b/gdb/testsuite/gdb.compile/compile.c > @@ -42,7 +42,7 @@ struct struct_type { > float floatfield; > double doublefield; > const union union_type *ptrfield; > - struct struct_type *selffield; > + volatile struct struct_type *selffield; > int arrayfield[5]; > _Complex double complexfield; > _Bool boolfield; > diff --git a/gdb/testsuite/gdb.compile/compile.exp b/gdb/testsuite/gdb.compile/compile.exp > index 07276bd..9668be8 100644 > --- a/gdb/testsuite/gdb.compile/compile.exp > +++ b/gdb/testsuite/gdb.compile/compile.exp > @@ -189,15 +189,7 @@ gdb_test "p localvar" " = 1" > # Test setting fields and also many different types. > # > > -set test "compile code struct_object.selffield = &struct_object" > -gdb_test_multiple $test $test { > - -re "^$test\r\n$gdb_prompt $" { > - pass "$test" > - } > - -re "gdb command line:1:25: warning: assignment discards 'volatile' qualifier from pointer target type \\\[-Wdiscarded-qualifiers\\\]\r\n$gdb_prompt $" { > - xfail "$test (PR compile/18202)" > - } > -} > +gdb_test_no_output "compile code struct_object.selffield = &struct_object" > gdb_test "print struct_object.selffield == &struct_object" " = 1" > > gdb_test_no_output "compile code struct_object.charfield = 1"
On 30/06/15 20:57, Joel Brobecker wrote: > Would anyone be in a better position to review this patch? > It's one patch identified as a blocking issue before we create > the GDB 7.10 branch. It looks reasonable to me, but I can only > do a superficial review... > > I think its fine, but no, not really. It's just Jan and I on the GDB side of the compile command. A maintainer will have to sign off on it. Cheers Phil
On 05/16/2015 02:25 PM, Jan Kratochvil wrote: > On Sat, 18 Apr 2015 19:28:43 +0200, Jan Kratochvil wrote: > Hi, > > with this modified testcase GDB would: > > compile code struct_object.selffield = &struct_object > ./compile/compile-c-types.c:83: internal-error: insert_type: Assertion `add == NULL || add->gcc_type == gcc_type' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) FAIL: gdb.compile/compile.exp: compile code struct_object.selffield = &struct_object (GDB internal error) > > While the insert_type() assertion looks unclear trying to fix it one ends up > with either GCC crash > [gcc libcc1] build_qualified_type for self-referencing/incomplete types > https://gcc.gnu.org/ml/gcc/2015-04/msg00108.html > c_incomplete_type_error() > or after fixing up the GCC type for proper error reporting one gets: > gdb command line:1:1: error: invalid use of incomplete typedef ‘sv’ > which is the real culprit of this bug as explained in this patch. > > This patch is related to the XFAIL introduced by > [PATCH v3 5/9] compile: Use -Wall, not -w > https://sourceware.org/ml/gdb-patches/2015-04/msg00429.html > as for proper -Wall happiness the 'volatile' qualifier needs to be added there > - but adding the qualifier has caused this crash. > > No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu. OK. Sorry for the delay. Thanks, Pedro Alves
Jan Kratochvil <jan.kratochvil@redhat.com> writes: > + quals = 0; > + if (TYPE_CONST (type)) > + quals |= GCC_QUALIFIER_CONST; > + if (TYPE_VOLATILE (type)) > + quals |= GCC_QUALIFIER_VOLATILE; > + if (TYPE_RESTRICT (type)) > + quals |= GCC_QUALIFIER_RESTRICT; > + result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context), > + result, quals); > insert_type (context, type, result); Can we use convert_qualified instead? I find code we added here is quite similar to convert_qualified. > > for (i = 0; i < TYPE_NFIELDS (type); ++i) > @@ -329,10 +342,13 @@ static gcc_type > convert_type_basic (struct compile_c_instance *context, struct type *type) > { > /* If we are converting a qualified type, first convert the > - unqualified type and then apply the qualifiers. */ > + unqualified type and then apply the qualifiers, except for the > + types handling qualifiers on their own. */ > if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST > | TYPE_INSTANCE_FLAG_VOLATILE > - | TYPE_INSTANCE_FLAG_RESTRICT)) != 0) > + | TYPE_INSTANCE_FLAG_RESTRICT)) != 0 > + && (TYPE_CODE (type) != TYPE_CODE_STRUCT > + && TYPE_CODE (type) != TYPE_CODE_UNION)) > return convert_qualified (context, type); It looks right to me, however, isn't cleaner to do in this way? /* Comments on why we do this first */ if (TYPE_CODE (type) == TYPE_CODE_STRUCT || TYPE_CODE (type) == TYPE_CODE_UNION) return convert_struct_or_union (context, type); /* If we are converting a qualified type, first convert the unqualified type and then apply the qualifiers. */ if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST | TYPE_INSTANCE_FLAG_VOLATILE | TYPE_INSTANCE_FLAG_RESTRICT)) != 0) return convert_qualified (context, type); switch (TYPE_CODE (type)) { /* Don't need to handle TYPE_CODE_STRUCT and TYPE_CODE_UNION here. */ } Otherwise, the patch looks good to me.
On Wed, 01 Jul 2015 13:21:32 +0200, Yao Qi wrote: > Jan Kratochvil <jan.kratochvil@redhat.com> writes: > > > + quals = 0; > > + if (TYPE_CONST (type)) > > + quals |= GCC_QUALIFIER_CONST; > > + if (TYPE_VOLATILE (type)) > > + quals |= GCC_QUALIFIER_VOLATILE; > > + if (TYPE_RESTRICT (type)) > > + quals |= GCC_QUALIFIER_RESTRICT; > > + result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context), > > + result, quals); > > insert_type (context, type, result); > > Can we use convert_qualified instead? I find code we added here is > quite similar to convert_qualified. Not directly to use convert_qualified() but convert_qualified() could be split in halves and one half could be used here, I agree. > > for (i = 0; i < TYPE_NFIELDS (type); ++i) > > @@ -329,10 +342,13 @@ static gcc_type > > convert_type_basic (struct compile_c_instance *context, struct type *type) > > { > > /* If we are converting a qualified type, first convert the > > - unqualified type and then apply the qualifiers. */ > > + unqualified type and then apply the qualifiers, except for the > > + types handling qualifiers on their own. */ > > if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST > > | TYPE_INSTANCE_FLAG_VOLATILE > > - | TYPE_INSTANCE_FLAG_RESTRICT)) != 0) > > + | TYPE_INSTANCE_FLAG_RESTRICT)) != 0 > > + && (TYPE_CODE (type) != TYPE_CODE_STRUCT > > + && TYPE_CODE (type) != TYPE_CODE_UNION)) > > return convert_qualified (context, type); > > It looks right to me, however, isn't cleaner to do in this way? > > /* Comments on why we do this first */ > if (TYPE_CODE (type) == TYPE_CODE_STRUCT > || TYPE_CODE (type) == TYPE_CODE_UNION) > return convert_struct_or_union (context, type); > > /* If we are converting a qualified type, first convert the > unqualified type and then apply the qualifiers. */ > if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST > | TYPE_INSTANCE_FLAG_VOLATILE > | TYPE_INSTANCE_FLAG_RESTRICT)) != 0) > return convert_qualified (context, type); > > switch (TYPE_CODE (type)) > { > /* Don't need to handle TYPE_CODE_STRUCT and TYPE_CODE_UNION > here. */ > } I can change it that way but when you ask "isn't cleaner" then no, I think your hack is even a bit more ugly than my ugly hack. There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))' and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though). > Otherwise, the patch looks good to me. OK, thanks. Just it causes a regression with latest GCC now as I have asked Alexandre Oliva off-list how it really should be fixed. Jan
On 07/01/2015 02:24 PM, Jan Kratochvil wrote: > I can change it that way but when you ask "isn't cleaner" then no, I think > your hack is even a bit more ugly than my ugly hack. > > There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))' > and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by > TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though). What would be the method name? There's nothing preventing adding a new type_FOO function that takes a type pointer as parameter and hides the TYPE_CODE checks inside. From the caller's perspective, it'll be the same. Once we get to C++ and if we consider objectifying type, then converting that function to a method will be trivial. Thanks, Pedro Alves
Jan Kratochvil <jan.kratochvil@redhat.com> writes: > I can change it that way but when you ask "isn't cleaner" then no, I think > your hack is even a bit more ugly than my ugly hack. > OK, that is sort of personal preference, that is fine to me to leave it as is. > There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))' > and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by > TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though). If we change to virtual methods, we don't need "switch" at all. Each type can have its virtual method to handle this separately.
On Wed, 01 Jul 2015 15:54:21 +0200, Pedro Alves wrote: > On 07/01/2015 02:24 PM, Jan Kratochvil wrote: > > > I can change it that way but when you ask "isn't cleaner" then no, I think > > your hack is even a bit more ugly than my ugly hack. > > > > There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))' > > and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by > > TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though). > > What would be the method name? class Type { protected: virtual GccType convert_unqualified ()=0; public: explicit virtual GccType operator() { if (instance_flags==0) return convert_unqualified(); ... } }; class StructType:public Type { protected: virtual GccType convert_unqualified () { assert(0) } public: explicit virtual GccType operator() override { ... } }; class IntegerType:public Type { protected: virtual GccType convert_unqualified () { assert(instance_flags==0); ... } }; Althoughth qualifications could be possibly also subclassed which would look differently again. My point was that the current code does not make much sense to clean up as it cannot be clean in C. > There's nothing preventing adding a new type_FOO function that takes a type > pointer as parameter and hides the TYPE_CODE checks inside. From the > caller's perspective, it'll be the same. Once we get to C++ and if we > consider objectifying type, then converting that function to a method will > be trivial. Do you mean simulation of C++ virtual method table by a struct of pointers, like in other cases in GDB? Jan
On 07/01/2015 03:10 PM, Jan Kratochvil wrote: > On Wed, 01 Jul 2015 15:54:21 +0200, Pedro Alves wrote: >> > On 07/01/2015 02:24 PM, Jan Kratochvil wrote: >> > >>> > > I can change it that way but when you ask "isn't cleaner" then no, I think >>> > > your hack is even a bit more ugly than my ugly hack. >>> > > >>> > > There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))' >>> > > and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by >>> > > TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though). >> > >> > What would be the method name? > class Type { > protected: > virtual GccType convert_unqualified ()=0; > public: > explicit virtual GccType operator() { > if (instance_flags==0) return convert_unqualified(); > ... > } > }; > > class StructType:public Type { > protected: > virtual GccType convert_unqualified () { assert(0) } > public: > explicit virtual GccType operator() override { ... } > }; > > class IntegerType:public Type { > protected: > virtual GccType convert_unqualified () { assert(instance_flags==0); ... } > }; > Well, I'd say that having the core GDB Type be aware of GccType directly would be a misdesign, not a feature. > Althoughth qualifications could be possibly also subclassed which would look > differently again. Yes, the "possibly" here is the crux. Subclassing isn't always the best choice. There are trade offs. >> > There's nothing preventing adding a new type_FOO function that takes a type >> > pointer as parameter and hides the TYPE_CODE checks inside. From the >> > caller's perspective, it'll be the same. Once we get to C++ and if we >> > consider objectifying type, then converting that function to a method will >> > be trivial. > Do you mean simulation of C++ virtual method table by a struct of pointers, > like in other cases in GDB? > No. I meant a straightforward conversion of your C++ methods to C functions implemented in terms of switch/TYPE_CODE. IIUC, in your C++ class tree you'd do: gcctype = (GccType) type; So a trivial 1-1 conversion or your code would be: // Type::convert_unqualified () // StructType::convert_unqualified () gcc_type type_convert_unqualified (struct type *) { switch (TYPE_CODE (type)) { case TYPE_CODE_STRUCT: assert(0); default: ... } } // Type::GccType operator() gcc_type type_as_gcc_type (struct type *type) { if (TYPE_INSTANCE_FLAGS (instance_flags) == 0) return type_convert_unqualified (type); ... } Then the caller in question would use: gcctype = type_as_gcc_type (type); I'm very much looking forward to C++ as well, but switch/case vs virtual methods here is really more about syntax sugar than design. You can easily end up with a broken class inheritance tree just as well. There's a lot more to it beyond design than language. Thanks, Pedro Alves
On Wed, 01 Jul 2015 16:59:37 +0200, Pedro Alves wrote: > Well, I'd say that having the core GDB Type be aware of GccType > directly would be a misdesign, not a feature. You are right, it would be reversed, anyway not the insance crap of TYPE_CODE_*. > So a trivial 1-1 conversion or your code would be: I find it complicates the code even more without C++. > I'm very much looking forward to C++ as well, So why that does not happen? You have added a disadvantage of more requirements for the code compliance with the new exception macros but there is still no advantage from that. > but switch/case vs > virtual methods here is really more about syntax sugar than design. C++ may look so to someone but is not just a syntax sugar. Jan
On 07/01/2015 04:12 PM, Jan Kratochvil wrote: >> I'm very much looking forward to C++ as well, > > So why that does not happen? For myself, just lack of time ATM. I've called out for help before. Yao helped a bit. The current status is in the wiki page. https://sourceware.org/gdb/wiki/cxx-conversion Basically the next step is tacking patches from: git@github.com:palves/gdb.git palves/cxx-conversion-attempt-part-2-no-fpermissive and polishing/submitting them upstream. > You have added a disadvantage of more > requirements for the code compliance with the new exception macros but there > is still no advantage from that. Why would you say that... I don't see what disadvantage you talk about. Thanks, Pedro Alves
On Wed, 01 Jul 2015 17:24:38 +0200, Pedro Alves wrote: > For myself, just lack of time ATM. I've called out for help before. > Yao helped a bit. The current status is in the wiki page. > > https://sourceware.org/gdb/wiki/cxx-conversion > > Basically the next step is tacking patches from: > > git@github.com:palves/gdb.git palves/cxx-conversion-attempt-part-2-no-fpermissive > > and polishing/submitting them upstream. OK, thanks for the status updated. > > You have added a disadvantage of more > > requirements for the code compliance with the new exception macros but there > > is still no advantage from that. > > Why would you say that... I don't see what disadvantage you talk about. From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches. When GDB codebase finally gets compiled by clang++ I agree it will be an improvement. I see there are no real roadblocks, just the time availability. Thanks, Jan
On 07/01/2015 04:29 PM, Jan Kratochvil wrote: > From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches. > When GDB codebase finally gets compiled by clang++ I agree it will be an > improvement. Seriously, that's a depressing and demotivating comment. You want it done, but you also want it not done. Thanks, Pedro Alves
On Wed, 01 Jul 2015 17:35:10 +0200, Pedro Alves wrote: > On 07/01/2015 04:29 PM, Jan Kratochvil wrote: > > From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches. > > When GDB codebase finally gets compiled by clang++ I agree it will be an > > improvement. > > Seriously, that's a depressing and demotivating comment. > You want it done, but you also want it not done. Do you mean the first sentence or the second sentence? I agree the first sentence is demotivating but I feel the same spending a lot of time rebasing patches again and again on changes that end up half-way or that are questionable whether they really cleaned up the code. Although specifically for this TRY_CATCH->TRY case I had fortunately only two such cases. For the second sentence I hope everybody - and at least Google - agrees, that G++ is not usable for C++ development as in many cases particularly involving templates the error messages only say something is wrong in the source, without giving the invalid source line number. I was bisecting the source before I found clang++ can just report the invalid code location. I have filed many diagnostics PRs for GCC but almost none of them are fixed yet. G++ is great to get a bit better performing final code after all the compilation errors are resolved by clang++. Jan
On 07/01/2015 05:07 PM, Jan Kratochvil wrote: > On Wed, 01 Jul 2015 17:35:10 +0200, Pedro Alves wrote: >> On 07/01/2015 04:29 PM, Jan Kratochvil wrote: >>> From 1 mandatory macro 3 mandatory macros and breaking all 3rd party patches. >>> When GDB codebase finally gets compiled by clang++ I agree it will be an >>> improvement. >> >> Seriously, that's a depressing and demotivating comment. >> You want it done, but you also want it not done. > > Do you mean the first sentence or the second sentence? > The whole of it. If we had converted to pure C++ in one go, you'd have had to revolve the same conflicts too. And the conflicts resulting from the final conversion from TRY/CATCH to native try/catch will be super trivial to resolve, as that won't move code around. > I agree the first sentence is demotivating but I feel the same spending a lot > of time rebasing patches again and again on changes that end up half-way or > that are questionable whether they really cleaned up the code. Although > specifically for this TRY_CATCH->TRY case I had fortunately only two such > cases. And still, you complain. This is truly pointless. Sorry, I'm out of here. Thanks, Pedro Alves
On Wed, 01 Jul 2015 15:24:06 +0200, Jan Kratochvil wrote: > Just it causes a regression with latest GCC now as I have asked > Alexandre Oliva off-list how it really should be fixed. I have removed requirement of this fix for the branching as there is currently no valid fix at hand and after all the 'compile' feature probably should not be a release blocker. Still hopefully it gets fixed before the 7.10 release. Jan
diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c index 2b521bc..420f61d 100644 --- a/gdb/compile/compile-c-types.c +++ b/gdb/compile/compile-c-types.c @@ -166,9 +166,13 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type) { int i; gcc_type result; + int quals; /* First we create the resulting type and enter it into our hash - table. This lets recursive types work. */ + table. This lets recursive types work. We have to create gcc_type + already with its qualifiers to prevent recursively calling + build_qualified_type for unfinished TYPE as build_qualified_type + creates a copy of the type, remaining unfinished forever. */ if (TYPE_CODE (type) == TYPE_CODE_STRUCT) result = C_CTX (context)->c_ops->build_record_type (C_CTX (context)); else @@ -176,6 +180,15 @@ convert_struct_or_union (struct compile_c_instance *context, struct type *type) gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION); result = C_CTX (context)->c_ops->build_union_type (C_CTX (context)); } + quals = 0; + if (TYPE_CONST (type)) + quals |= GCC_QUALIFIER_CONST; + if (TYPE_VOLATILE (type)) + quals |= GCC_QUALIFIER_VOLATILE; + if (TYPE_RESTRICT (type)) + quals |= GCC_QUALIFIER_RESTRICT; + result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context), + result, quals); insert_type (context, type, result); for (i = 0; i < TYPE_NFIELDS (type); ++i) @@ -329,10 +342,13 @@ static gcc_type convert_type_basic (struct compile_c_instance *context, struct type *type) { /* If we are converting a qualified type, first convert the - unqualified type and then apply the qualifiers. */ + unqualified type and then apply the qualifiers, except for the + types handling qualifiers on their own. */ if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST | TYPE_INSTANCE_FLAG_VOLATILE - | TYPE_INSTANCE_FLAG_RESTRICT)) != 0) + | TYPE_INSTANCE_FLAG_RESTRICT)) != 0 + && (TYPE_CODE (type) != TYPE_CODE_STRUCT + && TYPE_CODE (type) != TYPE_CODE_UNION)) return convert_qualified (context, type); switch (TYPE_CODE (type)) diff --git a/gdb/testsuite/gdb.compile/compile.c b/gdb/testsuite/gdb.compile/compile.c index 3d5f20a..41ff087 100644 --- a/gdb/testsuite/gdb.compile/compile.c +++ b/gdb/testsuite/gdb.compile/compile.c @@ -42,7 +42,7 @@ struct struct_type { float floatfield; double doublefield; const union union_type *ptrfield; - struct struct_type *selffield; + volatile struct struct_type *selffield; int arrayfield[5]; _Complex double complexfield; _Bool boolfield; diff --git a/gdb/testsuite/gdb.compile/compile.exp b/gdb/testsuite/gdb.compile/compile.exp index 07276bd..9668be8 100644 --- a/gdb/testsuite/gdb.compile/compile.exp +++ b/gdb/testsuite/gdb.compile/compile.exp @@ -189,15 +189,7 @@ gdb_test "p localvar" " = 1" # Test setting fields and also many different types. # -set test "compile code struct_object.selffield = &struct_object" -gdb_test_multiple $test $test { - -re "^$test\r\n$gdb_prompt $" { - pass "$test" - } - -re "gdb command line:1:25: warning: assignment discards 'volatile' qualifier from pointer target type \\\[-Wdiscarded-qualifiers\\\]\r\n$gdb_prompt $" { - xfail "$test (PR compile/18202)" - } -} +gdb_test_no_output "compile code struct_object.selffield = &struct_object" gdb_test "print struct_object.selffield == &struct_object" " = 1" gdb_test_no_output "compile code struct_object.charfield = 1"