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

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

[Weimin Pan]

To support C99 VLA, function value_from_contents_and_address() was
modified to add a call to resolve_dynamic_type(), which in turn
calls resolve_dynamic_array() to resolve the dynamic array bounds
to static values. But the problem arises when function copy_type(),
called by resolve_dynamic_array(), expects the type to be copied
to have an associated objfile from which the new type is allocated,
or asserts. Since type char[] doesn't have an associated objfile
when the following gdb command:

(gdb) set {char[]}$pc="hello"

was issued, gdb asserts.

The gdb_assert (TYPE_OBJFILE_OWNED (type)) line in copy_type() doesn't
look necessary or correct since space needed for the new type could be
allocated from either the type's objfile if it exists or gdbarch if
it doesn't, similar to what alloc_type_copy(), which is called after
gdb_assert() in copy_type(), does. Removing gdb_assert() fixes the
problem.

Tested on aarch64-linux-gnu. No regressions.
---
 gdb/ChangeLog  |    5 +++++
 gdb/gdbtypes.c |    7 +------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 238bcba..5758207 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-01-09  Weimin Pan  <weimin.pan@oracle.com>
+
+	* gdbtypes.c: (copy_type) Do not assert when a type is not associated
+	with an object file.
+
 2018-01-08  Samuel Thibault  <samuel.thibault@ens-lyon.org>
 
 	* gdb/gnu-nat.c: Include <elf.h> and <link.h>.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 7ba62df..e017b6a 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -4870,18 +4870,13 @@ copy_type_recursive (struct objfile *objfile,
 }
 
 /* Make a copy of the given TYPE, except that the pointer & reference
-   types are not preserved.
-   
-   This function assumes that the given type has an associated objfile.
-   This objfile is used to allocate the new type.  */
+   types are not preserved.  */
 
 struct type *
 copy_type (const struct type *type)
 {
   struct type *new_type;
 
-  gdb_assert (TYPE_OBJFILE_OWNED (type));
-
   new_type = alloc_type_copy (type);
   TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
   TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
-- 
1.7.1

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

[Joel Brobecker]

> To support C99 VLA, function value_from_contents_and_address() was
> modified to add a call to resolve_dynamic_type(), which in turn
> calls resolve_dynamic_array() to resolve the dynamic array bounds
> to static values. But the problem arises when function copy_type(),
> called by resolve_dynamic_array(), expects the type to be copied
> to have an associated objfile from which the new type is allocated,
> or asserts. Since type char[] doesn't have an associated objfile
> when the following gdb command:
> 
> (gdb) set {char[]}$pc="hello"
> 
> was issued, gdb asserts.
> 
> The gdb_assert (TYPE_OBJFILE_OWNED (type)) line in copy_type() doesn't
> look necessary or correct since space needed for the new type could be
> allocated from either the type's objfile if it exists or gdbarch if
> it doesn't, similar to what alloc_type_copy(), which is called after
> gdb_assert() in copy_type(), does. Removing gdb_assert() fixes the
> problem.

I think removing the assert just shifts the issue elsewhere.
Basically, you want the lifetime of the new type to match
the lifetime of the object using it. The gdbarch structure
has a lifetime that's different from objfiles.

I happen to have hit the same issue as you, but from an Ada expression,
and sent it a fix not long ago:
https://www.sourceware.org/ml/gdb-patches/2018-01/msg00240.html

Does it fix your problem too?

-- 
Joel

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

[Wei-min Pan]

On 1/24/2018 8:14 PM, Joel Brobecker wrote:
>> To support C99 VLA, function value_from_contents_and_address() was
>> modified to add a call to resolve_dynamic_type(), which in turn
>> calls resolve_dynamic_array() to resolve the dynamic array bounds
>> to static values. But the problem arises when function copy_type(),
>> called by resolve_dynamic_array(), expects the type to be copied
>> to have an associated objfile from which the new type is allocated,
>> or asserts. Since type char[] doesn't have an associated objfile
>> when the following gdb command:
>>
>> (gdb) set {char[]}$pc="hello"
>>
>> was issued, gdb asserts.
>>
>> The gdb_assert (TYPE_OBJFILE_OWNED (type)) line in copy_type() doesn't
>> look necessary or correct since space needed for the new type could be
>> allocated from either the type's objfile if it exists or gdbarch if
>> it doesn't, similar to what alloc_type_copy(), which is called after
>> gdb_assert() in copy_type(), does. Removing gdb_assert() fixes the
>> problem.
> I think removing the assert just shifts the issue elsewhere.
> Basically, you want the lifetime of the new type to match
> the lifetime of the object using it. The gdbarch structure
> has a lifetime that's different from objfiles.

Is there any reason why the gdbarch structure, which won't be freed until
the corresponding architecture is, needs to have a lifetime that matches
the objfiles?

> I happen to have hit the same issue as you, but from an Ada expression,
> and sent it a fix not long ago:
> https://www.sourceware.org/ml/gdb-patches/2018-01/msg00240.html
>
> Does it fix your problem too?
>
Yes, it does fix my problem of gdb asserting on the "set 
{char[]}$pc="hi"" command, as
reported in the PR, but still asserts on a slightly modified "set 
{unsigned char[]}$pc="hi"
command.

Thanks.

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

[Joel Brobecker]

> Is there any reason why the gdbarch structure, which won't be freed
> until the corresponding architecture is, needs to have a lifetime that
> matches the objfiles?

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.

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

> > I happen to have hit the same issue as you, but from an Ada expression,
> > and sent it a fix not long ago:
> > https://www.sourceware.org/ml/gdb-patches/2018-01/msg00240.html
> > 
> > Does it fix your problem too?
> > 
> Yes, it does fix my problem of gdb asserting on the "set {char[]}$pc="hi""
> command, as
> reported in the PR,

Good!

> but still asserts on a slightly modified "set {unsigned char[]}$pc="hi"
> command.

It's should be something fairly similar. Can you track down which type
it is which is arch-owned, and where it comes from? I have a feeling
that there is a simple fix similar to mine to be made that would
fix that.

I can help taking a look, but I'm a little tied up this week...

-- 
Joel

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

[Wei-min Pan]

On 1/30/2018 11:45 PM, Joel Brobecker wrote:
>> Is there any reason why the gdbarch structure, which won't be freed
>> until the corresponding architecture is, needs to have a lifetime that
>> matches the objfiles?
> 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.

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

>>> I happen to have hit the same issue as you, but from an Ada expression,
>>> and sent it a fix not long ago:
>>> https://www.sourceware.org/ml/gdb-patches/2018-01/msg00240.html
>>>
>>> Does it fix your problem too?
>>>
>> Yes, it does fix my problem of gdb asserting on the "set {char[]}$pc="hi""
>> command, as
>> reported in the PR,
> Good!
>
>> but still asserts on a slightly modified "set {unsigned char[]}$pc="hi"
>> command.
> It's should be something fairly similar. Can you track down which type
> it is which is arch-owned, and where it comes from? I have a feeling
> that there is a simple fix similar to mine to be made that would
> fix that.

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.

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.

>
> I can help taking a look, but I'm a little tied up this week...
>

I'd really appreciate it if you can take a look at your convenience. It 
doesn't
have to be this week. Thanks.

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

[Joel Brobecker]

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

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

-- 
Joel

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

[Wei-min Pan]

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.

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

[Wei-min Pan]

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

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

[Joel Brobecker]

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

I am not sure I understand what you are saying. On my end, I am
precisely saying that we should not create a type where we have
a mix and match. And if I read this last message correctly, you
are saying you don't see how it's possible to be doing that. So,
on the surface, your last message seems to be in agreement with
the property I am saying we should preserve. But aren't you reporting
a situation where we are actually trying to create a type with
inconsistent ownership? Otherwise, I don't see why we would have
triggered the assertion failure you reported.

-- 
Joel

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

[Wei-min Pan]

On 11/14/2018 3:51 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.
> I am not sure I understand what you are saying. On my end, I am
> precisely saying that we should not create a type where we have
> a mix and match. And if I read this last message correctly, you
> are saying you don't see how it's possible to be doing that. So,
> on the surface, your last message seems to be in agreement with
> the property I am saying we should preserve. But aren't you reporting
> a situation where we are actually trying to create a type with
> inconsistent ownership? Otherwise, I don't see why we would have
> triggered the assertion failure you reported.
>

Problem is copy_type will assert on to-be-copied type which does not 
have an
associated objfile. It doesn't matter if the type is entirely arch-owned.

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

[Tom Tromey]

>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:

>> Problem is copy_type will assert on to-be-copied type which does not
>> have an
>> associated objfile. It doesn't matter if the type is entirely arch-owned.

I didn't follow this thread in too much detail, but FWIW I believe the
rule is that an objfile-owned type can refer to a gdbarch-owned type --
but not vice versa.

Following that it seems to me that there should not be a need to call
copy_type on a gdbarch-owned type.  So maybe that can be avoided,
instead of removing the assert?

Tom

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

[Wei-min Pan]

On 11/29/2018 11:18 AM, Tom Tromey wrote:
>>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:
>>> Problem is copy_type will assert on to-be-copied type which does not
>>> have an
>>> associated objfile. It doesn't matter if the type is entirely arch-owned.
> I didn't follow this thread in too much detail, but FWIW I believe the
> rule is that an objfile-owned type can refer to a gdbarch-owned type --
> but not vice versa.
>
> Following that it seems to me that there should not be a need to call
> copy_type on a gdbarch-owned type.  So maybe that can be avoided,
> instead of removing the assert?
>
> Tom

Looks like we have at least 2 options:

(1) Making sure the type is objfile-owned before calling copy_type in
resolve_dynamic_range and resolve_dynamic_array as you suggested, or

(2) Replacing the assert with an objfile-owned check in copy_type, similar
to what copy_type_recursive does.

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

[Tom Tromey]

>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:

>> Looks like we have at least 2 options:

>> (1) Making sure the type is objfile-owned before calling copy_type in
>> resolve_dynamic_range and resolve_dynamic_array as you suggested, or

>> (2) Replacing the assert with an objfile-owned check in copy_type, similar
>> to what copy_type_recursive does.

Sorry, I still didn't read the whole thread... but I think what to do
depends on what is happening.

Most callers of copy_type are probably copying it to modify the copy.
If this is the case, then maybe just removing the assert is ok.
Or, maybe it makes sense to understand why the modified type isn't
objfile-allocated in the first place.

Could you recap?  What is calling copy_type here and where did the type
come from?

Tom

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

[Wei-min Pan]

On 11/29/2018 1:52 PM, Tom Tromey wrote:
>>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:
>>> Looks like we have at least 2 options:
>>> (1) Making sure the type is objfile-owned before calling copy_type in
>>> resolve_dynamic_range and resolve_dynamic_array as you suggested, or
>>> (2) Replacing the assert with an objfile-owned check in copy_type, similar
>>> to what copy_type_recursive does.
> Sorry, I still didn't read the whole thread... but I think what to do
> depends on what is happening.
>
> Most callers of copy_type are probably copying it to modify the copy.
> If this is the case, then maybe just removing the assert is ok.
> Or, maybe it makes sense to understand why the modified type isn't
> objfile-allocated in the first place.
>
> Could you recap?  What is calling copy_type here and where did the type
> come from?
>
> Tom

Let's use an example:

(gdb) p {char []}$pc

if the element type "char" is defined in the program, i.e. it has an 
objfile,
lookup_array_range_type sets the "index type" with that objfile's 
builtin_int.
Otherwise, it sets it with gdbarch's built-in. The index type is then used
to create the range type and the array type. Thus the array type is entirely
either objfile-owned or gdbarch-owned.

Both resolve_dynamic_array and resolve_dynamic_range call copy_type to
allocate a type and yes modify it to represent a static version of the array
type from lookup_array_range_type() above.

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

[Tom Tromey]

>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:

>> Let's use an example:
>> (gdb) p {char []}$pc

>> if the element type "char" is defined in the program, i.e. it has an
>> objfile, lookup_array_range_type sets the "index type" with that
>> objfile's builtin_int.

Thanks.

>> Both resolve_dynamic_array and resolve_dynamic_range call copy_type to
>> allocate a type and yes modify it to represent a static version of the array
>> type from lookup_array_range_type() above.

Ok, I see.  Removing the assertion does seem ok then.

thanks,
Tom

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

[Wei-min Pan]

On 11/30/2018 7:37 AM, Tom Tromey wrote:
>>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:
>>> Let's use an example:
>>> (gdb) p {char []}$pc
>>> if the element type "char" is defined in the program, i.e. it has an
>>> objfile, lookup_array_range_type sets the "index type" with that
>>> objfile's builtin_int.
> Thanks.
>
>>> Both resolve_dynamic_array and resolve_dynamic_range call copy_type to
>>> allocate a type and yes modify it to represent a static version of the array
>>> type from lookup_array_range_type() above.
> Ok, I see.  Removing the assertion does seem ok then.

Thanks for your review.


>
> thanks,
> Tom