diff mbox

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

Message ID m3iofdjpav.fsf@sspiff.org
State New
Headers show

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

Pierre Muller Feb. 8, 2015, 2:48 p.m. UTC | #1
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. UTC | #2
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 mbox

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: