Message ID | m3iofdjpav.fsf@sspiff.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 28134 invoked by alias); 7 Feb 2015 23:06: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 28122 invoked by uid 89); 7 Feb 2015 23:06:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pd0-f175.google.com Received: from mail-pd0-f175.google.com (HELO mail-pd0-f175.google.com) (209.85.192.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sat, 07 Feb 2015 23:06:00 +0000 Received: by pdbnh10 with SMTP id nh10so19936097pdb.12 for <gdb-patches@sourceware.org>; Sat, 07 Feb 2015 15:05:59 -0800 (PST) X-Received: by 10.66.102.2 with SMTP id fk2mr16511658pab.149.1423350359122; Sat, 07 Feb 2015 15:05:59 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id qc8sm11959057pdb.86.2015.02.07.15.05.57 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 07 Feb 2015 15:05:58 -0800 (PST) From: Doug Evans <xdje42@gmail.com> To: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> Cc: <gdb-patches@sourceware.org> Subject: Re: [PATCH 4/5] Remove struct main_type.vptr_{fieldno, basetype}: TYPE_SPECIFIC_SELF_TYPE References: <m3twze8kxc.fsf@sspiff.org> <54d3aa6b.a23d460a.0d37.0908SMTPIN_ADDED_BROKEN@mx.google.com> Date: Sat, 07 Feb 2015 15:05:12 -0800 In-Reply-To: <54d3aa6b.a23d460a.0d37.0908SMTPIN_ADDED_BROKEN@mx.google.com> (Pierre Muller's message of "Thu, 5 Feb 2015 18:37:00 +0100") Message-ID: <m3iofdjpav.fsf@sspiff.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes |
Commit Message
Doug Evans
Feb. 7, 2015, 11:05 p.m. UTC
"Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> writes: > Hi all, > > >> -----Message d'origine----- >> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches- >> owner@sourceware.org] De la part de Doug Evans >> Envoyé : lundi 26 janvier 2015 01:07 >> À : gdb-patches@sourceware.org; gaius@glam.ac.uk >> Objet : [PATCH 4/5] Remove struct main_type.vptr_{fieldno,basetype}: >> TYPE_SPECIFIC_SELF_TYPE >> >> Hi. >> >> This patch moves TYPE_SELF_TYPE into new field type_specific.self_type >> for MEMBERPTR,METHODPTR types, and into type_specific.func_stuff >> for METHODs, and then updates everything to use that. >> TYPE_CODE_METHOD could share some things with TYPE_CODE_FUNC >> (e.g. TYPE_NO_RETURN) and it seemed simplest to keep them together. >> >> Moving TYPE_SELF_TYPE into type_specific.func_stuff for >> TYPE_CODE_METHOD >> is also nice because when we allocate space for function types we >> assume >> they're TYPE_CODE_FUNCs. If TYPE_CODE_METHODs don't need or use that >> space then that space would be wasted, and cleaning that up would >> involve >> more invasive changes. >> >> In order to catch errant uses I've added accessor functions >> that do some checking. >> >> One can no longer assign to TYPE_SELF_TYPE like this: >> >> TYPE_SELF_TYPE (foo) = bar; >> >> One instead has to do: >> >> set_type_self_type (foo, bar); >> >> But I've left reading of the type to the macro: >> >> bar = TYPE_SELF_TYPE (foo); >> >> I could add SET_TYPE_SELF_TYPE as a wrapper on set_type_self_type >> if you prefer that. >> >> In order to discourage bypassing the TYPE_SELF_TYPE macro >> I've named the underlying function that implements it > .... >> * stabsread.c (read_member_functions): Mark methods with >> TYPE_CODE_METHOD, not TYPE_CODE_FUNC. Update setting of >> TYPE_SELF_TYPE. > ..... >> diff --git a/gdb/stabsread.c b/gdb/stabsread.c >> index 1f46f75..423c442 100644 >> --- a/gdb/stabsread.c >> +++ b/gdb/stabsread.c >> @@ -2376,14 +2376,21 @@ read_member_functions (struct field_info *fip, >> char **pp, struct type *type, >> p++; >> } >> >> - /* If this is just a stub, then we don't have the real name >> here. */ >> + /* These are methods, not functions. */ >> + if (TYPE_CODE (new_sublist->fn_field.type) == TYPE_CODE_FUNC) >> + TYPE_CODE (new_sublist->fn_field.type) = TYPE_CODE_METHOD; >> + else >> + gdb_assert (TYPE_CODE (new_sublist->fn_field.type) >> + == TYPE_CODE_METHOD); >> >> + /* If this is just a stub, then we don't have the real name >> here. */ >> if (TYPE_STUB (new_sublist->fn_field.type)) >> { >> if (!TYPE_SELF_TYPE (new_sublist->fn_field.type)) > I suspect this is the part that generates the failure > I saw when trying to test my pascal patch that used stabs debugging > information. > > internal_type_self_type generates an internal error > it does not simply return NULL... > >> - TYPE_SELF_TYPE (new_sublist->fn_field.type) = type; >> + set_type_self_type (new_sublist->fn_field.type, type); >> new_sublist->fn_field.is_stub = 1; >> } >> + >> new_sublist->fn_field.physname = savestring (*pp, p - *pp); >> *pp = p + 1; > > The patch below removes the internal error, > but I am not sure it is the correct fix... > Maybe set_type_self_type should be called unconditionally. > > Likewise, the line: > valops.c:2547: gdb_assert (TYPE_SELF_TYPE (fns_ptr[0].type) != NULL); > is not compatible with your new internal_type_self_type as this > new function never returns NULL.... > > > Pierre Muller > > $ git diff > diff --git a/gdb/stabsread.c b/gdb/stabsread.c > index 2a160c5..392fdb2 100644 > --- a/gdb/stabsread.c > +++ b/gdb/stabsread.c > @@ -2386,7 +2386,7 @@ read_member_functions (struct field_info *fip, char > **pp, struct type *type, > /* If this is just a stub, then we don't have the real name here. > */ > if (TYPE_STUB (new_sublist->fn_field.type)) > { > - if (!TYPE_SELF_TYPE (new_sublist->fn_field.type)) > + if (TYPE_SPECIFIC_FIELD (new_sublist->fn_field.type) == > TYPE_SPECIFIC_NONE) > set_type_self_type (new_sublist->fn_field.type, type); > new_sublist->fn_field.is_stub = 1; > } Thanks for the testcase! I found the culprit. I added some logging to allocate_stub_method, and running the entire testsuite with stabs (-gstabs+) does not exercise this function. Bleah. I don't know stabs well enough to want to spend time coming up with a testcase. Is it possible for you to write one from your test? Here's a patch. You're patch is on the right track, TYPE_SPECIFIC_FIELD can legitimately be TYPE_SPECIFIC_NONE if the field hasn't been initialized yet. I like the patch below as it's more general. 2015-02-07 Doug Evans <xdje42@gmail.com> * gdbtypes.c (internal_type_self_type): If TYPE_SPECIFIC_FIELD hasn't been initialized yet, return NULL.
Comments
Hi, > -----Message d'origine----- > De : gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] De la part de Doug Evans > Envoyé : dimanche 8 février 2015 00:05 > À : Pierre Muller > Cc : gdb-patches@sourceware.org > Objet : Re: [PATCH 4/5] Remove struct > main_type.vptr_{fieldno,basetype}: TYPE_SPECIFIC_SELF_TYPE > > "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> writes: > > Hi all, > > > > > >> -----Message d'origine----- > >> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches- > >> owner@sourceware.org] De la part de Doug Evans > >> Envoyé : lundi 26 janvier 2015 01:07 > >> À : gdb-patches@sourceware.org; gaius@glam.ac.uk > >> Objet : [PATCH 4/5] Remove struct main_type.vptr_{fieldno,basetype}: > >> TYPE_SPECIFIC_SELF_TYPE > >> > >> Hi. > >> > >> This patch moves TYPE_SELF_TYPE into new field > type_specific.self_type > >> for MEMBERPTR,METHODPTR types, and into type_specific.func_stuff > >> for METHODs, and then updates everything to use that. > >> TYPE_CODE_METHOD could share some things with TYPE_CODE_FUNC > >> (e.g. TYPE_NO_RETURN) and it seemed simplest to keep them together. > >> > >> Moving TYPE_SELF_TYPE into type_specific.func_stuff for > >> TYPE_CODE_METHOD > >> is also nice because when we allocate space for function types we > >> assume > >> they're TYPE_CODE_FUNCs. If TYPE_CODE_METHODs don't need or use that > >> space then that space would be wasted, and cleaning that up would > >> involve > >> more invasive changes. > >> > >> In order to catch errant uses I've added accessor functions > >> that do some checking. > >> > >> One can no longer assign to TYPE_SELF_TYPE like this: > >> > >> TYPE_SELF_TYPE (foo) = bar; > >> > >> One instead has to do: > >> > >> set_type_self_type (foo, bar); > >> > >> But I've left reading of the type to the macro: > >> > >> bar = TYPE_SELF_TYPE (foo); > >> > >> I could add SET_TYPE_SELF_TYPE as a wrapper on set_type_self_type > >> if you prefer that. > >> > >> In order to discourage bypassing the TYPE_SELF_TYPE macro > >> I've named the underlying function that implements it > > .... > >> * stabsread.c (read_member_functions): Mark methods with > >> TYPE_CODE_METHOD, not TYPE_CODE_FUNC. Update setting of > >> TYPE_SELF_TYPE. > > ..... > >> diff --git a/gdb/stabsread.c b/gdb/stabsread.c > >> index 1f46f75..423c442 100644 > >> --- a/gdb/stabsread.c > >> +++ b/gdb/stabsread.c > >> @@ -2376,14 +2376,21 @@ read_member_functions (struct field_info > *fip, > >> char **pp, struct type *type, > >> p++; > >> } > >> > >> - /* If this is just a stub, then we don't have the real name > >> here. */ > >> + /* These are methods, not functions. */ > >> + if (TYPE_CODE (new_sublist->fn_field.type) == TYPE_CODE_FUNC) > >> + TYPE_CODE (new_sublist->fn_field.type) = TYPE_CODE_METHOD; > >> + else > >> + gdb_assert (TYPE_CODE (new_sublist->fn_field.type) > >> + == TYPE_CODE_METHOD); > >> > >> + /* If this is just a stub, then we don't have the real name > >> here. */ > >> if (TYPE_STUB (new_sublist->fn_field.type)) > >> { > >> if (!TYPE_SELF_TYPE (new_sublist->fn_field.type)) > > I suspect this is the part that generates the failure > > I saw when trying to test my pascal patch that used stabs debugging > > information. > > > > internal_type_self_type generates an internal error > > it does not simply return NULL... > > > >> - TYPE_SELF_TYPE (new_sublist->fn_field.type) = type; > >> + set_type_self_type (new_sublist->fn_field.type, type); > >> new_sublist->fn_field.is_stub = 1; > >> } > >> + > >> new_sublist->fn_field.physname = savestring (*pp, p - *pp); > >> *pp = p + 1; > > > > The patch below removes the internal error, > > but I am not sure it is the correct fix... > > Maybe set_type_self_type should be called unconditionally. > > > > Likewise, the line: > > valops.c:2547: gdb_assert (TYPE_SELF_TYPE (fns_ptr[0].type) != > NULL); > > is not compatible with your new internal_type_self_type as this > > new function never returns NULL.... > > > > > > Pierre Muller > > > > $ git diff > > diff --git a/gdb/stabsread.c b/gdb/stabsread.c > > index 2a160c5..392fdb2 100644 > > --- a/gdb/stabsread.c > > +++ b/gdb/stabsread.c > > @@ -2386,7 +2386,7 @@ read_member_functions (struct field_info *fip, > char > > **pp, struct type *type, > > /* If this is just a stub, then we don't have the real name > here. > > */ > > if (TYPE_STUB (new_sublist->fn_field.type)) > > { > > - if (!TYPE_SELF_TYPE (new_sublist->fn_field.type)) > > + if (TYPE_SPECIFIC_FIELD (new_sublist->fn_field.type) > == > > TYPE_SPECIFIC_NONE) > > set_type_self_type (new_sublist->fn_field.type, > type); > > new_sublist->fn_field.is_stub = 1; > > } > > Thanks for the testcase! I found the culprit. > > I added some logging to allocate_stub_method, and running the entire > testsuite with stabs (-gstabs+) does not exercise this function. Bleah. > > I don't know stabs well enough to want to spend time coming up with > a testcase. Is it possible for you to write one from your test? Yes, I also tried with g++ and never got to the same internal_error. I have a minimal pascal source, and extracted from it a subset of the stabs indormation. I have put both at the end of this email. > Here's a patch. > You're patch is on the right track, TYPE_SPECIFIC_FIELD can > legitimately be TYPE_SPECIFIC_NONE if the field hasn't been initialized > yet. I like the patch below as it's more general. I agree with you that this patch seems much secure, as it allows to use TYPE_SELF_TYPE as a test as before. > 2015-02-07 Doug Evans <xdje42@gmail.com> > > * gdbtypes.c (internal_type_self_type): If TYPE_SPECIFIC_FIELD > hasn't > been initialized yet, return NULL. > > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c > index 140fc6f..44483d4 100644 > --- a/gdb/gdbtypes.c > +++ b/gdb/gdbtypes.c > @@ -1198,9 +1198,13 @@ internal_type_self_type (struct type *type) > { > case TYPE_CODE_METHODPTR: > case TYPE_CODE_MEMBERPTR: > + if (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_NONE) > + return NULL; > gdb_assert (TYPE_SPECIFIC_FIELD (type) == > TYPE_SPECIFIC_SELF_TYPE); > return TYPE_MAIN_TYPE (type)->type_specific.self_type; > case TYPE_CODE_METHOD: > + if (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_NONE) > + return NULL; > gdb_assert (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_FUNC); > return TYPE_MAIN_TYE (type)->type_specific.func_stuff- > >self_type; > default: Simple pascal source: muller@gcc45:~/pas/trunk/fpcsrc/ide$ cat test-obj.pp type tobj = object constructor create; end; constructor tobj.create; begin end; var t : tobj; begin t.create; end. Compiled with free pascal using ppc386 -al -gs test-obj.pp the option -al generates an intermediate assembler file called test-obj.s that is converted into an object file using GNU assembler for i386 cpu. I reduced the generated file, keeping only the global stabs information. muller@gcc45:~/pas/trunk/fpcsrc/ide$ cat test-min.s .file "test-obj.pp" # Begin asmlist al_begin .section .text .globl DEBUGSTART_P$PROGRAM .type DEBUGSTART_P$PROGRAM,@object DEBUGSTART_P$PROGRAM: .stabs "/home/muller/pas/trunk/fpcsrc/ide/",100,0,0,.Lf3 .stabs "test-obj.pp",100,0,0,.Lf3 .Lf3: # End asmlist al_begin # Begin asmlist al_stabs .section .data .globl DEBUGINFO_P$PROGRAM .type DEBUGINFO_P$PROGRAM,@object DEBUGINFO_P$PROGRAM: # Defs - Begin unit SYSTEM has index 1 .stabs "void:t2=2",128,0,0,0 .stabs "LONGBOOL:t3=-23;",128,0,0,0 .stabs "POINTER:t4=*2",128,0,0,0 # Defs - End unit SYSTEM has index 1 # Defs - Begin unit FPINTRES has index 2 # Defs - End unit FPINTRES has index 2 # Defs - Begin unit SI_PRC has index 2 # Defs - End unit SI_PRC has index 2 # Defs - Begin Staticsymtable .stabs "LONGINT:t5=r5;-2147483648;2147483647;",128,0,0,0 .stabs "CHAR:t6=-20;",128,0,0,0 .stabs "BYTE:t7=r7;0;255;",128,0,0,0 .stabs "SHORTSTRING:Tt8=s256length:7,0,8;st:ar7;1;255;6,8,2040;;",128,0,0,0 .stabs ":t9=*8",128,0,0,0 .stabs ":t10=ar5;0;0;4",128,0,0,0 .stabs "__vtbl_ptr_type:Tt11=s2;",128,0,0,0 .stabs "pvmt:t12=*11",128,0,0,0 .stabs "vtblarray:t13=ar5;0;1;12",128,0,0,0 .stabs ":Tt1=s4$vf1:13,0;CREATE::14=##3;:__ct__4TOBJ7POINTER;2A.;;~%1;",128,0,0,0 .stabs "vmt_PROGRAMTOBJ:S11",38,0,0,VMT_P$PROGRAM_TOBJ # Defs - End Staticsymtable # Syms - Begin Staticsymtable .stabs "TOBJ:Tt1",128,0,3,0 .stabs "T:S1",40,0,13,U_P$PROGRAM_T # Syms - End Staticsymtable # End asmlist al_stabs # Begin asmlist al_procedures It seems that the trouble is really coming from .stabs ":Tt1=s4$vf1:13,0;CREATE::14=##3;:__ct__4TOBJ7POINTER;2A.;;~%1;",128,0,0,0 especially the first "implicit parameter of the constructor called create, because TOBJ type is not yet known (and thus is a stub). The named type TOVJ is defined later in: .stabs "TOBJ:Tt1",128,0,3,0 In a g++ equivalent source, the parameters of the functions are given via dbx types so that no stub type is generated, and thus also no internal error. I do believe that the stabs generated by Free Pascal is correct and should still be accepted. Anyhow, your patch does solve the issue and should be committed. Pierre Muller
On Sun, Feb 8, 2015 at 6:48 AM, Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> wrote: >> Here's a patch. >> You're patch is on the right track, TYPE_SPECIFIC_FIELD can >> legitimately be TYPE_SPECIFIC_NONE if the field hasn't been initialized >> yet. I like the patch below as it's more general. > > I agree with you that this patch seems > much secure, as it allows to use TYPE_SELF_TYPE as > a test as before. > > >> 2015-02-07 Doug Evans <xdje42@gmail.com> >> >> * gdbtypes.c (internal_type_self_type): If TYPE_SPECIFIC_FIELD >> hasn't >> been initialized yet, return NULL. Committed.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index 140fc6f..44483d4 100644 --- a/gdb/gdbtypes.c +++ b/gdb/gdbtypes.c @@ -1198,9 +1198,13 @@ internal_type_self_type (struct type *type) { case TYPE_CODE_METHODPTR: case TYPE_CODE_MEMBERPTR: + if (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_NONE) + return NULL; gdb_assert (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_SELF_TYPE); return TYPE_MAIN_TYPE (type)->type_specific.self_type; case TYPE_CODE_METHOD: + if (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_NONE) + return NULL; gdb_assert (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_FUNC); return TYPE_MAIN_TYPE (type)->type_specific.func_stuff->self_type; default: