Patchwork [11/25] Use VEC for target_desc.reg_defs

login
register
mail settings
Submitter Yao Qi
Date June 12, 2017, 8:41 a.m.
Message ID <1497256916-4958-12-git-send-email-yao.qi@linaro.org>
Download mbox | patch
Permalink /patch/20926/
State New
Headers show

Comments

Yao Qi - June 12, 2017, 8:41 a.m.
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(-)
Pedro Alves - June 28, 2017, 7:01 p.m.
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
Yao Qi - June 29, 2017, 11:05 a.m.
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?
Pedro Alves - June 29, 2017, 11:31 a.m.
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
Yao Qi - June 29, 2017, 1:24 p.m.
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.

Patch

diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 01dfdc0..1974db0 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -387,7 +387,7 @@  gdbreplay$(EXEEXT): $(GDBREPLAY_OBS) $(LIBGNU) $(LIBIBERTY)
 IPA_OBJS = ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o \
 	regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o \
 	tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o errors-ipa.o \
-	${IPA_DEPFILES}
+	vec-ipa.o ${IPA_DEPFILES}
 
 IPA_LIB = libinproctrace.so
 
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 0f16f60..7dbdca7 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -54,7 +54,7 @@  get_thread_regcache (struct thread_info *thread, int fetch)
       current_thread = thread;
       /* Invalidate all registers, to prevent stale left-overs.  */
       memset (regcache->register_status, REG_UNAVAILABLE,
-	      regcache->tdesc->num_registers);
+	      VEC_length (tdesc_reg_p, regcache->tdesc->reg_defs));
       fetch_inferior_registers (regcache, -1);
       current_thread = saved_thread;
       regcache->registers_valid = 1;
@@ -145,9 +145,9 @@  init_register_cache (struct regcache *regcache,
 	= (unsigned char *) xcalloc (1, tdesc->registers_size);
       regcache->registers_owned = 1;
       regcache->register_status
-	= (unsigned char *) xmalloc (tdesc->num_registers);
+	= (unsigned char *) xmalloc (VEC_length (tdesc_reg_p, tdesc->reg_defs));
       memset ((void *) regcache->register_status, REG_UNAVAILABLE,
-	      tdesc->num_registers);
+	      VEC_length (tdesc_reg_p, tdesc->reg_defs));
 #else
       gdb_assert_not_reached ("can't allocate memory from the heap");
 #endif
@@ -204,7 +204,7 @@  regcache_cpy (struct regcache *dst, struct regcache *src)
 #ifndef IN_PROCESS_AGENT
   if (dst->register_status != NULL && src->register_status != NULL)
     memcpy (dst->register_status, src->register_status,
-	    src->tdesc->num_registers);
+	    VEC_length (tdesc_reg_p, src->tdesc->reg_defs));
 #endif
   dst->registers_valid = src->registers_valid;
 }
@@ -218,8 +218,9 @@  registers_to_string (struct regcache *regcache, char *buf)
   unsigned char *registers = regcache->registers;
   const struct target_desc *tdesc = regcache->tdesc;
   int i;
+  struct reg *reg;
 
-  for (i = 0; i < tdesc->num_registers; i++)
+  for (i = 0; VEC_iterate (tdesc_reg_p, tdesc->reg_defs, i, reg); i++)
     {
       if (regcache->register_status[i] == REG_VALID)
 	{
@@ -257,22 +258,23 @@  int
 find_regno (const struct target_desc *tdesc, const char *name)
 {
   int i;
+  struct reg *reg;
 
-  for (i = 0; i < tdesc->num_registers; i++)
-    if (strcmp (name, tdesc->reg_defs[i].name) == 0)
+  for (i = 0; VEC_iterate (tdesc_reg_p, tdesc->reg_defs, i, reg); i++)
+    if (strcmp (name, reg->name) == 0)
       return i;
   internal_error (__FILE__, __LINE__, "Unknown register %s requested",
 		  name);
 }
 
+#endif
+
 struct reg *
 find_register_by_number (const struct target_desc *tdesc, int n)
 {
-  return &tdesc->reg_defs[n];
+  return VEC_index (tdesc_reg_p, tdesc->reg_defs, n);
 }
 
-#endif
-
 #ifndef IN_PROCESS_AGENT
 static void
 free_register_cache_thread (struct thread_info *thread)
@@ -312,7 +314,7 @@  register_cache_size (const struct target_desc *tdesc)
 int
 register_size (const struct target_desc *tdesc, int n)
 {
-  return tdesc->reg_defs[n].size / 8;
+  return find_register_by_number (tdesc, n)->size / 8;
 }
 
 /* See common/common-regcache.h.  */
@@ -326,7 +328,8 @@  regcache_register_size (const struct regcache *regcache, int n)
 static unsigned char *
 register_data (struct regcache *regcache, int n, int fetch)
 {
-  return regcache->registers + regcache->tdesc->reg_defs[n].offset / 8;
+  return (regcache->registers
+	  + find_register_by_number (regcache->tdesc, n)->offset / 8);
 }
 
 /* Supply register N, whose contents are stored in BUF, to REGCACHE.
@@ -385,7 +388,7 @@  supply_regblock (struct regcache *regcache, const void *buf)
       {
 	int i;
 
-	for (i = 0; i < tdesc->num_registers; i++)
+	for (i = 0; i < VEC_length (tdesc_reg_p, tdesc->reg_defs); i++)
 	  regcache->register_status[i] = REG_VALID;
       }
 #endif
@@ -399,7 +402,7 @@  supply_regblock (struct regcache *regcache, const void *buf)
       {
 	int i;
 
-	for (i = 0; i < tdesc->num_registers; i++)
+	for (i = 0; i < VEC_length (tdesc_reg_p, tdesc->reg_defs); i++)
 	  regcache->register_status[i] = REG_UNAVAILABLE;
       }
 #endif
@@ -431,7 +434,8 @@  regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
   int size;
 
   gdb_assert (regcache != NULL);
-  gdb_assert (regnum >= 0 && regnum < regcache->tdesc->num_registers);
+  gdb_assert (regnum >= 0
+	      && regnum < VEC_length (tdesc_reg_p, regcache->tdesc->reg_defs));
 
   size = register_size (regcache->tdesc, regnum);
 
diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
index fdd35197..1b1882e 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -23,12 +23,13 @@  void
 init_target_desc (struct target_desc *tdesc)
 {
   int offset, i;
+  struct reg *reg;
 
   offset = 0;
-  for (i = 0; i < tdesc->num_registers; i++)
+  for (i = 0; VEC_iterate (tdesc_reg_p, tdesc->reg_defs, i, reg); i++)
     {
-      tdesc->reg_defs[i].offset = offset;
-      offset += tdesc->reg_defs[i].size;
+      reg->offset = offset;
+      offset += reg->size;
     }
 
   tdesc->registers_size = offset / 8;
@@ -40,14 +41,13 @@  init_target_desc (struct target_desc *tdesc)
 
 #ifndef IN_PROCESS_AGENT
 
-static const struct target_desc default_description = { 0 };
+static const struct target_desc default_description = { NULL, 0, NULL, NULL };
 
 void
 copy_target_description (struct target_desc *dest,
 			 const struct target_desc *src)
 {
   dest->reg_defs = src->reg_defs;
-  dest->num_registers = src->num_registers;
   dest->expedite_regs = src->expedite_regs;
   dest->registers_size = src->registers_size;
   dest->xmltarget = src->xmltarget;
diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
index 0341278..424a2fd 100644
--- a/gdb/gdbserver/tdesc.h
+++ b/gdb/gdbserver/tdesc.h
@@ -21,17 +21,16 @@ 
 
 struct reg;
 
+typedef struct reg *tdesc_reg_p;
+DEF_VEC_P(tdesc_reg_p);
+
 /* A target description.  */
 
 struct target_desc
 {
-  /* An array of NUM_REGISTERS elements of register definitions that
+  /* A vector of elements of register definitions that
      describe the inferior's register set.  */
-  struct reg *reg_defs;
-
-  /* The number of registers in inferior's register set (and thus in
-     the regcache).  */
-  int num_registers;
+  VEC(tdesc_reg_p) *reg_defs;
 
   /* The register cache size, in bytes.  */
   int registers_size;
diff --git a/gdb/regformats/regdat.sh b/gdb/regformats/regdat.sh
index 2c764cd..236cd93 100755
--- a/gdb/regformats/regdat.sh
+++ b/gdb/regformats/regdat.sh
@@ -131,8 +131,8 @@  do
     echo "{"
     echo "  static struct target_desc tdesc_${name}_s;"
     echo "  struct target_desc *result = &tdesc_${name}_s;"
+    echo "  memset (result, 0, sizeof (*result));"
 
-    echo "static struct reg regs_${name}[] = {"
     continue
   elif test "${type}" = "xmltarget"; then
     xmltarget="${entry}"
@@ -150,13 +150,17 @@  do
     echo "$0: $1 does not specify \`\`name''." 1>&2
     exit 1
   else
-    echo "  { \"${entry}\", ${offset}, ${type} },"
+    echo "  {struct reg *reg = XCNEW (struct reg);"
+    echo "  reg->name = \"${entry}\";"
+    echo "  reg->offset = ${offset};"
+    echo "  reg->size = ${type};"
+    echo "  VEC_safe_push (tdesc_reg_p, result->reg_defs, reg);"
+    echo "  };"
     offset=`expr ${offset} + ${type}`
     i=`expr $i + 1`
   fi
 done
 
-echo "};"
 echo
 echo "static const char *expedite_regs_${name}[] = { \"`echo ${expedite} | sed 's/,/", "/g'`\", 0 };"
 if test "${xmltarget}" = x; then
@@ -178,9 +182,6 @@  fi
 echo
 
 cat <<EOF
-  result->reg_defs = regs_${name};
-  result->num_registers = sizeof (regs_${name}) / sizeof (regs_${name}[0]);
-
 #ifndef IN_PROCESS_AGENT
   result->expedite_regs = expedite_regs_${name};
   result->xmltarget = xmltarget_${name};