Re: [PATCH PR gdb/20057] Internal error on trying to set {char[]}$pc="string"

[Wei-min Pan <weimin.pan@oracle.com>]

On 1/31/2018 11:59 PM, Joel Brobecker wrote:
>>> Unfortunately, I only have vague answers for you. I know it's not
>>> as satisfactory as a firm one, but I haven't had time to investigate
>>> further.
>>>
>>> My feeling is that it's (intuitively) a bad idea to start mixing
>>> and matching the ownership type for a give type chain. It just
>>> muddies the waters, and makes memory management more complex.
>> Given there are functions such as arch_integer_type(),
>> arch_character_type(),
>> and arch_float_type() that can be used to add types to an arch, it doesn't
>> seem terribly wrong to add a type which is not associated with any objfile
>> to gdbarch? Also a type can actually exist in both an arch and an objfile.
> I am not sure we understand each other. For me, what seems wrong
> is the fact that we have an array type where part of the type is
> objfile-owned, and part of it arch-owned.
>
> Creating arch-owned type is fine, as long as the entire type is
> arch-owned.

Sorry, I just don't see how it's possible to have an array type where 
part of the type
is objfile-owned and part of it arch-owned. When an array type is 
copied, the space
allocation depends on whether or not the element type is defined in the 
program.
If it is defined, space for the index type, range type, and array type 
itself are all
allocated from that object file. Otherwise, they are all allocated from 
the arch.

>
>>> Parallel to that, there is another obstacle if you want to enhance
>>> copy_type to handle arch-owned types, as the current implementation
>>> explicitly assumes that the type is objfile-owned, and therefore
>>> references its objfile's obstack:
>>>
>>>     if (TYPE_DYN_PROP_LIST (type) != NULL)
>>>       TYPE_DYN_PROP_LIST (new_type)
>>>         = copy_dynamic_prop_list (&TYPE_OBJFILE (type) -> objfile_obstack,
>>>                                   TYPE_DYN_PROP_LIST (type));
>> Good point. The following statement
>>
>>    if (TYPE_DYN_PROP_LIST (type) != NULL)
>>
>> needs to be changed to:
>>
>>    if (TYPE_DYN_PROP_LIST (type) != NULL && TYPE_OBJFILE_OWNED(type))
> That would be wrong, because the resulting type would be missing
> that dynamic property list, which means the resulting type would
> be a complete copy of the original type. It's not so simple!
>
>> Your fix in lookup_array_range_type() takes care the case where
>> "element_type" was owned by an objfile but still creates an arch-owned
>> index type if it was not.
> That is correct, and it is not a problem as long as the entire type
> is consistent.
>
>> Here is the test case that comes with the PR:
>>
>> % cat x.c
>> char p[] = "hello";
>>
>> int main()
>> {
>>    return ((int)(p[0]));
>> }
>>
>> Please note that the test case declares base type "char" which has an
>> associated objfile and is picked up by lookup_symbol_aux() when
>> command "set {char[]}$pc="hi" is parsed and eventually is passed as
>> the element type argument to lookup_array_range_type(). Using any
>> other type, such as "unsigned char", in that gdb command results in
>> the element type that is picked up from gdbarch and has no associated
>> objfile.
> That is exactly the problem. At the point where it decides to use
> an arch-owned type, it should check the type it is for, and whether
> it is arch or objfile owned, and then create the type from there.
> If my intuition is right, my patch should be a good example of what
> needs to be done.
>