[11/25] Use VEC for target_desc.reg_defs

  Nowadays, target_desc.reg_defs is a pointer points to a pre-generated
array, which is not flexible.  This patch changes it from an array
to a VEC so that GDBserver can create target descriptions dynamically
later.  Instead of using pre-generated array, the -generated.c calls
VEC_safe_push to add each register to vector.

Since target_desc.reg_defs is used in IPA, we need to build common/vec.c
for IPA too.

gdb/gdbserver:

2017-05-23  Yao Qi  <yao.qi@linaro.org>

	* Makefile.in (IPA_OBJS): Add vec-ipa.o
	* regcache.c (get_thread_regcache): Use VEC_length.
	(init_register_cache): Likewise.
	(regcache_cpy): Likewise.
	(registers_to_string): Iterate reg_defs via VEC_iterate.
	(find_regno): Likewise.
	(find_register_by_number): Use VEC_index.
	(register_size): Call find_register_by_number.
	(register_data): Call find_register_by_number.
	(supply_regblock): Use VEC_length.
	(regcache_raw_read_unsigned): Likewise.
	* tdesc.c (init_target_desc): Iterate reg_defs via
	VEC_iterate.
	(default_description): Update initializer.
	(copy_target_description): Don't update field num_registers.
	* tdesc.h (struct target_desc) <reg_defs>: Change it to VEC.
	<num_registers>: Remove.

gdb:

2017-05-23  Yao Qi  <yao.qi@linaro.org>

	* regformats/regdat.sh: Update generated code.
---
 gdb/gdbserver/Makefile.in |  2 +-
 gdb/gdbserver/regcache.c  | 34 +++++++++++++++++++---------------
 gdb/gdbserver/tdesc.c     | 10 +++++-----
 gdb/gdbserver/tdesc.h     | 11 +++++------
 gdb/regformats/regdat.sh  | 13 +++++++------
 5 files changed, 37 insertions(+), 33 deletions(-)
  

On 06/12/2017 09:41 AM, Yao Qi wrote:
> Nowadays, target_desc.reg_defs is a pointer points to a pre-generated
> array, which is not flexible.  This patch changes it from an array
> to a VEC so that GDBserver can create target descriptions dynamically
> later.  Instead of using pre-generated array, the -generated.c calls
> VEC_safe_push to add each register to vector.
> 
> Since target_desc.reg_defs is used in IPA, we need to build common/vec.c
> for IPA too.

Can you say more about the choice of VEC?  It feels
like new uses should come with a rationale for why it'd
be preferred over std::vector.

I'm guessing that it's because the gdb side uses VEC too?
Or is it something else?  (I can guess other reasons, but
the point is that we shouldn't have to guess.)

[Note that the IPA avoids calling the inferior's malloc during
normal operation, to avoid deadlocking the inferior.
This is initialization code, so it's not covered by the exact
same level of concern, even though one of the original goals was
to also be able to inject the IPA into a running inferior (e.g., by
calling dlopen via gdb).  That does work (or at least used to),
but it's a little unsafe because the IPA initialization code
already calls malloc and other non-async-signal-safe functions.
I guess std::vector would make it possible to use a custom
allocator in the IPA that would allocate memory with mmap
directly (or we'd make the IPA's xmalloc allocate with mmap,
and then the allocator would use xmalloc).]

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> Can you say more about the choice of VEC?  It feels
> like new uses should come with a rationale for why it'd
> be preferred over std::vector.
>
> I'm guessing that it's because the gdb side uses VEC too?
> Or is it something else?  (I can guess other reasons, but
> the point is that we shouldn't have to guess.)

Yes, GDB target description uses VEC, so I chose VEC for GDBserver
target description too.

>
> [Note that the IPA avoids calling the inferior's malloc during
> normal operation, to avoid deadlocking the inferior.
> This is initialization code, so it's not covered by the exact
> same level of concern, even though one of the original goals was
> to also be able to inject the IPA into a running inferior (e.g., by
> calling dlopen via gdb).  That does work (or at least used to),
> but it's a little unsafe because the IPA initialization code
> already calls malloc and other non-async-signal-safe functions.

Such usage is documented, at least,
https://sourceware.org/gdb/onlinedocs/gdb/Server.html

> I guess std::vector would make it possible to use a custom
> allocator in the IPA that would allocate memory with mmap
> directly (or we'd make the IPA's xmalloc allocate with mmap,
> and then the allocator would use xmalloc).]

Do you suggest that we need to use std::vector plus a customized
allocator which uses mmap?
  

On 06/29/2017 12:05 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Can you say more about the choice of VEC?  It feels
>> like new uses should come with a rationale for why it'd
>> be preferred over std::vector.
>>
>> I'm guessing that it's because the gdb side uses VEC too?
>> Or is it something else?  (I can guess other reasons, but
>> the point is that we shouldn't have to guess.)
> 
> Yes, GDB target description uses VEC, so I chose VEC for GDBserver
> target description too.

OK.

> 
>>
>> [Note that the IPA avoids calling the inferior's malloc during
>> normal operation, to avoid deadlocking the inferior.
>> This is initialization code, so it's not covered by the exact
>> same level of concern, even though one of the original goals was
>> to also be able to inject the IPA into a running inferior (e.g., by
>> calling dlopen via gdb).  That does work (or at least used to),
>> but it's a little unsafe because the IPA initialization code
>> already calls malloc and other non-async-signal-safe functions.
> 
> Such usage is documented, at least,
> https://sourceware.org/gdb/onlinedocs/gdb/Server.html

Yeah.  We also call "malloc" in the inferior (value_allocate_space_in_inferior)
for coercing arrays, etc. to the inferior ('print "hello"', etc.) which
suffers from the same problem.  I'm often surprised how we
don't hear more bug reports about that.

> 
>> I guess std::vector would make it possible to use a custom
>> allocator in the IPA that would allocate memory with mmap
>> directly (or we'd make the IPA's xmalloc allocate with mmap,
>> and then the allocator would use xmalloc).]
> 
> Do you suggest that we need to use std::vector plus a customized
> allocator which uses mmap?

No, not now, at least.  At first I thought that using VEC instead of
static arrays would be the introducing the problem of using
malloc in the initialization path.  Then while writing a
review in that direction I looked at the code and
realized/remembered that it's not really a new problem.
But I chose to dump my thoughts on how that might
be fixable in the future as a parenthesis.  I don't mean
to go fix that now.

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

>>> [Note that the IPA avoids calling the inferior's malloc during
>>> normal operation, to avoid deadlocking the inferior.
>>> This is initialization code, so it's not covered by the exact
>>> same level of concern, even though one of the original goals was
>>> to also be able to inject the IPA into a running inferior (e.g., by
>>> calling dlopen via gdb).  That does work (or at least used to),
>>> but it's a little unsafe because the IPA initialization code
>>> already calls malloc and other non-async-signal-safe functions.
>> 
>> Such usage is documented, at least,
>> https://sourceware.org/gdb/onlinedocs/gdb/Server.html
>
> Yeah.  We also call "malloc" in the inferior (value_allocate_space_in_inferior)
> for coercing arrays, etc. to the inferior ('print "hello"', etc.) which
> suffers from the same problem.  I'm often surprised how we
> don't hear more bug reports about that.
>

I can understand IPA calls malloc may cause deadlock in the inferior, if
the inferior has already acquired the lock for malloc.  Think it a bit
more during lunch, I don't understand how it is related to
non-async-signal-safe functions?  Is it that main program executed some
non-async-signal-safe function, signal arrives, goes to signal handler,
hits a breakpoint or fast tracepoint here.  Then, the
non-async-signal-safe functions may be executed again either by gdb
inferior call or IPA.  If this is the problem, ...

>> 
>>> I guess std::vector would make it possible to use a custom
>>> allocator in the IPA that would allocate memory with mmap
>>> directly (or we'd make the IPA's xmalloc allocate with mmap,
>>> and then the allocator would use xmalloc).]
>> 
>> Do you suggest that we need to use std::vector plus a customized
>> allocator which uses mmap?
>
> No, not now, at least.  At first I thought that using VEC instead of
> static arrays would be the introducing the problem of using
> malloc in the initialization path.  Then while writing a
> review in that direction I looked at the code and
> realized/remembered that it's not really a new problem.
> But I chose to dump my thoughts on how that might
> be fixable in the future as a parenthesis.  I don't mean
> to go fix that now.

... then, we can't use std::vector either, because it may throw
exception, or we somehow need to guarantee that the std::vector we used
doesn't throw exception.