Patchwork [04/25] Centralize i386 linux target descriptions

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

Comments

Yao Qi - June 12, 2017, 8:41 a.m.
This patch moves all the tdesc_i386*_linux target descriptions to a
function i386_linux_read_description, which returns the right target
description according to xcr0.  This also remove the duplication in
getting target descriptions in corefile and native target.

gdb:

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

	* i386-linux-tdep.c (i386_linux_read_description): New function.
	(i386_linux_core_read_description): Call
	i386_linux_read_description.
	* i386-linux-tdep.h (i386_linux_read_description): Declare.
	(tdesc_i386_linux, tdesc_i386_mmx_linux): Remove declarations.
	(tdesc_i386_avx_linux, tdesc_i386_mpx_linux): Likewise
	(tdesc_i386_avx_mpx_linux, tdesc_i386_avx_avx512_linux): Likewise.
	(tdesc_i386_avx_mpx_avx512_pku_linux): Likewise.
	* x86-linux-nat.c (x86_linux_read_description): Call
	i386_linux_read_description.
---
 gdb/i386-linux-tdep.c | 34 ++++++++++++++++++++++------------
 gdb/i386-linux-tdep.h | 10 ++--------
 gdb/x86-linux-nat.c   | 24 ++++++++----------------
 3 files changed, 32 insertions(+), 36 deletions(-)
Simon Marchi - June 19, 2017, 9:26 p.m.
On 2017-06-12 10:41, Yao Qi wrote:
> This patch moves all the tdesc_i386*_linux target descriptions to a
> function i386_linux_read_description, which returns the right target
> description according to xcr0.  This also remove the duplication in
> getting target descriptions in corefile and native target.
> 
> gdb:
> 
> 2017-04-27  Yao Qi  <yao.qi@linaro.org>
> 
> 	* i386-linux-tdep.c (i386_linux_read_description): New function.
> 	(i386_linux_core_read_description): Call
> 	i386_linux_read_description.
> 	* i386-linux-tdep.h (i386_linux_read_description): Declare.
> 	(tdesc_i386_linux, tdesc_i386_mmx_linux): Remove declarations.
> 	(tdesc_i386_avx_linux, tdesc_i386_mpx_linux): Likewise
> 	(tdesc_i386_avx_mpx_linux, tdesc_i386_avx_avx512_linux): Likewise.
> 	(tdesc_i386_avx_mpx_avx512_pku_linux): Likewise.
> 	* x86-linux-nat.c (x86_linux_read_description): Call
> 	i386_linux_read_description.
> ---
>  gdb/i386-linux-tdep.c | 34 ++++++++++++++++++++++------------
>  gdb/i386-linux-tdep.h | 10 ++--------
>  gdb/x86-linux-nat.c   | 24 ++++++++----------------
>  3 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
> index 1909d61..1bc1a6f 100644
> --- a/gdb/i386-linux-tdep.c
> +++ b/gdb/i386-linux-tdep.c
> @@ -678,16 +678,9 @@ i386_linux_core_read_xcr0 (bfd *abfd)
>    return xcr0;
>  }
> 
> -/* Get Linux/x86 target description from core dump.  */
> -
> -static const struct target_desc *
> -i386_linux_core_read_description (struct gdbarch *gdbarch,
> -				  struct target_ops *target,
> -				  bfd *abfd)
> +const struct target_desc *
> +i386_linux_read_description (uint64_t xcr0)

This would need a comment

   /* See i386-linux-tdep.h.  */

>  {
> -  /* Linux/i386.  */
> -  uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
> -
>    switch ((xcr0 & X86_XSTATE_ALL_MASK))
>      {
>      case X86_XSTATE_AVX_MPX_AVX512_PKU_MASK:
> @@ -708,10 +701,27 @@ i386_linux_core_read_description (struct gdbarch 
> *gdbarch,
>        break;
>      }
> 
> +  return NULL;
> +}

I was going to say: I think it would make more sense if 
i386_linux_read_description always returned something non-NULL, but then 
I realized you need to handle the special case in 
i386_linux_core_read_description...

> +
> +/* Get Linux/x86 target description from core dump.  */
> +
> +static const struct target_desc *
> +i386_linux_core_read_description (struct gdbarch *gdbarch,
> +				  struct target_ops *target,
> +				  bfd *abfd)
> +{
> +  /* Linux/i386.  */
> +  uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
> +  const struct target_desc * tdesc = i386_linux_read_description 
> (xcr0);

Extra space after *.

The logic seems OK to me.  It's not related to your patch, but I find a 
bit confusing that i386-linux has SSE, and in i386-mmx-linux, the mmx is 
there to denote the lack of sse.  I looks like Mark agrees with me :)

   https://sourceware.org/ml/gdb-patches/2010-04/msg00300.html

Simon

Patch

diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 1909d61..1bc1a6f 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -678,16 +678,9 @@  i386_linux_core_read_xcr0 (bfd *abfd)
   return xcr0;
 }
 
-/* Get Linux/x86 target description from core dump.  */
-
-static const struct target_desc *
-i386_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+const struct target_desc *
+i386_linux_read_description (uint64_t xcr0)
 {
-  /* Linux/i386.  */
-  uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
-
   switch ((xcr0 & X86_XSTATE_ALL_MASK))
     {
     case X86_XSTATE_AVX_MPX_AVX512_PKU_MASK:
@@ -708,10 +701,27 @@  i386_linux_core_read_description (struct gdbarch *gdbarch,
       break;
     }
 
+  return NULL;
+}
+
+/* Get Linux/x86 target description from core dump.  */
+
+static const struct target_desc *
+i386_linux_core_read_description (struct gdbarch *gdbarch,
+				  struct target_ops *target,
+				  bfd *abfd)
+{
+  /* Linux/i386.  */
+  uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
+  const struct target_desc * tdesc = i386_linux_read_description (xcr0);
+
+  if (tdesc != NULL)
+    return tdesc;
+
   if (bfd_get_section_by_name (abfd, ".reg-xfp") != NULL)
-    return tdesc_i386_linux;
+    return i386_linux_read_description (X86_XSTATE_SSE_MASK);
   else
-    return tdesc_i386_mmx_linux;
+    return i386_linux_read_description (X86_XSTATE_X87_MASK);
 }
 
 /* Similar to i386_supply_fpregset, but use XSAVE extended state.  */
@@ -835,7 +845,7 @@  i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_num_regs (gdbarch, I386_LINUX_NUM_REGS);
 
   if (! tdesc_has_registers (tdesc))
-    tdesc = tdesc_i386_linux;
+    tdesc = i386_linux_read_description (X86_XSTATE_SSE_MASK);
   tdep->tdesc = tdesc;
 
   feature = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.linux");
diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h
index 8d4c7ca..1ca8d6b 100644
--- a/gdb/i386-linux-tdep.h
+++ b/gdb/i386-linux-tdep.h
@@ -42,14 +42,8 @@  extern uint64_t i386_linux_core_read_xcr0 (bfd *abfd);
 extern void i386_linux_handle_segmentation_fault (struct gdbarch *gdbarch,
 						  struct ui_out *uiout);
 
-/* Linux target description.  */
-extern struct target_desc *tdesc_i386_linux;
-extern struct target_desc *tdesc_i386_mmx_linux;
-extern struct target_desc *tdesc_i386_avx_linux;
-extern struct target_desc *tdesc_i386_mpx_linux;
-extern struct target_desc *tdesc_i386_avx_mpx_linux;
-extern struct target_desc *tdesc_i386_avx_avx512_linux;
-extern struct target_desc *tdesc_i386_avx_mpx_avx512_pku_linux;
+/* Return the target description according to XCR0.  */
+extern const struct target_desc *i386_linux_read_description (uint64_t xcr0);
 
 /* Format of XSAVE extended state is:
  	struct
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index 7362282..2c4afb1 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -153,7 +153,7 @@  x86_linux_read_description (struct target_ops *ops)
 	{
 	  have_ptrace_getfpxregs = 0;
 	  have_ptrace_getregset = TRIBOOL_FALSE;
-	  return tdesc_i386_mmx_linux;
+	  return i386_linux_read_description (X86_XSTATE_X87_MASK);
 	}
     }
 #endif
@@ -230,21 +230,13 @@  x86_linux_read_description (struct target_ops *ops)
     }
   else
     {
-      switch (xcr0_features_bits)
-	{
-	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;
-	default:
-	  return tdesc_i386_linux;
-	}
+      const struct target_desc * tdesc
+	= i386_linux_read_description (xcr0_features_bits);
+
+      if (tdesc == NULL)
+	tdesc = i386_linux_read_description (X86_XSTATE_SSE_MASK);
+
+      return tdesc;
     }
 
   gdb_assert_not_reached ("failed to return tdesc");