diff mbox

[07/25] Lazily and dynamically create i386-linux target descriptions

Message ID 1497256916-4958-8-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi June 12, 2017, 8:41 a.m. UTC 
  Instead of using pre-generated target descriptions, this patch
changes GDB to lazily and dynamically create target descriptions
according to the target hardware capability (xcr0 in i386).
This support any combination of target features.

This patch also adds a unit test to make sure dynamically generated
tdesc are identical to these generated from xml files.

gdb:

2017-04-27  Yao Qi  <yao.qi@linaro.org>

	* i386-linux-tdep.c (i386_linux_read_description): Generate
	target description if it doesn't exist.
        [GDB_SELF_TEST] (i386_linux_read_description_test): New unit test.
        (_initialize_i386_linux_tdep) [GDB_SELF_TEST]: Register unit test.
---
 gdb/features/i386/32bit-linux.c |  1 +
 gdb/features/i386/32bit-sse.c   |  1 +
 gdb/i386-linux-tdep.c           | 83 ++++++++++++++++++++++++-----------------
 gdb/target-descriptions.c       | 11 ++++++
 4 files changed, 62 insertions(+), 34 deletions(-)

Comments

Pedro Alves June 20, 2017, 11:01 a.m. UTC | #1 
On 06/12/2017 09:41 AM, Yao Qi wrote:
> Instead of using pre-generated target descriptions, this patch
> changes GDB to lazily and dynamically create target descriptions
> according to the target hardware capability (xcr0 in i386).
> This support any combination of target features.
> 
> This patch also adds a unit test to make sure dynamically generated
> tdesc are identical to these generated from xml files.

I don't see the unit test in the patch.

Can you say something about the "regnum = %ld" change?  Is it
related to the rest of the patch?

> 
> 2017-04-27  Yao Qi  <yao.qi@linaro.org>
> 
> 	* i386-linux-tdep.c (i386_linux_read_description): Generate
> 	target description if it doesn't exist.
>         [GDB_SELF_TEST] (i386_linux_read_description_test): New unit test.
>         (_initialize_i386_linux_tdep) [GDB_SELF_TEST]: Register unit test.

Leading TAB vs spaces.

> +private:
> +  /* The register number to use for the next register we see.  */
> +  int next_regnum = 0;
>  };

"m_" prefix.

Thanks,
Pedro Alves
Yao Qi June 20, 2017, 2:07 p.m. UTC | #2 
Pedro Alves <palves@redhat.com> writes:

> I don't see the unit test in the patch.
>

It is moved to patch 08/25.

> Can you say something about the "regnum = %ld" change?  Is it
> related to the rest of the patch?
>

Without this change, the lazily generated tdesc doesn't equal to the
original pre-generated tdesc if the register has regnum attribute, like

  <reg name="xmm0" bitsize="128" type="vec128" regnum="32"/>

most of the reg doesn't have "regnum" attribute so the number is
allocated sequentially.  If the reg has "regnum", it explicitly set the
register number.  I'll rewrite the commit log this way,

Instead of using pre-generated target descriptions, this patch
changes GDB to lazily and dynamically create target descriptions
according to the target hardware capability (xcr0 in i386).
This support any combination of target features.

Some reg in XML target description has attribute "regnum" which set the
register number explicitly rather than using the sequentially allocated
number,

  <reg name="xmm0" bitsize="128" type="vec128" regnum="32"/>

In order to handle this, this patch also changes the visitor to print
the code to set regnum when reg has attribute "regnum".

2017-06-20  Yao Qi  <yao.qi@linaro.org>

	* i386-linux-tdep.c: Don't include features/i386/i386-XXX-linux.c.
        include features/i386/32bit-XXX.c instead.
        (i386_linux_read_description): Generate
        target description if it doesn't exist.
        * target-descriptions.c (print_c_tdesc::visit): Print code to
        set regnum.
        (print_c_tdesc) <m_next_regnum>: New field.
        * features/i386/32bit-linux.c: Regenerated.
        * features/i386/32bit-sse.c: Regenerated.

>> 
>> 2017-04-27  Yao Qi  <yao.qi@linaro.org>
>> 
>> 	* i386-linux-tdep.c (i386_linux_read_description): Generate
>> 	target description if it doesn't exist.
>>         [GDB_SELF_TEST] (i386_linux_read_description_test): New unit test.
>>         (_initialize_i386_linux_tdep) [GDB_SELF_TEST]: Register unit test.
>
> Leading TAB vs spaces.
>

These two lines should be removed.

>> +private:
>> +  /* The register number to use for the next register we see.  */
>> +  int next_regnum = 0;
>>  };
>
> "m_" prefix.

I'll fix it.
Pedro Alves June 28, 2017, 3:30 p.m. UTC | #3 
On 06/20/2017 03:07 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:

>> Can you say something about the "regnum = %ld" change?  Is it
>> related to the rest of the patch?
>>

...

> Some reg in XML target description has attribute "regnum" which set the
> register number explicitly rather than using the sequentially allocated
> number,
> 
>   <reg name="xmm0" bitsize="128" type="vec128" regnum="32"/>
> 
> In order to handle this, this patch also changes the visitor to print
> the code to set regnum when reg has attribute "regnum".

I see.

   void visit (const tdesc_reg *reg) override
   {
+    if (reg->target_regnum > next_regnum)
+      {
+	printf_unfiltered ("  regnum = %ld;\n", reg->target_regnum);
+	next_regnum = reg->target_regnum;
+      }

I guess "reg->target_regnum < next_regnum" shouldn't ever
happen, right?  Should we add an assert?

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/features/i386/32bit-linux.c b/gdb/features/i386/32bit-linux.c
index 3f7bfe7..ff90d40 100644
--- a/gdb/features/i386/32bit-linux.c
+++ b/gdb/features/i386/32bit-linux.c
@@ -11,6 +11,7 @@  create_feature_i386_32bit_linux (struct target_desc *result, long regnum)
   struct tdesc_feature *feature;
 
   feature = tdesc_create_feature (result, "org.gnu.gdb.i386.linux");
+  regnum = 41;
   tdesc_create_reg (feature, "orig_eax", regnum++, 1, NULL, 32, "int");
   return regnum;
 }
diff --git a/gdb/features/i386/32bit-sse.c b/gdb/features/i386/32bit-sse.c
index 9aa7d3e..08c3948 100644
--- a/gdb/features/i386/32bit-sse.c
+++ b/gdb/features/i386/32bit-sse.c
@@ -63,6 +63,7 @@  create_feature_i386_32bit_sse (struct target_desc *result, long regnum)
   tdesc_add_flag (type, 12, "PM");
   tdesc_add_flag (type, 15, "FZ");
 
+  regnum = 32;
   tdesc_create_reg (feature, "xmm0", regnum++, 1, NULL, 128, "vec128");
   tdesc_create_reg (feature, "xmm1", regnum++, 1, NULL, 128, "vec128");
   tdesc_create_reg (feature, "xmm2", regnum++, 1, NULL, 128, "vec128");
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 1bc1a6f..5ca58a1 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -45,13 +45,14 @@ 
 
 #include "record-full.h"
 #include "linux-record.h"
-#include "features/i386/i386-linux.c"
-#include "features/i386/i386-mmx-linux.c"
-#include "features/i386/i386-mpx-linux.c"
-#include "features/i386/i386-avx-mpx-linux.c"
-#include "features/i386/i386-avx-linux.c"
-#include "features/i386/i386-avx-avx512-linux.c"
-#include "features/i386/i386-avx-mpx-avx512-pku-linux.c"
+
+#include "features/i386/32bit-core.c"
+#include "features/i386/32bit-sse.c"
+#include "features/i386/32bit-linux.c"
+#include "features/i386/32bit-avx.c"
+#include "features/i386/32bit-mpx.c"
+#include "features/i386/32bit-avx512.c"
+#include "features/i386/32bit-pkeys.c"
 
 /* Return non-zero, when the register is in the corresponding register
    group.  Put the LINUX_ORIG_EAX register in the system group.  */
@@ -681,27 +682,50 @@  i386_linux_core_read_xcr0 (bfd *abfd)
 const struct target_desc *
 i386_linux_read_description (uint64_t xcr0)
 {
-  switch ((xcr0 & X86_XSTATE_ALL_MASK))
+  if (xcr0 == 0)
+    return NULL;
+
+  static struct target_desc *i386_linux_tdescs \
+    [2/*X87*/][2/*SSE*/][2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
+  struct target_desc **tdesc;
+
+  tdesc = &i386_linux_tdescs[(xcr0 & X86_XSTATE_X87) ? 1 : 0]
+    [(xcr0 & X86_XSTATE_SSE) ? 1 : 0]
+    [(xcr0 & X86_XSTATE_AVX) ? 1 : 0]
+    [(xcr0 & X86_XSTATE_MPX) ? 1 : 0]
+    [(xcr0 & X86_XSTATE_AVX512) ? 1 : 0]
+    [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0];
+
+  if (*tdesc == NULL)
     {
-    case X86_XSTATE_AVX_MPX_AVX512_PKU_MASK:
-      return tdesc_i386_avx_mpx_avx512_pku_linux;
-    case X86_XSTATE_AVX_AVX512_MASK:
-      return tdesc_i386_avx_avx512_linux;
-    case X86_XSTATE_MPX_MASK:
-      return tdesc_i386_mpx_linux;
-    case X86_XSTATE_AVX_MPX_MASK:
-      return tdesc_i386_avx_mpx_linux;
-    case X86_XSTATE_AVX_MASK:
-      return tdesc_i386_avx_linux;
-    case X86_XSTATE_SSE_MASK:
-      return tdesc_i386_linux;
-    case X86_XSTATE_X87_MASK:
-      return tdesc_i386_mmx_linux;
-    default:
-      break;
+      *tdesc = allocate_target_description ();
+      set_tdesc_architecture (*tdesc, bfd_scan_arch ("i386"));
+      set_tdesc_osabi (*tdesc, osabi_from_tdesc_string ("GNU/Linux"));
+
+      long regnum = 0;
+
+      if (xcr0 & X86_XSTATE_X87)
+	regnum = create_feature_i386_32bit_core (*tdesc, regnum);
+
+      if (xcr0 & X86_XSTATE_SSE)
+	regnum = create_feature_i386_32bit_sse (*tdesc, regnum);
+
+      regnum = create_feature_i386_32bit_linux (*tdesc, regnum);
+
+      if (xcr0 & X86_XSTATE_AVX)
+	regnum = create_feature_i386_32bit_avx (*tdesc, regnum);
+
+      if (xcr0 & X86_XSTATE_MPX)
+	regnum = create_feature_i386_32bit_mpx (*tdesc, regnum);
+
+      if (xcr0 & X86_XSTATE_AVX512)
+	regnum = create_feature_i386_32bit_avx512 (*tdesc, regnum);
+
+      if (xcr0 & X86_XSTATE_PKRU)
+	regnum = create_feature_i386_32bit_pkeys (*tdesc, regnum);
     }
 
-  return NULL;
+  return *tdesc;
 }
 
 /* Get Linux/x86 target description from core dump.  */
@@ -1092,13 +1116,4 @@  _initialize_i386_linux_tdep (void)
 {
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_LINUX,
 			  i386_linux_init_abi);
-
-  /* Initialize the Linux target description.  */
-  initialize_tdesc_i386_linux ();
-  initialize_tdesc_i386_mmx_linux ();
-  initialize_tdesc_i386_avx_linux ();
-  initialize_tdesc_i386_mpx_linux ();
-  initialize_tdesc_i386_avx_mpx_linux ();
-  initialize_tdesc_i386_avx_avx512_linux ();
-  initialize_tdesc_i386_avx_mpx_avx512_pku_linux ();
 }
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index fceab5f..c0b716a 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -2100,6 +2100,12 @@  public:
 
   void visit (const tdesc_reg *reg) override
   {
+    if (reg->target_regnum > next_regnum)
+      {
+	printf_unfiltered ("  regnum = %ld;\n", reg->target_regnum);
+	next_regnum = reg->target_regnum;
+      }
+
     printf_unfiltered ("  tdesc_create_reg (feature, \"%s\", regnum++, %d, ",
 		       reg->name, reg->save_restore);
     if (reg->group)
@@ -2107,8 +2113,13 @@  public:
     else
       printf_unfiltered ("NULL, ");
     printf_unfiltered ("%d, \"%s\");\n", reg->bitsize, reg->type);
+
+    next_regnum++;
   }
 
+private:
+  /* The register number to use for the next register we see.  */
+  int next_regnum = 0;
 };
 
 static void