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.

I see, thanks for the clarification. But it doesn't seem to be the case
when we have a declaration like "unsigned char p[] = "abc";". The array
type and its element type will split between an objfile and an 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!

What needs to be done then is to pick the correct obstack, from either
objfile_obstack or gdbarch->obstack, when copying the dynamic property list?

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

Since the type to be copied needs to be objfile-owned in copy_type(), 
will it still trigger
the assertion if the complete type was created and owned by an arch?

Thanks.