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

[Doug Evans <xdje42@gmail.com>]

"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.

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: