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

["Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>]

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