Patchwork [4/5] Remove struct main_type.vptr_{fieldno,basetype}: TYPE_SPECIFIC_SELF_TYPE

login
register
mail settings
Submitter Doug Evans
Date Feb. 7, 2015, 11:05 p.m.
Message ID <m3iofdjpav.fsf@sspiff.org>
Download mbox | patch
Permalink /patch/4965/
State New
Headers show

Comments

Doug Evans - Feb. 7, 2015, 11:05 p.m.
"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.
Pierre Muller - Feb. 8, 2015, 2:48 p.m.
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
Doug Evans - Feb. 11, 2015, 6:28 a.m.
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.

Patch

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: