Move initialize_tdesc_mips* calls from mips-linux-nat.c to mips-linux-tdep.c

Message ID 1494324439-15918-1-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi May 9, 2017, 10:07 a.m. UTC
  Target description initialization should be called in -tdep.c, instead of
-nat.c.  Rebuild mips-linux native GDB.  Regression test is not run.

gdb:

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

	* mips-linux-nat.c: Move include features/mips*-linux.c to
	mips-linux-tdep.c.
	(_initialize_mips_linux_nat): Move initialize_tdesc_mips* calls
	to mips-linux-tdep.c.
	* mips-linux-tdep.c: Include features/mips*-linux.c
	(_initialize_mips_linux_tdep): Call initialize_tdesc_mips*
	functions.
	* mips-linux-tdep.h (tdesc_mips_linux): Declare.
	(tdesc_mips_dsp_linux, tdesc_mips64_linux): Declare.
	(tdesc_mips64_dsp_linux): Declare.
---
 gdb/mips-linux-nat.c  | 11 -----------
 gdb/mips-linux-tdep.c | 11 +++++++++++
 gdb/mips-linux-tdep.h |  6 ++++++
 3 files changed, 17 insertions(+), 11 deletions(-)
  

Comments

Maciej W. Rozycki May 10, 2017, 11:51 a.m. UTC | #1
On Tue, 9 May 2017, Yao Qi wrote:

> Target description initialization should be called in -tdep.c, instead of
> -nat.c.  Rebuild mips-linux native GDB.  Regression test is not run.

 Why?  These descriptions are only used in the native case, otherwise 
gdbserver supplies its own.  The current arrangement has worked for some 
12 years now.

  Maciej
  
John Baldwin May 10, 2017, 4:02 p.m. UTC | #2
On Wednesday, May 10, 2017 12:51:59 PM Maciej W. Rozycki wrote:
> On Tue, 9 May 2017, Yao Qi wrote:
> 
> > Target description initialization should be called in -tdep.c, instead of
> > -nat.c.  Rebuild mips-linux native GDB.  Regression test is not run.
> 
>  Why?  These descriptions are only used in the native case, otherwise 
> gdbserver supplies its own.  The current arrangement has worked for some 
> 12 years now.

Target descriptions (in general) might be determined purely by a core dump's
contents.  For example, in my out-of-tree patches for CHERI MIPS I added
target descriptions for the CHERI capability registers and then use those
target descriptions instead of the default for FreeBSD/mips core dumps that
contain a special "capregs" note (this is implemented via a gdbarch
"core_read_description" method).  For native binaries I depend on a working
ptrace op to determine if the native CPU supports the registers via
the "read_description" target method.

If core dumps of Linux binaries on processors with DSP registers included
those registers in process cores then you would need a similar method for
the Linux MIPS gdbarch that worked similar to mips_linux_read_description
to select the appropriate target description for process cores.

Another example of target descriptions used this way is on i386/amd64 for
both Linux and FreeBSD which use the register note with XSAVE registers
(and it's embedded value of %xcr0) to decide which target description to
use for plain SSE vs AVX vs MPX, etc.  Again, these checks are performed
both in the native target "read_description" method for live processes and
in the gdbarch "core_read_description" method for cores.

My guess is that Yao is just fixing this to follow the general rule that
target descriptions belong to the 'tdep' layer so that they can be used
by gdbarch "core_read_description" methods.
  
Yao Qi May 10, 2017, 4:16 p.m. UTC | #3
"Maciej W. Rozycki" <macro@imgtec.com> writes:

>  Why?  These descriptions are only used in the native case, otherwise 
> gdbserver supplies its own.  The current arrangement has worked for some 
> 12 years now.

Because I want these target descriptions be available in -tdep.c files
so that I can test them without running the test on a mips-linux
machine.

Long story is I changed the output of "maint print c-tdesc", and I add a
test to make sure these generated C files (by this command) is still
equivalent to the xml files.  If it is not clear to you, I can include
this patch in my whole patch series.
  
Maciej W. Rozycki May 16, 2017, 1:43 p.m. UTC | #4
On Wed, 10 May 2017, John Baldwin wrote:

> >  Why?  These descriptions are only used in the native case, otherwise 
> > gdbserver supplies its own.  The current arrangement has worked for some 
> > 12 years now.
> 
> Target descriptions (in general) might be determined purely by a core dump's
> contents.  For example, in my out-of-tree patches for CHERI MIPS I added
> target descriptions for the CHERI capability registers and then use those
> target descriptions instead of the default for FreeBSD/mips core dumps that
> contain a special "capregs" note (this is implemented via a gdbarch
> "core_read_description" method).  For native binaries I depend on a working
> ptrace op to determine if the native CPU supports the registers via
> the "read_description" target method.
> 
> If core dumps of Linux binaries on processors with DSP registers included
> those registers in process cores then you would need a similar method for
> the Linux MIPS gdbarch that worked similar to mips_linux_read_description
> to select the appropriate target description for process cores.

 Your explanation makes sense to me, although as you have also observed a 
target description corresponding to a core dump may not necessarily be the 
same as one produced for a live target that core dump has been obtained 
from, because for various reasons the lists of registers included in each 
may be different.  For these cases where there is no difference, I see no 
sense to duplicate code of course.

 But such details would have to be included in the description of a patch
proposed, IMHO.

  Maciej
  
Maciej W. Rozycki May 16, 2017, 2 p.m. UTC | #5
On Wed, 10 May 2017, Yao Qi wrote:

> >  Why?  These descriptions are only used in the native case, otherwise 
> > gdbserver supplies its own.  The current arrangement has worked for some 
> > 12 years now.
> 
> Because I want these target descriptions be available in -tdep.c files
> so that I can test them without running the test on a mips-linux
> machine.
> 
> Long story is I changed the output of "maint print c-tdesc", and I add a
> test to make sure these generated C files (by this command) is still
> equivalent to the xml files.  If it is not clear to you, I can include
> this patch in my whole patch series.

 Fair enough, although can you please add such details to the patch 
description (as it appears in the actual commit), so that also someone 
looking at it in several years' time have a chance to understand the 
change without the need to wade through mailing list archives?

 It is often the case that a change has to be understood long since it has 
been made and a clear description of the *intent* rather than just a terse 
ChangeLog entry or code modifications themselves is then invaluable, when 
the person who made it is no longer available to answer questions or may 
not remember anymore what exactly the change was about.

 If as you say this is a part of a set of changes, then I think it will be 
enough if you include such a description in the first change and then only 
refer to it by commit ID and heading from any subsequent changes, in 
addition to any change-specific details possibly required.

  Maciej
  

Patch

diff --git a/gdb/mips-linux-nat.c b/gdb/mips-linux-nat.c
index 8041d84..1fd3365 100644
--- a/gdb/mips-linux-nat.c
+++ b/gdb/mips-linux-nat.c
@@ -38,11 +38,6 @@ 
 
 #include "nat/mips-linux-watch.h"
 
-#include "features/mips-linux.c"
-#include "features/mips-dsp-linux.c"
-#include "features/mips64-linux.c"
-#include "features/mips64-dsp-linux.c"
-
 #ifndef PTRACE_GET_THREAD_AREA
 #define PTRACE_GET_THREAD_AREA 25
 #endif
@@ -803,10 +798,4 @@  triggers a breakpoint or watchpoint."),
 
   linux_nat_add_target (t);
   linux_nat_set_new_thread (t, mips_linux_new_thread);
-
-  /* Initialize the standard target descriptions.  */
-  initialize_tdesc_mips_linux ();
-  initialize_tdesc_mips_dsp_linux ();
-  initialize_tdesc_mips64_linux ();
-  initialize_tdesc_mips64_dsp_linux ();
 }
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 57e75b5..dc34242 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -40,6 +40,11 @@ 
 #include "xml-syscall.h"
 #include "gdb_signals.h"
 
+#include "features/mips-linux.c"
+#include "features/mips-dsp-linux.c"
+#include "features/mips64-linux.c"
+#include "features/mips64-dsp-linux.c"
+
 static struct target_so_ops mips_svr4_so_ops;
 
 /* This enum represents the signals' numbers on the MIPS
@@ -1774,4 +1779,10 @@  _initialize_mips_linux_tdep (void)
 			      GDB_OSABI_LINUX,
 			      mips_linux_init_abi);
     }
+
+  /* Initialize the standard target descriptions.  */
+  initialize_tdesc_mips_linux ();
+  initialize_tdesc_mips_dsp_linux ();
+  initialize_tdesc_mips64_linux ();
+  initialize_tdesc_mips64_dsp_linux ();
 }
diff --git a/gdb/mips-linux-tdep.h b/gdb/mips-linux-tdep.h
index 407b577..cca4798 100644
--- a/gdb/mips-linux-tdep.h
+++ b/gdb/mips-linux-tdep.h
@@ -105,3 +105,9 @@  enum {
 /* Return 1 if MIPS_RESTART_REGNUM is usable.  */
 
 int mips_linux_restart_reg_p (struct gdbarch *gdbarch);
+
+/* Target descriptions.  */
+extern struct target_desc *tdesc_mips_linux;
+extern struct target_desc *tdesc_mips64_linux;
+extern struct target_desc *tdesc_mips_dsp_linux;
+extern struct target_desc *tdesc_mips64_dsp_linux;