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

Hi.
Is it easy/possible to send me a repro?
Even just the binary that triggers the problem would help.
It would be good to verify what's going on here.
If internal_type_self_type got an internal error it must be this one:

      gdb_assert (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_FUNC);

But it's hard to see how that would fail given the earlier check
for TYPE_CODE_FUNC:

	  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);

I'm not sure how we'd get here with anything other than TYPE_CODE_FUNC
but for robustness I added the assert for TYPE_CODE_METHOD.
And if we've got a TYPE_CODE_FUNC then AFAICT that means we've called
make_function_type which sets TYPE_SPECIFIC_FIELD (type) = TYPE_SPECIFIC_FUNC.
Ergo IWBN to reproduce this and see the details.

I'm not saying there isn't a problem of course,
just that I may need some help understanding it.

Also, why couldn't internal_type_self_type 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;
>             }