[PATCH 2/3] Trust readonly sections if target has memory protection

[PATCH 2/3] Trust readonly sections if target has memory protection

[Yao Qi]

This patch first changes command "trust-readonly-sections" to an
auto_boolean command, so "auto" means that GDB trusts read-only
sections if the target has memory protection.  Then, this patch adds a
gdbarch hook method "has_memory_protection".  Patch 2/2 is to
implement the hook method for linux target.

gdb:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* arch-utils.c (default_has_memory_protection): New function.
	* arch-utils.h (default_has_memory_protection): Declaration.
	* gdbarch.sh (has_memory_protection): New hook method.
	* gdbarch.c: Re-generated.
	* gdbarch.h: Re-generated.
	* target.c (trust_readonly): Change type to 'enum auto_boolean'
	and initialize it to 'AUTO_BOOLEAN_AUTO'.
	(trust_readonly_p): New function.
	(memory_xfer_partial_1): Call trust_readonly_p.
	(initialize_targets): Register command
	"trust-readonly-sections" as add_setshow_auto_boolean_cmd.

	* NEWS: Describe the default option of
	"trust-readonly-sections" becomes "auto" and the related
	changes.

gdb/doc:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (Files): Explain the default option of
	"trust-readonly-sections" is "auto".
---
 gdb/NEWS            |    4 ++++
 gdb/arch-utils.c    |    7 +++++++
 gdb/arch-utils.h    |    2 ++
 gdb/doc/gdb.texinfo |    6 +++++-
 gdb/gdbarch.c       |   24 ++++++++++++++++++++++++
 gdb/gdbarch.h       |    6 ++++++
 gdb/gdbarch.sh      |    3 +++
 gdb/target.c        |   32 +++++++++++++++++++++++---------
 8 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index ad97f6f..a3bdf84 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@
 
 *** Changes since GDB 7.6
 
+* The default value of option "trust-readonly-sections" is "auto".  GDB
+  trusts the contents of read-only sections from the object file on the
+  GNU/Linux targets.
+
 * The "maintenance print objfiles" command now takes an optional regexp.
 
 * The "catch syscall" command now works on arm*-linux* targets.
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 459fd88..8418bbc 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -769,6 +769,13 @@ default_has_shared_address_space (struct gdbarch *gdbarch)
 }
 
 int
+default_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Simply say no.  */
+  return 0;
+}
+
+int
 default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
 				  CORE_ADDR addr, int *isize, char **msg)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 3f0e64f..61973c0 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -153,6 +153,8 @@ extern struct gdbarch *get_current_arch (void);
 
 extern int default_has_shared_address_space (struct gdbarch *);
 
+extern int default_has_memory_protection (struct gdbarch *gdbarch);
+
 extern int default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
 					     CORE_ADDR addr,
 					     int *isize, char **msg);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d8e4adf..5efa966 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -16673,7 +16673,11 @@ out of the object file, rather than from the target program.
 For some targets (notably embedded ones), this can be a significant
 enhancement to debugging performance.
 
-The default is off.
+@item set trust-readonly-sections auto
+This is the default mode.  Tell @value{GDBN} to trust read-only
+sections on some targets which have memory protection, such as
+GNU/Linux, because the contents of the section in the target program
+can't change.
 
 @item set trust-readonly-sections off
 Tell @value{GDBN} not to trust readonly sections.  This means that
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 1f3380e..13e5e82 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -278,6 +278,7 @@ struct gdbarch
   int has_global_solist;
   int has_global_breakpoints;
   gdbarch_has_shared_address_space_ftype *has_shared_address_space;
+  gdbarch_has_memory_protection_ftype *has_memory_protection;
   gdbarch_fast_tracepoint_valid_at_ftype *fast_tracepoint_valid_at;
   gdbarch_auto_charset_ftype *auto_charset;
   gdbarch_auto_wide_charset_ftype *auto_wide_charset;
@@ -451,6 +452,7 @@ struct gdbarch startup_gdbarch =
   0,  /* has_global_solist */
   0,  /* has_global_breakpoints */
   default_has_shared_address_space,  /* has_shared_address_space */
+  default_has_memory_protection,  /* has_memory_protection */
   default_fast_tracepoint_valid_at,  /* fast_tracepoint_valid_at */
   default_auto_charset,  /* auto_charset */
   default_auto_wide_charset,  /* auto_wide_charset */
@@ -546,6 +548,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->displaced_step_location = NULL;
   gdbarch->relocate_instruction = NULL;
   gdbarch->has_shared_address_space = default_has_shared_address_space;
+  gdbarch->has_memory_protection = default_has_memory_protection;
   gdbarch->fast_tracepoint_valid_at = default_fast_tracepoint_valid_at;
   gdbarch->auto_charset = default_auto_charset;
   gdbarch->auto_wide_charset = default_auto_wide_charset;
@@ -757,6 +760,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of has_global_solist, invalid_p == 0 */
   /* Skip verify of has_global_breakpoints, invalid_p == 0 */
   /* Skip verify of has_shared_address_space, invalid_p == 0 */
+  /* Skip verify of has_memory_protection, invalid_p == 0 */
   /* Skip verify of fast_tracepoint_valid_at, invalid_p == 0 */
   /* Skip verify of auto_charset, invalid_p == 0 */
   /* Skip verify of auto_wide_charset, invalid_p == 0 */
@@ -1078,6 +1082,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: has_global_solist = %s\n",
                       plongest (gdbarch->has_global_solist));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: has_memory_protection = <%s>\n",
+                      host_address_to_string (gdbarch->has_memory_protection));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: has_shared_address_space = <%s>\n",
                       host_address_to_string (gdbarch->has_shared_address_space));
   fprintf_unfiltered (file,
@@ -4240,6 +4247,23 @@ set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch,
 }
 
 int
+gdbarch_has_memory_protection (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->has_memory_protection != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_has_memory_protection called\n");
+  return gdbarch->has_memory_protection (gdbarch);
+}
+
+void
+set_gdbarch_has_memory_protection (struct gdbarch *gdbarch,
+                                   gdbarch_has_memory_protection_ftype has_memory_protection)
+{
+  gdbarch->has_memory_protection = has_memory_protection;
+}
+
+int
 gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 013f071..ade07d7 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1176,6 +1176,12 @@ typedef int (gdbarch_has_shared_address_space_ftype) (struct gdbarch *gdbarch);
 extern int gdbarch_has_shared_address_space (struct gdbarch *gdbarch);
 extern void set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch, gdbarch_has_shared_address_space_ftype *has_shared_address_space);
 
+/* True if the target has memory protection. */
+
+typedef int (gdbarch_has_memory_protection_ftype) (struct gdbarch *gdbarch);
+extern int gdbarch_has_memory_protection (struct gdbarch *gdbarch);
+extern void set_gdbarch_has_memory_protection (struct gdbarch *gdbarch, gdbarch_has_memory_protection_ftype *has_memory_protection);
+
 /* True if a fast tracepoint can be set at an address. */
 
 typedef int (gdbarch_fast_tracepoint_valid_at_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 04ca38c..080a5ec 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -932,6 +932,9 @@ v:int:has_global_breakpoints:::0:0::0
 # True if inferiors share an address space (e.g., uClinux).
 m:int:has_shared_address_space:void:::default_has_shared_address_space::0
 
+# True if the target has memory protection.
+m:int:has_memory_protection:void:::default_has_memory_protection::0
+
 # True if a fast tracepoint can be set at an address.
 m:int:fast_tracepoint_valid_at:CORE_ADDR addr, int *isize, char **msg:addr, isize, msg::default_fast_tracepoint_valid_at::0
 
diff --git a/gdb/target.c b/gdb/target.c
index d55712d..80f1f87 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -168,10 +168,10 @@ struct target_ops current_target;
 
 static struct cmd_list_element *targetlist = NULL;
 
-/* Nonzero if we should trust readonly sections from the
+/* If AUTO_BOOLEAN_TRUE, we should trust readonly sections from the
    executable when reading memory.  */
 
-static int trust_readonly = 0;
+static enum auto_boolean trust_readonly = AUTO_BOOLEAN_AUTO;
 
 /* Nonzero if we should show true memory content including
    memory breakpoint inserted by gdb.  */
@@ -1437,6 +1437,18 @@ memory_xfer_live_readonly_partial (struct target_ops *ops,
   return 0;
 }
 
+/* Return true if GDB trusts readonly sections.  Return false
+   otherwise.  */
+
+static int
+trust_readonly_p (void)
+{
+  if (trust_readonly != AUTO_BOOLEAN_AUTO)
+    return trust_readonly == AUTO_BOOLEAN_TRUE;
+
+  return gdbarch_has_memory_protection (target_gdbarch ());
+}
+
 /* Perform a partial memory transfer.
    For docs see target.h, to_xfer_partial.  */
 
@@ -1472,7 +1484,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
     }
 
   /* Try the executable files, if "trust-readonly-sections" is set.  */
-  if (readbuf != NULL && trust_readonly)
+  if (readbuf != NULL && trust_readonly_p ())
     {
       struct target_section *secp;
       struct target_section_table *table;
@@ -5086,16 +5098,18 @@ command."),
 			     show_targetdebug,
 			     &setdebuglist, &showdebuglist);
 
-  add_setshow_boolean_cmd ("trust-readonly-sections", class_support,
-			   &trust_readonly, _("\
+  add_setshow_auto_boolean_cmd ("trust-readonly-sections", class_support,
+				&trust_readonly, _("\
 Set mode for reading from readonly sections."), _("\
 Show mode for reading from readonly sections."), _("\
 When this mode is on, memory reads from readonly sections (such as .text)\n\
 will be read from the object file instead of from the target.  This will\n\
-result in significant performance improvement for remote targets."),
-			   NULL,
-			   show_trust_readonly,
-			   &setlist, &showlist);
+result in significant performance improvement for remote targets.\n\
+When this mode is auto, memory reads from readonly sections will be\n\
+read from the object file if the target has memory protection.\n"),
+				NULL,
+				show_trust_readonly,
+				&setlist, &showlist);
 
   add_com ("monitor", class_obscure, do_monitor_command,
 	   _("Send a command to the remote monitor (remote targets only)."));
-- 
1.7.7.6

[PATCH 0/3] Trust readonly sections if target has memory protection

[Yao Qi]

Hi,
When option "trust-readonly-sections" was introduced in this patch
http://www.sourceware.org/ml/gdb-patches/2002-01/msg00711.html, there
was the discussion to turn it on in default.  However, we didn't do
that because we have a concern that bad or buggy program may modify the
code on the non-memory protected system.  On the other hand, it is
highly recommended to turn this on in remote debugging.

I am wondering if we can teach GDB to trust read-only sections if it
knows the target system has memory protection.  For some targets, such
as linux, we know they do have memory protection.  This is what this
patch series tries to do.

This change improves GDB's out-of-box performance.  I wrote a
micro-benchmark to disassemble GDB in GDB in remote debugging
(native-gdbserver) on x86-linux, and collect the time usage
and the number of 'm' packets,

                    /wo patch     /w patch

time usage    :       0.44s        0.10
number of 'm' :       43157         4

As we can see, the number of RSP 'm' packet is reduced dramatically,
and the time usage is reduced to some extent.  In my experiment,
GDBserver is running natively, so connection speed is quite fast.  In
some slow connections, the improvement will be more.

This patch series only teach GDB to trust readonly sections on
GNU/Linux targets.  Some ancient Windows targets don't have memory
protection, so we can't let GDB to rust readoly sections on mingw
and cygwin target.

*** BLURB HERE ***

Yao Qi (3):
  set trust-readonly-sections off in test cases
  Trust readonly sections if target has memory protection
  Linux has memory protection.

 gdb/NEWS                                |    4 ++++
 gdb/arch-utils.c                        |    7 +++++++
 gdb/arch-utils.h                        |    2 ++
 gdb/doc/gdb.texinfo                     |   11 +++++++++--
 gdb/gdbarch.c                           |   24 ++++++++++++++++++++++++
 gdb/gdbarch.h                           |    6 ++++++
 gdb/gdbarch.sh                          |    3 +++
 gdb/linux-tdep.c                        |   10 ++++++++++
 gdb/target.c                            |   28 ++++++++++++++++++++--------
 gdb/testsuite/gdb.base/break-always.exp |    4 ++++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp |    5 +++++
 11 files changed, 94 insertions(+), 10 deletions(-)

-- 
1.7.7.6

[PATCH 1/3] set trust-readonly-sections off in test cases

[Yao Qi]

In the test cases, if GDB modified code in the inferior, it should
'set trust-readonly-sections off' too.  Similarly, users has to set it
off if users modified code for some purpose.

gdb/testsuite:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* gdb.base/break-always.exp: Set trust-readonly-sections off.
	* gdb.mi/mi-fill-memory.exp: Likewise.

gdb/doc:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (File): Mention that user has
	'set trust-readonly-sections off' to get updated contents if
	they are modified.
---
 gdb/doc/gdb.texinfo                     |    5 ++++-
 gdb/testsuite/gdb.base/break-always.exp |    4 ++++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp |    5 +++++
 3 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 21250fe..d8e4adf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -16678,7 +16678,10 @@ The default is off.
 @item set trust-readonly-sections off
 Tell @value{GDBN} not to trust readonly sections.  This means that
 the contents of the section might change while the program is running,
-and must therefore be fetched from the target when needed.
+and must therefore be fetched from the target when needed.  If user
+modified the code in the target program, user has to
+@code{set trust-readonly-sections off} to guarantee @value{GDBN} reads
+updated contents from the target program instead of object file.
 
 @item show trust-readonly-sections
 Show the current setting of trusting readonly sections.
diff --git a/gdb/testsuite/gdb.base/break-always.exp b/gdb/testsuite/gdb.base/break-always.exp
index b0a9faf..16f6d47 100644
--- a/gdb/testsuite/gdb.base/break-always.exp
+++ b/gdb/testsuite/gdb.base/break-always.exp
@@ -69,6 +69,10 @@ gdb_test "p /x \$shadow = *(char *) $bp_address" \
 # and still leave the breakpoint insn planted.  Try twice with
 # different values, in case we happen to be writting exactly what was
 # there already.
+# Turn option "trust-readonly-sections" off to force GDB to read
+# contents from the target instead of the object file.
+gdb_test_no_output "set trust-readonly-sections off"
+
 gdb_test "p /x *(char *) $bp_address = 0" \
     " = 0x0" \
     "write 0 to breakpoint's address"
diff --git a/gdb/testsuite/gdb.mi/mi-fill-memory.exp b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
index 5f68cbb..ac9313b 100644
--- a/gdb/testsuite/gdb.mi/mi-fill-memory.exp
+++ b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
@@ -51,6 +51,11 @@ mi_gdb_test "4-data-write-memory-bytes \$pc ab 8"\
 	"4\\\^done" \
 	"memory successfully filled (8 bytes)"
 
+# Turn option "trust-readonly-sections" off to force GDB to read
+# contents from the target instead of the object file.
+mi_gdb_test "-gdb-set trust-readonly-sections off" "\\\^done" \
+    "set trust-readonly-sections off"
+
 mi_gdb_test "5-interpreter-exec console \"x \$pc\"" \
     ".*0xabababab.*" \
     "pattern correctly read from memory"
-- 
1.7.7.6

[PATCH 3/3] Linux has memory protection.

[Yao Qi]

gdb:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* linux-tdep.c (linux_has_memory_protection): New function.
	(linux_init_abi): Register linux_has_memory_protection
	to gdbarch 'has_memory_protection'.
---
 gdb/linux-tdep.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index eb8ea2b..36e7119 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -259,6 +259,15 @@ linux_has_shared_address_space (struct gdbarch *gdbarch)
   return linux_is_uclinux ();
 }
 
+/* This is the implementation of gdbarch method has_memory_protection.  */
+
+static int
+linux_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Normal linux has memory protection, while uClinux doesn't.  */
+  return !linux_is_uclinux ();
+}
+
 /* This is how we want PTIDs from core files to be printed.  */
 
 static char *
@@ -1783,6 +1792,7 @@ linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 				      linux_gdb_signal_from_target);
   set_gdbarch_gdb_signal_to_target (gdbarch,
 				    linux_gdb_signal_to_target);
+  set_gdbarch_has_memory_protection (gdbarch, linux_has_memory_protection);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
-- 
1.7.7.6

Re: [PATCH 1/3] set trust-readonly-sections off in test cases

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 6 Sep 2013 10:01:58 +0800
> 
> 2013-09-06  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (File): Mention that user has
> 	'set trust-readonly-sections off' to get updated contents if
> 	they are modified.

Thanks.

>  @item set trust-readonly-sections off
>  Tell @value{GDBN} not to trust readonly sections.  This means that
>  the contents of the section might change while the program is running,
> -and must therefore be fetched from the target when needed.
> +and must therefore be fetched from the target when needed.  If user
> +modified the code in the target program, user has to
> +@code{set trust-readonly-sections off} to guarantee @value{GDBN} reads
> +updated contents from the target program instead of object file.

Please don't talk about the user in the "third-person" style.  Please
use "you" instead.

Also, if this patch series turns the flag on, we should change the
sentence above this text that says default is off, and announce that
in NEWS.

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 6 Sep 2013 10:01:57 +0800
> 
> This patch series only teach GDB to trust readonly sections on
> GNU/Linux targets.  Some ancient Windows targets don't have memory
> protection, so we can't let GDB to rust readoly sections on mingw
> and cygwin target.

Which "ancient" Windows versions are those?  Can't we decide at run
time whether to trust read-only sections, by looking at the Windows
version?

Re: [PATCH 2/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 6 Sep 2013 10:01:59 +0800
> 
> This patch first changes command "trust-readonly-sections" to an
> auto_boolean command, so "auto" means that GDB trusts read-only
> sections if the target has memory protection.  Then, this patch adds a
> gdbarch hook method "has_memory_protection".  Patch 2/2 is to
> implement the hook method for linux target.
> 
> gdb:
> 
> 2013-09-06  Yao Qi  <yao@codesourcery.com>
> 
> 	* arch-utils.c (default_has_memory_protection): New function.
> 	* arch-utils.h (default_has_memory_protection): Declaration.
> 	* gdbarch.sh (has_memory_protection): New hook method.
> 	* gdbarch.c: Re-generated.
> 	* gdbarch.h: Re-generated.
> 	* target.c (trust_readonly): Change type to 'enum auto_boolean'
> 	and initialize it to 'AUTO_BOOLEAN_AUTO'.
> 	(trust_readonly_p): New function.
> 	(memory_xfer_partial_1): Call trust_readonly_p.
> 	(initialize_targets): Register command
> 	"trust-readonly-sections" as add_setshow_auto_boolean_cmd.
> 
> 	* NEWS: Describe the default option of
> 	"trust-readonly-sections" becomes "auto" and the related
> 	changes.
> 
> gdb/doc:
> 
> 2013-09-06  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (Files): Explain the default option of
> 	"trust-readonly-sections" is "auto".

Is it possible NOT to split documentation changes between changesets,
when they are all parts of the same patch series?  Reviewing changes
piecemeal like that is extremely inconvenient and error-prone,
especially since you never tell in the beginning that the
documentation changes are split.  TIA.

>  *** Changes since GDB 7.6
>  
> +* The default value of option "trust-readonly-sections" is "auto".  GDB
> +  trusts the contents of read-only sections from the object file on the
> +  GNU/Linux targets.

The second sentence is in contradiction with the first.  "Auto" means
GDB decides automatically whether to trust these section; it does not
mean the decision is YES for GNU/Linux and NO otherwise.

> +@item set trust-readonly-sections auto
> +This is the default mode.  Tell @value{GDBN} to trust read-only
> +sections on some targets which have memory protection, such as
> +GNU/Linux, because the contents of the section in the target program
> +can't change.

Again, no need to mention one platform here.  Such references quickly
become obsolete with time.

> +When this mode is auto, memory reads from readonly sections will be\n\
> +read from the object file if the target has memory protection.\n"),

I would suggest

  When this mode is auto, GDB will decide based on the target memory
  protection features whether to read readonly sections from object file
  instead of from the inferior's memory.

IOW, "auto" means GDB will decide by its own internal logic.

Thanks.

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/06/2013 01:57 PM, Eli Zaretskii wrote:
> Which "ancient" Windows versions are those?  Can't we decide at run
> time whether to trust read-only sections, by looking at the Windows
> version?

I meant Windows 3.0 and 9x, which don't have a full memory protection,
AFAIK.

http://en.wikipedia.org/wiki/Memory_protection tells me that "Windows 
family from Windows NT 3.1" have memory protection.

Yes, we can extend osdata mechanism to get os version, and decide 
whether to trust read-only sections.

-- 
Yao (齐尧)

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> Date: Fri, 6 Sep 2013 16:23:27 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 09/06/2013 01:57 PM, Eli Zaretskii wrote:
> > Which "ancient" Windows versions are those?  Can't we decide at run
> > time whether to trust read-only sections, by looking at the Windows
> > version?
> 
> I meant Windows 3.0 and 9x, which don't have a full memory protection,
> AFAIK.

MinGW doesn't support Windows 3.x, and I think Cygwin doesn't support
9x anymore.

Re: [PATCH 2/3] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/06/2013 02:05 PM, Eli Zaretskii wrote:
> Is it possible NOT to split documentation changes between changesets,
> when they are all parts of the same patch series?  Reviewing changes
> piecemeal like that is extremely inconvenient and error-prone,
> especially since you never tell in the beginning that the
> documentation changes are split.  TIA.
> 

OK.  I re-org the patches and move all doc-related changes into one
patch.

>> >  *** Changes since GDB 7.6
>> >  
>> >+* The default value of option "trust-readonly-sections" is "auto".  GDB
>> >+  trusts the contents of read-only sections from the object file on the
>> >+  GNU/Linux targets.
> The second sentence is in contradiction with the first.  "Auto" means
> GDB decides automatically whether to trust these section; it does not
> mean the decision is YES for GNU/Linux and NO otherwise.
> 

The patch below addresses all your comments on doc.

-- 
Yao (齐尧)

gdb:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* NEWS: Describe the default option of
	"trust-readonly-sections" becomes "auto" and the related
	changes.

gdb/doc:

2013-09-06  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (File): Mention that user has
	'set trust-readonly-sections off' to get updated contents if
	they are modified.  Explain the default option of
	"trust-readonly-sections" is "auto".
---
 gdb/NEWS            |    5 +++++
 gdb/doc/gdb.texinfo |   11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index ad97f6f..e60a553 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,11 @@
 
 *** Changes since GDB 7.6
 
+* The default value of option "trust-readonly-sections" is "auto".  GDB
+  will decide based on the target memory protection features whether to
+  read readonly sections from object file instead of from the inferior's
+  memory.
+
 * The "maintenance print objfiles" command now takes an optional regexp.
 
 * The "catch syscall" command now works on arm*-linux* targets.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 21250fe..cd7859a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -16673,12 +16673,19 @@ out of the object file, rather than from the target program.
 For some targets (notably embedded ones), this can be a significant
 enhancement to debugging performance.
 
-The default is off.
+@item set trust-readonly-sections auto
+This is the default mode.  @value{GDBN} will decide based on the
+target memory protection features whether to read readonly sections
+from object file instead of from the inferior's memory, because the
+contents of the section in the inferior can't change.
 
 @item set trust-readonly-sections off
 Tell @value{GDBN} not to trust readonly sections.  This means that
 the contents of the section might change while the program is running,
-and must therefore be fetched from the target when needed.
+and must therefore be fetched from the target when needed.  If you
+modified the code in the target program, you have to
+@code{set trust-readonly-sections off} to guarantee @value{GDBN} reads
+updated contents from the target program instead of object file.
 
 @item show trust-readonly-sections
 Show the current setting of trusting readonly sections.
-- 
1.7.7.6

Re: [PATCH 2/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> Date: Fri, 6 Sep 2013 17:06:27 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 09/06/2013 02:05 PM, Eli Zaretskii wrote:
> > Is it possible NOT to split documentation changes between changesets,
> > when they are all parts of the same patch series?  Reviewing changes
> > piecemeal like that is extremely inconvenient and error-prone,
> > especially since you never tell in the beginning that the
> > documentation changes are split.  TIA.
> > 
> 
> OK.  I re-org the patches and move all doc-related changes into one
> patch.

Not needed for this patch, only for the future.  Thanks.

> The patch below addresses all your comments on doc.

OK.

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Joel Brobecker]

> This patch series only teach GDB to trust readonly sections on
> GNU/Linux targets.  Some ancient Windows targets don't have memory
> protection, so we can't let GDB to rust readoly sections on mingw
> and cygwin target.

Do you know which ancient versions of Windows are we talking about?
If known, we could document that somewhere in the code, allowing us
to also enable it on Windows when we decide to stop supporting those
versions.

-- 
Joel

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Joel Brobecker]

> MinGW doesn't support Windows 3.x, and I think Cygwin doesn't support
> 9x anymore.

IMO, XP is probably the most ancient version that would be reasonable
to support. Are people still developping on more ancient versions?

-- 
Joel

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/06/2013 09:03 PM, Joel Brobecker wrote:
>> MinGW doesn't support Windows 3.x, and I think Cygwin doesn't support
>> >9x anymore.
> IMO, XP is probably the most ancient version that would be reasonable
> to support. Are people still developping on more ancient versions?

I don't know.  Then, we can safely think Windows host has full memory
protection.  I'll install has_memory_protection gdbarch hook in
gdb/windows-tdep.c too.  It will be included in the V2 of this patch
series.

-- 
Yao (齐尧)

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> Date: Fri, 6 Sep 2013 06:03:32 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Yao Qi <yao@codesourcery.com>, gdb-patches@sourceware.org
> 
> > MinGW doesn't support Windows 3.x, and I think Cygwin doesn't support
> > 9x anymore.
> 
> IMO, XP is probably the most ancient version that would be reasonable
> to support. Are people still developping on more ancient versions?

If we support XP, extending support to Windows 2000 or even NT 4.0
comes almost at no cost.  IMO, there's no need to drop support for
platforms whose support doesn't impose any tangible maintenance
headaches.

Windows 9X can be easily recognized at run time, and the feature that
is the subject of this thread can be turned off then.

RE: [PATCH 0/3] Trust readonly sections if target has memory protection

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé : vendredi 6 septembre 2013 15:32
> À : Joel Brobecker
> Cc : yao@codesourcery.com; gdb-patches@sourceware.org
> Objet : Re: [PATCH 0/3] Trust readonly sections if target has memory
> protection
> 
> > Date: Fri, 6 Sep 2013 06:03:32 -0700
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: Yao Qi <yao@codesourcery.com>, gdb-patches@sourceware.org
> >
> > > MinGW doesn't support Windows 3.x, and I think Cygwin doesn't support
> > > 9x anymore.
> >
> > IMO, XP is probably the most ancient version that would be reasonable
> > to support. Are people still developping on more ancient versions?

  I have a question:
if Windows OS is supposed to support memory protection,
then why is it allowed to set software interrupts?
  We do overwrite the .text section of the debuggee to do this, no?

  Does this simply mean that the program itself would not be allowed
to modify its own .text section (or any other read-only section), 
but that the debugger has a higher privilege, which allows him 
to overwrite read-only sections...

  If this is true, does it mean that if we "set trust-readonly-sections
auto" 
and use the debugger to overwrite any memory in READ_ONLY section,
and read it back subsequently, it will still display the unmodified memory?

  Is the "feature/problem" limited to use of gdbserver?

  Is the behavior the same on Linux systems?

 Pierre Muller

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Date: Fri, 6 Sep 2013 16:17:34 +0200
> 
>   I have a question:
> if Windows OS is supposed to support memory protection,
> then why is it allowed to set software interrupts?

What do you mean by "set software interrupts", in the context of
Windows debugging?

>   We do overwrite the .text section of the debuggee to do this, no?

I don't think so, but maybe I misunderstand what you mean.

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Joel Brobecker]

> > IMO, XP is probably the most ancient version that would be reasonable
> > to support. Are people still developping on more ancient versions?
> 
> If we support XP, extending support to Windows 2000 or even NT 4.0
> comes almost at no cost.  IMO, there's no need to drop support for
> platforms whose support doesn't impose any tangible maintenance
> headaches.
> 
> Windows 9X can be easily recognized at run time, and the feature that
> is the subject of this thread can be turned off then.

Perhaps a compromise: For the new feature to fail, it'd need an
extremely ancient version of Windows, coupled with a bad program
that writes outside of its memory area. How about we forgo the
Windows 9x detection, and just enable the feature on Windows
unconditionally? This removes the burden from Yao, who shouldn't
be asked to do this work unless he wants to. Then, if someone
believes it's necessary to turn the feature off by default on
old versions of Windows, it should be easy for them to do so.

-- 
Joel

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> Date: Fri, 6 Sep 2013 07:52:05 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: yao@codesourcery.com, gdb-patches@sourceware.org
> 
> Perhaps a compromise: For the new feature to fail, it'd need an
> extremely ancient version of Windows, coupled with a bad program
> that writes outside of its memory area. How about we forgo the
> Windows 9x detection, and just enable the feature on Windows
> unconditionally? This removes the burden from Yao, who shouldn't
> be asked to do this work unless he wants to. Then, if someone
> believes it's necessary to turn the feature off by default on
> old versions of Windows, it should be easy for them to do so.

Fine with me, but then why not enable this on all platforms?

Re: [PATCH 1/3] set trust-readonly-sections off in test cases

[Pedro Alves]

On 09/06/2013 03:01 AM, Yao Qi wrote:
> In the test cases, if GDB modified code in the inferior, it should
> 'set trust-readonly-sections off' too.  Similarly, users has to set it
> off if users modified code for some purpose.

Hmm, I think this is likely to confuse users that actually
do that, a lot.

One idea would be to either warn about, or even disallow user
initiated writes to readonly sections if trust-readonly-sections
is enabled.

But a better one would be to mark a written section (or memory
block/range) as "tainted", and then trust-readonly-sections would
not trust it anymore.

-- 
Pedro Alves

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Joel Brobecker]

> > Perhaps a compromise: For the new feature to fail, it'd need an
> > extremely ancient version of Windows, coupled with a bad program
> > that writes outside of its memory area. How about we forgo the
> > Windows 9x detection, and just enable the feature on Windows
> > unconditionally? This removes the burden from Yao, who shouldn't
> > be asked to do this work unless he wants to. Then, if someone
> > believes it's necessary to turn the feature off by default on
> > old versions of Windows, it should be easy for them to do so.
> 
> Fine with me, but then why not enable this on all platforms?

Not all platforms are in the particular situation I was describing.

-- 
Joel

Re: [PATCH 0/3] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> Date: Fri, 6 Sep 2013 11:10:15 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: yao@codesourcery.com, gdb-patches@sourceware.org
> 
> > > Perhaps a compromise: For the new feature to fail, it'd need an
> > > extremely ancient version of Windows, coupled with a bad program
> > > that writes outside of its memory area. How about we forgo the
> > > Windows 9x detection, and just enable the feature on Windows
> > > unconditionally? This removes the burden from Yao, who shouldn't
> > > be asked to do this work unless he wants to. Then, if someone
> > > believes it's necessary to turn the feature off by default on
> > > old versions of Windows, it should be easy for them to do so.
> > 
> > Fine with me, but then why not enable this on all platforms?
> 
> Not all platforms are in the particular situation I was describing.

Which platforms don't support memory protection of the kind we discuss
here?

[PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true

[Yao Qi]

If users write a readonly section, such as .text, the contents of the inferior
and of the executable become out of sync.  It is better to emit a warning
to ask users to "set trust-readonly-sections off".

(gdb) set trust-readonly-sections on
(gdb) p /x* (char *) 0x080484c1 = 0xcc
warning: Writing to a readonly section so that the contents in the
inferior and in the executable are out of sync.  Please 'set
trust-readonly-sections off'.
$1 = 0xcc

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* target.c (memory_xfer_partial_1): Emit a warning if GDB
	writes to a readonly section and 'trust_readonly' is true.
---
 gdb/target.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index d55712d..f6a58ff 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1471,11 +1471,13 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	}
     }
 
-  /* Try the executable files, if "trust-readonly-sections" is set.  */
-  if (readbuf != NULL && trust_readonly)
+  /* If "trust-readonly-sections" is set, read from the executable
+     files.  When GDB writes to a readonly section of the inferior,
+     emit a warning that option 'trust-readonly-sections' should be
+     turned off.  */
+  if (trust_readonly)
     {
       struct target_section *secp;
-      struct target_section_table *table;
 
       secp = target_section_by_addr (ops, memaddr);
       if (secp != NULL
@@ -1483,12 +1485,23 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 				     secp->the_bfd_section)
 	      & SEC_READONLY))
 	{
-	  table = target_get_section_table (ops);
-	  return section_table_xfer_memory_partial (readbuf, writebuf,
-						    memaddr, len,
-						    table->sections,
-						    table->sections_end,
-						    NULL);
+
+	  if (readbuf != NULL)
+	    {
+	      struct target_section_table *table;
+
+	      table = target_get_section_table (ops);
+	      return section_table_xfer_memory_partial (readbuf, writebuf,
+							memaddr, len,
+							table->sections,
+							table->sections_end,
+							NULL);
+	    }
+
+	  if (writebuf != NULL)
+	    warning (_("Writing to a readonly section so that the contents"
+		       " in the inferior and in the executable are out of"
+		       " sync.  Please 'set trust-readonly-sections off'."));
 	}
     }
 
-- 
1.7.7.6

[PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Yao Qi]

Here is the V2, to address comments to V1,

  - Warn about writes to readonly sections if trust-readonly-sections
is enabled. (patch 1/7)
  - Windows targets have memory protection. (patch 7/7)

Do some refactor in Windows related targets, add a new function
windows_init_abi (done by patch 3/7).  It paves the way for patch
7/7.

Below the description of this series in V1:
https://sourceware.org/ml/gdb-patches/2013-09/msg00193.html

When option "trust-readonly-sections" was introduced in this patch
http://www.sourceware.org/ml/gdb-patches/2002-01/msg00711.html, there
was the discussion to turn it on in default.  However, we didn't do
that because we have a concern that bad or buggy program may modify the
code on the non-memory protected system.  On the other hand, it is
highly recommended to turn this on in remote debugging.

I am wondering if we can teach GDB to trust read-only sections if it
knows the target system has memory protection.  For some targets, such
as linux, we know they do have memory protection.  This is what this
patch series tries to do.

This change improves GDB's out-of-box performance.  I wrote a
micro-benchmark to disassemble GDB in GDB in remote debugging
(native-gdbserver) on x86-linux, and collect the time usage
and the number of 'm' packets,

                    /wo patch     /w patch

time usage    :       0.44s        0.10
number of 'm' :       43157         4

As we can see, the number of RSP 'm' packet is reduced dramatically,
and the time usage is reduced to some extent.  In my experiment,
GDBserver is running natively, so connection speed is quite fast.  In
some slow connections, the improvement will be more.

*** BLURB HERE ***

Yao Qi (7):
  Emit a warning when writing to a readonly section and trust_readonly is true
  set trust-readonly-sections off in test cases
  New function windows_init_abi
  Trust readonly sections if target has memory protection
  DOC and NEWS
  Linux has memory protection.
  Windows has memory protection

 gdb/NEWS                                |    5 ++
 gdb/amd64-windows-tdep.c                |    5 +-
 gdb/arch-utils.c                        |    7 +++
 gdb/arch-utils.h                        |    2 +
 gdb/doc/gdb.texinfo                     |   11 ++++-
 gdb/gdbarch.c                           |   24 ++++++++++++
 gdb/gdbarch.h                           |    6 +++
 gdb/gdbarch.sh                          |    3 +
 gdb/i386-cygwin-tdep.c                  |    9 +---
 gdb/linux-tdep.c                        |   10 +++++
 gdb/target.c                            |   62 ++++++++++++++++++++++--------
 gdb/testsuite/gdb.base/break-always.exp |    4 ++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp |    5 ++
 gdb/windows-tdep.c                      |   31 +++++++++++++++-
 gdb/windows-tdep.h                      |    6 +--
 15 files changed, 156 insertions(+), 34 deletions(-)

-- 
1.7.7.6

[PATCH 6/7] Linux has memory protection.

[Yao Qi]

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* linux-tdep.c (linux_has_memory_protection): New function.
	(linux_init_abi): Register linux_has_memory_protection
	to gdbarch 'has_memory_protection'.
---
 gdb/linux-tdep.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index eb8ea2b..36e7119 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -259,6 +259,15 @@ linux_has_shared_address_space (struct gdbarch *gdbarch)
   return linux_is_uclinux ();
 }
 
+/* This is the implementation of gdbarch method has_memory_protection.  */
+
+static int
+linux_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Normal linux has memory protection, while uClinux doesn't.  */
+  return !linux_is_uclinux ();
+}
+
 /* This is how we want PTIDs from core files to be printed.  */
 
 static char *
@@ -1783,6 +1792,7 @@ linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 				      linux_gdb_signal_from_target);
   set_gdbarch_gdb_signal_to_target (gdbarch,
 				    linux_gdb_signal_to_target);
+  set_gdbarch_has_memory_protection (gdbarch, linux_has_memory_protection);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
-- 
1.7.7.6

[PATCH 3/7] New function windows_init_abi

[Yao Qi]

Hi,
When I look for a function, which does all abi-related initialization in
windows-tdep.c, it doesn't exist.  It would be good to create such
function, similar to linux_init_abi, and put all windows abi related
initialization in it.

This patch adds a new function windows_init_abi and move these two
functions calls in it,

  set_gdbarch_has_dos_based_file_system (gdbarch, 1);

  set_gdbarch_iterate_over_objfiles_in_search_order
    (gdbarch, windows_iterate_over_objfiles_in_search_order);

amd64-windows doesn't set gdbarch flag has_dos_based_file_system, so
with this patch applied, this flag is set for amd64-windows.
I build GDB with "--host=i686-pc-linux-gnu --target=x86_64-mingw32",
and start GDB with a x86_64 PE executable,

Without this patch, we get,
(gdb) show target-file-system-kind
The assumed file system kind for target reported file names is "auto" (currently "unix").

With this path, we get,
(gdb) show target-file-system-kind
The assumed file system kind for target reported file names is "auto" (currently "dos-based").

As the result of this patch, windows_iterate_over_objfiles_in_search_order
can be made static too.

I don't have a x86_64-mingw32 env to test this patch.

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* amd64-windows-tdep.c (amd64_windows_init_abi): Don't call
	set_gdbarch_iterate_over_objfiles_in_search_order.  Call
	windows_init_abi instead.
	* i386-cygwin-tdep.c (i386_cygwin_init_abi): Don't call
	set_gdbarch_has_dos_based_file_system and
	set_gdbarch_iterate_over_objfiles_in_search_order.  Call
	windows_init_abi instead.
	* windows-tdep.c (windows_init_abi): New function.
	(windows_iterate_over_objfiles_in_search_order): Make it
	static.
	* windows-tdep.h (windows_init_abi): Declare.
	(windows_iterate_over_objfiles_in_search_order): Remove
	declaration.
---
 gdb/amd64-windows-tdep.c |    5 ++---
 gdb/i386-cygwin-tdep.c   |    9 ++-------
 gdb/windows-tdep.c       |   16 +++++++++++++++-
 gdb/windows-tdep.h       |    6 ++----
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 4e750a1..0dfc906 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -972,6 +972,8 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   amd64_init_abi (info, gdbarch);
 
+  windows_init_abi (info, gdbarch);
+
   /* On Windows, "long"s are only 32bit.  */
   set_gdbarch_long_bit (gdbarch, 32);
 
@@ -987,9 +989,6 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_skip_trampoline_code (gdbarch,
 				    amd64_windows_skip_trampoline_code);
 
-  set_gdbarch_iterate_over_objfiles_in_search_order
-    (gdbarch, windows_iterate_over_objfiles_in_search_order);
-
   set_gdbarch_skip_prologue (gdbarch, amd64_windows_skip_prologue);
 
   set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charset);
diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-cygwin-tdep.c
index a3e4e7c..01fc15b 100644
--- a/gdb/i386-cygwin-tdep.c
+++ b/gdb/i386-cygwin-tdep.c
@@ -233,6 +233,8 @@ i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
+  windows_init_abi (info, gdbarch);
+
   set_gdbarch_skip_trampoline_code (gdbarch, i386_cygwin_skip_trampoline_code);
 
   set_gdbarch_skip_main_prologue (gdbarch, i386_skip_main_prologue);
@@ -253,13 +255,6 @@ i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_core_pid_to_str (gdbarch, i386_windows_core_pid_to_str);
 
   set_gdbarch_auto_wide_charset (gdbarch, i386_cygwin_auto_wide_charset);
-
-  /* Canonical paths on this target look like
-     `c:\Program Files\Foo App\mydll.dll', for example.  */
-  set_gdbarch_has_dos_based_file_system (gdbarch, 1);
-
-  set_gdbarch_iterate_over_objfiles_in_search_order
-    (gdbarch, windows_iterate_over_objfiles_in_search_order);
 }
 
 static enum gdb_osabi
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index f90323f..50c5795 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -427,7 +427,7 @@ windows_xfer_shared_library (const char* so_name, CORE_ADDR load_addr,
    to print the value of another global variable defined with the same
    name, but in a different DLL.  */
 
-void
+static void
 windows_iterate_over_objfiles_in_search_order
   (struct gdbarch *gdbarch,
    iterate_over_objfiles_in_search_order_cb_ftype *cb,
@@ -481,6 +481,20 @@ init_w32_command_list (void)
     }
 }
 
+/* To be called from the various GDB_OSABI_CYGWIN handlers for the
+   various Windows architectures and machine types.  */
+
+void
+windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  /* Canonical paths on this target look like
+     `c:\Program Files\Foo App\mydll.dll', for example.  */
+  set_gdbarch_has_dos_based_file_system (gdbarch, 1);
+
+  set_gdbarch_iterate_over_objfiles_in_search_order
+    (gdbarch, windows_iterate_over_objfiles_in_search_order);
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_windows_tdep;
 
diff --git a/gdb/windows-tdep.h b/gdb/windows-tdep.h
index 99136fa..7b04bb2 100644
--- a/gdb/windows-tdep.h
+++ b/gdb/windows-tdep.h
@@ -30,8 +30,6 @@ extern void windows_xfer_shared_library (const char* so_name,
 					 struct gdbarch *gdbarch,
 					 struct obstack *obstack);
 
-extern void windows_iterate_over_objfiles_in_search_order
-  (struct gdbarch *gdbarch,
-   iterate_over_objfiles_in_search_order_cb_ftype *cb,
-   void *cb_data, struct objfile *current_objfile);
+extern void windows_init_abi (struct gdbarch_info info,
+			      struct gdbarch *gdbarch);
 #endif
-- 
1.7.7.6

[PATCH 5/7] DOC and NEWS

[Yao Qi]

The doc and NEWS patch was reviewed by Eli here
https://sourceware.org/ml/gdb-patches/2013-09/msg00214.html

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* NEWS: Describe the default option of
	"trust-readonly-sections" becomes "auto" and the related
	changes.

gdb/doc:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (File): Mention that user has
	'set trust-readonly-sections off' to get updated contents if
	they are modified.  Explain the default option of
	"trust-readonly-sections" is "auto".
---
 gdb/NEWS            |    5 +++++
 gdb/doc/gdb.texinfo |   11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index ad97f6f..e60a553 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,11 @@
 
 *** Changes since GDB 7.6
 
+* The default value of option "trust-readonly-sections" is "auto".  GDB
+  will decide based on the target memory protection features whether to
+  read readonly sections from object file instead of from the inferior's
+  memory.
+
 * The "maintenance print objfiles" command now takes an optional regexp.
 
 * The "catch syscall" command now works on arm*-linux* targets.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 21250fe..cd7859a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -16673,12 +16673,19 @@ out of the object file, rather than from the target program.
 For some targets (notably embedded ones), this can be a significant
 enhancement to debugging performance.
 
-The default is off.
+@item set trust-readonly-sections auto
+This is the default mode.  @value{GDBN} will decide based on the
+target memory protection features whether to read readonly sections
+from object file instead of from the inferior's memory, because the
+contents of the section in the inferior can't change.
 
 @item set trust-readonly-sections off
 Tell @value{GDBN} not to trust readonly sections.  This means that
 the contents of the section might change while the program is running,
-and must therefore be fetched from the target when needed.
+and must therefore be fetched from the target when needed.  If you
+modified the code in the target program, you have to
+@code{set trust-readonly-sections off} to guarantee @value{GDBN} reads
+updated contents from the target program instead of object file.
 
 @item show trust-readonly-sections
 Show the current setting of trusting readonly sections.
-- 
1.7.7.6

[PATCH 4/7] Trust readonly sections if target has memory protection

[Yao Qi]

This patch first changes command "trust-readonly-sections" to an
auto_boolean command, so "auto" means that GDB trusts read-only
sections if the target has memory protection.  Then, this patch adds a
gdbarch hook method "has_memory_protection".  Next patches are to
implement the hook method for linux target and windows target.

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* arch-utils.c (default_has_memory_protection): New function.
	* arch-utils.h (default_has_memory_protection): Declaration.
	* gdbarch.sh (has_memory_protection): New hook method.
	* gdbarch.c: Re-generated.
	* gdbarch.h: Re-generated.
	* target.c (trust_readonly): Change type to 'enum auto_boolean'
	and initialize it to 'AUTO_BOOLEAN_AUTO'.
	(trust_readonly_p): New function.
	(memory_xfer_partial_1): Call trust_readonly_p.
	(initialize_targets): Register command
	"trust-readonly-sections" as add_setshow_auto_boolean_cmd.
---
 gdb/arch-utils.c |    7 +++++++
 gdb/arch-utils.h |    2 ++
 gdb/gdbarch.c    |   24 ++++++++++++++++++++++++
 gdb/gdbarch.h    |    6 ++++++
 gdb/gdbarch.sh   |    3 +++
 gdb/target.c     |   33 ++++++++++++++++++++++++---------
 6 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 459fd88..8418bbc 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -769,6 +769,13 @@ default_has_shared_address_space (struct gdbarch *gdbarch)
 }
 
 int
+default_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Simply say no.  */
+  return 0;
+}
+
+int
 default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
 				  CORE_ADDR addr, int *isize, char **msg)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 3f0e64f..61973c0 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -153,6 +153,8 @@ extern struct gdbarch *get_current_arch (void);
 
 extern int default_has_shared_address_space (struct gdbarch *);
 
+extern int default_has_memory_protection (struct gdbarch *gdbarch);
+
 extern int default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
 					     CORE_ADDR addr,
 					     int *isize, char **msg);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 1f3380e..13e5e82 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -278,6 +278,7 @@ struct gdbarch
   int has_global_solist;
   int has_global_breakpoints;
   gdbarch_has_shared_address_space_ftype *has_shared_address_space;
+  gdbarch_has_memory_protection_ftype *has_memory_protection;
   gdbarch_fast_tracepoint_valid_at_ftype *fast_tracepoint_valid_at;
   gdbarch_auto_charset_ftype *auto_charset;
   gdbarch_auto_wide_charset_ftype *auto_wide_charset;
@@ -451,6 +452,7 @@ struct gdbarch startup_gdbarch =
   0,  /* has_global_solist */
   0,  /* has_global_breakpoints */
   default_has_shared_address_space,  /* has_shared_address_space */
+  default_has_memory_protection,  /* has_memory_protection */
   default_fast_tracepoint_valid_at,  /* fast_tracepoint_valid_at */
   default_auto_charset,  /* auto_charset */
   default_auto_wide_charset,  /* auto_wide_charset */
@@ -546,6 +548,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->displaced_step_location = NULL;
   gdbarch->relocate_instruction = NULL;
   gdbarch->has_shared_address_space = default_has_shared_address_space;
+  gdbarch->has_memory_protection = default_has_memory_protection;
   gdbarch->fast_tracepoint_valid_at = default_fast_tracepoint_valid_at;
   gdbarch->auto_charset = default_auto_charset;
   gdbarch->auto_wide_charset = default_auto_wide_charset;
@@ -757,6 +760,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of has_global_solist, invalid_p == 0 */
   /* Skip verify of has_global_breakpoints, invalid_p == 0 */
   /* Skip verify of has_shared_address_space, invalid_p == 0 */
+  /* Skip verify of has_memory_protection, invalid_p == 0 */
   /* Skip verify of fast_tracepoint_valid_at, invalid_p == 0 */
   /* Skip verify of auto_charset, invalid_p == 0 */
   /* Skip verify of auto_wide_charset, invalid_p == 0 */
@@ -1078,6 +1082,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: has_global_solist = %s\n",
                       plongest (gdbarch->has_global_solist));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: has_memory_protection = <%s>\n",
+                      host_address_to_string (gdbarch->has_memory_protection));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: has_shared_address_space = <%s>\n",
                       host_address_to_string (gdbarch->has_shared_address_space));
   fprintf_unfiltered (file,
@@ -4240,6 +4247,23 @@ set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch,
 }
 
 int
+gdbarch_has_memory_protection (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->has_memory_protection != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_has_memory_protection called\n");
+  return gdbarch->has_memory_protection (gdbarch);
+}
+
+void
+set_gdbarch_has_memory_protection (struct gdbarch *gdbarch,
+                                   gdbarch_has_memory_protection_ftype has_memory_protection)
+{
+  gdbarch->has_memory_protection = has_memory_protection;
+}
+
+int
 gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 013f071..ade07d7 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1176,6 +1176,12 @@ typedef int (gdbarch_has_shared_address_space_ftype) (struct gdbarch *gdbarch);
 extern int gdbarch_has_shared_address_space (struct gdbarch *gdbarch);
 extern void set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch, gdbarch_has_shared_address_space_ftype *has_shared_address_space);
 
+/* True if the target has memory protection. */
+
+typedef int (gdbarch_has_memory_protection_ftype) (struct gdbarch *gdbarch);
+extern int gdbarch_has_memory_protection (struct gdbarch *gdbarch);
+extern void set_gdbarch_has_memory_protection (struct gdbarch *gdbarch, gdbarch_has_memory_protection_ftype *has_memory_protection);
+
 /* True if a fast tracepoint can be set at an address. */
 
 typedef int (gdbarch_fast_tracepoint_valid_at_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 04ca38c..080a5ec 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -932,6 +932,9 @@ v:int:has_global_breakpoints:::0:0::0
 # True if inferiors share an address space (e.g., uClinux).
 m:int:has_shared_address_space:void:::default_has_shared_address_space::0
 
+# True if the target has memory protection.
+m:int:has_memory_protection:void:::default_has_memory_protection::0
+
 # True if a fast tracepoint can be set at an address.
 m:int:fast_tracepoint_valid_at:CORE_ADDR addr, int *isize, char **msg:addr, isize, msg::default_fast_tracepoint_valid_at::0
 
diff --git a/gdb/target.c b/gdb/target.c
index f6a58ff..df80150 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -168,10 +168,10 @@ struct target_ops current_target;
 
 static struct cmd_list_element *targetlist = NULL;
 
-/* Nonzero if we should trust readonly sections from the
+/* If AUTO_BOOLEAN_TRUE, we should trust readonly sections from the
    executable when reading memory.  */
 
-static int trust_readonly = 0;
+static enum auto_boolean trust_readonly = AUTO_BOOLEAN_AUTO;
 
 /* Nonzero if we should show true memory content including
    memory breakpoint inserted by gdb.  */
@@ -1437,6 +1437,18 @@ memory_xfer_live_readonly_partial (struct target_ops *ops,
   return 0;
 }
 
+/* Return true if GDB trusts readonly sections.  Return false
+   otherwise.  */
+
+static int
+trust_readonly_p (void)
+{
+  if (trust_readonly != AUTO_BOOLEAN_AUTO)
+    return trust_readonly == AUTO_BOOLEAN_TRUE;
+
+  return gdbarch_has_memory_protection (target_gdbarch ());
+}
+
 /* Perform a partial memory transfer.
    For docs see target.h, to_xfer_partial.  */
 
@@ -1475,7 +1487,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
      files.  When GDB writes to a readonly section of the inferior,
      emit a warning that option 'trust-readonly-sections' should be
      turned off.  */
-  if (trust_readonly)
+  if (trust_readonly_p ())
     {
       struct target_section *secp;
 
@@ -5099,16 +5111,19 @@ command."),
 			     show_targetdebug,
 			     &setdebuglist, &showdebuglist);
 
-  add_setshow_boolean_cmd ("trust-readonly-sections", class_support,
-			   &trust_readonly, _("\
+  add_setshow_auto_boolean_cmd ("trust-readonly-sections", class_support,
+				&trust_readonly, _("\
 Set mode for reading from readonly sections."), _("\
 Show mode for reading from readonly sections."), _("\
 When this mode is on, memory reads from readonly sections (such as .text)\n\
 will be read from the object file instead of from the target.  This will\n\
-result in significant performance improvement for remote targets."),
-			   NULL,
-			   show_trust_readonly,
-			   &setlist, &showlist);
+result in significant performance improvement for remote targets.\n\
+When this mode is auto, GDB will decide based on the target memory\n\
+protection features whether to read readonly sections from object file\n\
+instead of from the inferior's memory.\n"),
+				NULL,
+				show_trust_readonly,
+				&setlist, &showlist);
 
   add_com ("monitor", class_obscure, do_monitor_command,
 	   _("Send a command to the remote monitor (remote targets only)."));
-- 
1.7.7.6

[PATCH 7/7] Windows has memory protection

[Yao Qi]

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* windows-tdep.c (windows_has_memory_protection): New function.
	(windows_init_abi): Install windows_has_memory_protection to
	gdbarch method 'has_memory_protection'.
---
 gdb/windows-tdep.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 50c5795..93d6fe6 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -481,6 +481,18 @@ init_w32_command_list (void)
     }
 }
 
+/* This is the implementation of gdbarch method has_memory_protection.  */
+
+static int
+windows_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Modern Windows has memory protection.  However, some ancient
+     versions of Windows don't have full memory protection, such as
+     Windows 9x.  If necessary, GDB should detect the version of
+     Windows and return false if the target OS is Windows 9x.  */
+  return 1;
+}
+
 /* To be called from the various GDB_OSABI_CYGWIN handlers for the
    various Windows architectures and machine types.  */
 
@@ -493,6 +505,9 @@ windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   set_gdbarch_iterate_over_objfiles_in_search_order
     (gdbarch, windows_iterate_over_objfiles_in_search_order);
+
+  set_gdbarch_has_memory_protection (gdbarch,
+				     windows_has_memory_protection);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
-- 
1.7.7.6

[PATCH 2/7] set trust-readonly-sections off in test cases

[Yao Qi]

In the test cases, if GDB modified code in the inferior, it should
'set trust-readonly-sections off'.

gdb/testsuite:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* gdb.base/break-always.exp: Set trust-readonly-sections off.
	* gdb.mi/mi-fill-memory.exp: Likewise.
---
 gdb/testsuite/gdb.base/break-always.exp |    4 ++++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.base/break-always.exp b/gdb/testsuite/gdb.base/break-always.exp
index b0a9faf..16f6d47 100644
--- a/gdb/testsuite/gdb.base/break-always.exp
+++ b/gdb/testsuite/gdb.base/break-always.exp
@@ -69,6 +69,10 @@ gdb_test "p /x \$shadow = *(char *) $bp_address" \
 # and still leave the breakpoint insn planted.  Try twice with
 # different values, in case we happen to be writting exactly what was
 # there already.
+# Turn option "trust-readonly-sections" off to force GDB to read
+# contents from the target instead of the object file.
+gdb_test_no_output "set trust-readonly-sections off"
+
 gdb_test "p /x *(char *) $bp_address = 0" \
     " = 0x0" \
     "write 0 to breakpoint's address"
diff --git a/gdb/testsuite/gdb.mi/mi-fill-memory.exp b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
index 5f68cbb..2b7408a 100644
--- a/gdb/testsuite/gdb.mi/mi-fill-memory.exp
+++ b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
@@ -35,6 +35,11 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}.c" "${binfile}" executable {d
 mi_run_to_main
 mi_next_to "main" "" "mi-read-memory.c" "20" "next at main"
 
+# Turn option "trust-readonly-sections" off to force GDB to read
+# contents from the target instead of the object file.
+mi_gdb_test "-gdb-set trust-readonly-sections off" "\\\^done" \
+    "set trust-readonly-sections off"
+
 mi_gdb_test "1-data-write-memory-bytes"\
 	"1\\\^error,msg=\"Usage: ADDR DATA \\\[COUNT\\\]\.\""\
 	"no arguments"
-- 
1.7.7.6

Re: [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Sun, 8 Sep 2013 20:03:21 +0800
> 
> If users write a readonly section, such as .text, the contents of the inferior
> and of the executable become out of sync.  It is better to emit a warning
> to ask users to "set trust-readonly-sections off".
> 
> (gdb) set trust-readonly-sections on
> (gdb) p /x* (char *) 0x080484c1 = 0xcc
> warning: Writing to a readonly section so that the contents in the
> inferior and in the executable are out of sync.  Please 'set
> trust-readonly-sections off'.
> $1 = 0xcc

This warning got me puzzled: did GDB write to the address or did it
not?  If it did, why do we need the warning?  If it did not, the
warning does not say that clearly enough.

How about the following instead:

  (gdb) set trust-readonly-sections on
  (gdb) p /x* (char *) 0x080484c1 = 0xcc
  Address is read-only and trust-readonly-sections is set to "on";
  set it to "off" and write to a read-only section? (y or n)

And then if the user answers "yes", set trust-readonly-sections to off
automatically.  Isn't this a better UI?

Re: [PATCH 4/7] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Sun, 8 Sep 2013 20:03:24 +0800
> 
>  int
> +default_has_memory_protection (struct gdbarch *gdbarch)
> +{
> +  /* Simply say no.  */
> +  return 0;
> +}

Do most of our supported targets have or don't have memory protection?
How about most native targets?

If a large subset of targets have the protection, it might make more
sense to say YES by default, not NO.

Re: [PATCH 4/7] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/08/2013 11:13 PM, Eli Zaretskii wrote:
> Do most of our supported targets have or don't have memory protection?
> How about most native targets?

Nearly all targets with OS have memory protection, but bare-metal
targets don't.

>
> If a large subset of targets have the protection, it might make more
> sense to say YES by default, not NO.

If the default is YES, in ${arch}-tdep.c, we should set it to NO, and 
set it back to YES in ${arch}-${os}-tdep.c.  More changes are involved.

Since the number of supported architectures is greater than the number 
of supported operating systems, we'll do less changes if the default is 
NO.  Only have to set it to YES for supported operating systems.

-- 
Yao (齐尧)

Re: [PATCH 4/7] Trust readonly sections if target has memory protection

[Eli Zaretskii]

> Date: Mon, 9 Sep 2013 15:48:58 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 09/08/2013 11:13 PM, Eli Zaretskii wrote:
> > Do most of our supported targets have or don't have memory protection?
> > How about most native targets?
> 
> Nearly all targets with OS have memory protection, but bare-metal
> targets don't.

Is there a reasonably easy way of setting the default to YES on all
native targets?

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Mark Kettenis]

> From: Yao Qi <yao@codesourcery.com>
> Date: Sun, 8 Sep 2013 20:03:20 +0800
> 
> Here is the V2, to address comments to V1,
> 
>   - Warn about writes to readonly sections if trust-readonly-sections
> is enabled. (patch 1/7)
>   - Windows targets have memory protection. (patch 7/7)
> 
> Do some refactor in Windows related targets, add a new function
> windows_init_abi (done by patch 3/7).  It paves the way for patch
> 7/7.
> 
> Below the description of this series in V1:
> https://sourceware.org/ml/gdb-patches/2013-09/msg00193.html
> 
> When option "trust-readonly-sections" was introduced in this patch
> http://www.sourceware.org/ml/gdb-patches/2002-01/msg00711.html, there
> was the discussion to turn it on in default.  However, we didn't do
> that because we have a concern that bad or buggy program may modify the
> code on the non-memory protected system.  On the other hand, it is
> highly recommended to turn this on in remote debugging.
> 
> I am wondering if we can teach GDB to trust read-only sections if it
> knows the target system has memory protection.  For some targets, such
> as linux, we know they do have memory protection.  This is what this
> patch series tries to do.

What does "memory protection" mean?  That a target has an MMU that
allows pages to be marked read-only?  That really is more a hardware
feature than a OS aatribute.

Even on systems that have an MMU that can mark pages read-only, system
calls like mprotect(2) can be used to make read-only pages
(temporarily) writable.  This is done by the OpenBSD dynamic linker
during relocation processing.  I expect other systems implementing
strict W^X to do the same.  Enabling trust-readonly-sections on such
systems would be a bad idea.

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/10/2013 03:16 AM, Mark Kettenis wrote:
> What does "memory protection" mean?  That a target has an MMU that
> allows pages to be marked read-only?  That really is more a hardware
> feature than a OS aatribute.

"memory protection" means prevent modifying readonly sections or regions 
of the process.  "memory protection" is a joint effort by MMU
and OS together, IMO.

>
> Even on systems that have an MMU that can mark pages read-only, system
> calls like mprotect(2) can be used to make read-only pages
> (temporarily) writable.  This is done by the OpenBSD dynamic linker
> during relocation processing.  I expect other systems implementing
> strict W^X to do the same.  Enabling trust-readonly-sections on such
> systems would be a bad idea.

If GDB can monitor mprotect syscall, it can still trust readonly
sections if their pages are not changed to writable by mprotect.

GDB is able to 'catch syscall mprotect', only on linux-nat 
unfortunately.  It doesn't work on remote target

   "catch syscall" support in the remote protocol
   https://sourceware.org/bugzilla/show_bug.cgi?id=13585

Similarly, GDB can monitor function VirtualProtect on Windows target
too.
-- 
Yao (齐尧)

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/10/2013 12:05 PM, Yao Qi wrote:
>> Even on systems that have an MMU that can mark pages read-only, system
>> >calls like mprotect(2) can be used to make read-only pages
>> >(temporarily) writable.  This is done by the OpenBSD dynamic linker
>> >during relocation processing.  I expect other systems implementing
>> >strict W^X to do the same.  Enabling trust-readonly-sections on such
>> >systems would be a bad idea.
> If GDB can monitor mprotect syscall, it can still trust readonly
> sections if their pages are not changed to writable by mprotect.
>
> GDB is able to 'catch syscall mprotect', only on linux-nat
> unfortunately.  It doesn't work on remote target
>
>     "catch syscall" support in the remote protocol
>     https://sourceware.org/bugzilla/show_bug.cgi?id=13585
>
> Similarly, GDB can monitor function VirtualProtect on Windows target
> too.

Do we have a concrete criteria to reject or accept this patch series?
I need this to plan for my next step.  Here are some possibilities in
my brain,

  1.  This series is rejected because GDB is incorrect when program uses 
mprotect and change some readonly pages, unless....
  2.  ... GDB is able to monitor syscall mprotect, and trust readonly
sections if they are still readonly in the process's space.
  3.  This series can be accepted.  I am wondering how popular that user
program modifies readonly sections in process space by mprotect.  Do we
really sacrifice the performance of GDB in some common cases, like
remote debugging, for this corner case?  I'd like to add doc for the
case using mprotect and remind user to turn trust-readonly-sections off
by him/her self.

-- 
Yao (齐尧)

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Mark Kettenis]

> Date: Thu, 12 Sep 2013 16:29:26 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 09/10/2013 12:05 PM, Yao Qi wrote:
> >> Even on systems that have an MMU that can mark pages read-only, system
> >> >calls like mprotect(2) can be used to make read-only pages
> >> >(temporarily) writable.  This is done by the OpenBSD dynamic linker
> >> >during relocation processing.  I expect other systems implementing
> >> >strict W^X to do the same.  Enabling trust-readonly-sections on such
> >> >systems would be a bad idea.
> > If GDB can monitor mprotect syscall, it can still trust readonly
> > sections if their pages are not changed to writable by mprotect.
> >
> > GDB is able to 'catch syscall mprotect', only on linux-nat
> > unfortunately.  It doesn't work on remote target
> >
> >     "catch syscall" support in the remote protocol
> >     https://sourceware.org/bugzilla/show_bug.cgi?id=13585
> >
> > Similarly, GDB can monitor function VirtualProtect on Windows target
> > too.
> 
> Do we have a concrete criteria to reject or accept this patch series?
> I need this to plan for my next step.  Here are some possibilities in
> my brain,
> 
>   1.  This series is rejected because GDB is incorrect when program uses 
> mprotect and change some readonly pages, unless....

I'm certainly not outright rejecting it.  But you'll certainly need to
rethink in which contexts it is safe/acceptable that "auto" actually
turns on the trust-readonly-sections feature.  That decision should
probably be done on a per-architecture, per-OS basis, and only for
remote debugging.

>   2.  ... GDB is able to monitor syscall mprotect, and trust readonly
> sections if they are still readonly in the process's space.

Of course monitoring syscalls comes with a performance penalty as
well.  Worse, it will affect the timing of the program you're
debugging, so turning it on by default would probably be a seriously
bad idea unless you can intercept "just" the mprotect syscalls.

>   3.  This series can be accepted.  I am wondering how popular that user
> program modifies readonly sections in process space by mprotect.  Do we
> really sacrifice the performance of GDB in some common cases, like
> remote debugging, for this corner case?  I'd like to add doc for the
> case using mprotect and remind user to turn trust-readonly-sections off
> by him/her self.

It will happen for *any* dynamically linked binary on OpenBSD.  I
expect the same to be true for many security-enhanced Linux variants.
But running strace on a randomly chosen binary from Ubuntu 10.4
suggests its not common there.  There are a couple of mprotect calls,
but none that add PROT_WRITE permission.

Personally, I think trust-readonly-sections remains a dangerous
feature that should only be enabled by people who know what they're
doing.  Having GDB print values for variables that are different from
what the program itself is actually seeing would be very frustrating
and potentially waste a lot of my time.  But I only really care about
native debugging.

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/12/2013 05:49 PM, Mark Kettenis wrote:
> I'm certainly not outright rejecting it.  But you'll certainly need to
> rethink in which contexts it is safe/acceptable that "auto" actually
> turns on the trust-readonly-sections feature.  That decision should
> probably be done on a per-architecture, per-OS basis, and only for
> remote debugging.

Yeah, that is reasonable to me.  Then, the proposed scope could be
{x86, x86_64}-{linux,mingw,cygwin} for remote debugging only.  What do
you think?

I am fine to extend this scope to all linux targets, as what I did in
my patches, but looks it is safe to keep this set small at the
beginning.

-- 
Yao (齐尧)

[PATCH 0/7 V3] Trust readonly sections if target has memory protection

[Yao Qi]

Hello,
Here is the V3, to address two comments to V2.

 - Give a yes-or-no query when read-only address is modified and
   trust-readonly-sections is on.  See patch 1/7.  A test case
   gdb.base/trust-readonly.exp is added too.
 - Enable trust-readonly-sections on all linux and windows targets
   for remote debugging only.  It is different from what I proposed,
   {x86, x86_64}-{linux,mingw,cygwin}, because it looks odd to enable
   this feature only on x86*-linux targets and disable it on the rest of
   linux targets.  See patch 4/7.

Patch 2/7, 3/7, 6/7 and 7/7 are unchanged.  Since the condition on
trusting readonly-sections is restricted (only on remote debugging),
so the doc (patch 5/7) is updated as well.

V2 can be found https://sourceware.org/ml/gdb-patches/2013-09/msg00258.html

*** BLURB HERE ***

Yao Qi (7):
  Emit a warning when writing to a readonly section and trust_readonly
    is true
  set trust-readonly-sections off in test cases
  New function windows_init_abi
  Trust readonly sections if target has memory protection and in remote
    debugging
  DOC and NEWS
  Linux has memory protection.
  Windows has memory protection

 gdb/NEWS                                  |    5 ++
 gdb/amd64-windows-tdep.c                  |    5 +-
 gdb/arch-utils.c                          |    7 +++
 gdb/arch-utils.h                          |    2 +
 gdb/doc/gdb.texinfo                       |   12 ++++-
 gdb/gdbarch.c                             |   24 ++++++++
 gdb/gdbarch.h                             |    6 ++
 gdb/gdbarch.sh                            |    3 +
 gdb/i386-cygwin-tdep.c                    |    9 +---
 gdb/linux-tdep.c                          |   10 ++++
 gdb/target.c                              |   70 ++++++++++++++++++------
 gdb/testsuite/gdb.base/break-always.exp   |    4 ++
 gdb/testsuite/gdb.base/trust-readonly.exp |   85 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp   |    5 ++
 gdb/windows-tdep.c                        |   31 ++++++++++-
 gdb/windows-tdep.h                        |    6 +--
 16 files changed, 250 insertions(+), 34 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/trust-readonly.exp

-- 
1.7.7.6

[PATCH 4/7] Trust readonly sections if target has memory protection and in remote debugging

[Yao Qi]

This patch first changes command "trust-readonly-sections" to an
auto_boolean command, so "auto" means that GDB trusts read-only
sections if the target has memory protection and GDB is donging remote
debugging.  Then, this patch adds a gdbarch hook method
"has_memory_protection".  Next patches are to implement the hook
method for linux target and windows target.

Compared with V2, the only change in V3 is here,

+/* Return true if GDB trusts readonly sections.  Return false
+   otherwise.  */
+
+static int
+trust_readonly_p (void)
+{
+  if (trust_readonly != AUTO_BOOLEAN_AUTO)
+    return trust_readonly == AUTO_BOOLEAN_TRUE;
+
+  /* We only trust readonly sections when GDB is dong remote debugging
+     on the targets which have memory protection.  */
+  return (gdbarch_has_memory_protection (target_gdbarch ())
+         && (strcmp (target_shortname, "remote") == 0
+             || strcmp (target_shortname, "extended-remote") == 0));
+}

we add some code to checker whether the target is remote or
extended-remote.

gdb:

2013-09-20  Yao Qi  <yao@codesourcery.com>

	* arch-utils.c (default_has_memory_protection): New function.
	* arch-utils.h (default_has_memory_protection): Declaration.
	* gdbarch.sh (has_memory_protection): New hook method.
	* gdbarch.c: Re-generated.
	* gdbarch.h: Re-generated.
	* target.c (trust_readonly): Change type to 'enum auto_boolean'
	and initialize it to 'AUTO_BOOLEAN_AUTO'.
	(trust_readonly_p): New function.
	(memory_xfer_partial_1): Call trust_readonly_p.
	(initialize_targets): Register command
	"trust-readonly-sections" as add_setshow_auto_boolean_cmd.
---
 gdb/arch-utils.c |    7 +++++++
 gdb/arch-utils.h |    2 ++
 gdb/gdbarch.c    |   24 ++++++++++++++++++++++++
 gdb/gdbarch.h    |    6 ++++++
 gdb/gdbarch.sh   |    3 +++
 gdb/target.c     |   39 +++++++++++++++++++++++++++++----------
 6 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 459fd88..8418bbc 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -769,6 +769,13 @@ default_has_shared_address_space (struct gdbarch *gdbarch)
 }
 
 int
+default_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Simply say no.  */
+  return 0;
+}
+
+int
 default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
 				  CORE_ADDR addr, int *isize, char **msg)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 3f0e64f..61973c0 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -153,6 +153,8 @@ extern struct gdbarch *get_current_arch (void);
 
 extern int default_has_shared_address_space (struct gdbarch *);
 
+extern int default_has_memory_protection (struct gdbarch *gdbarch);
+
 extern int default_fast_tracepoint_valid_at (struct gdbarch *gdbarch,
 					     CORE_ADDR addr,
 					     int *isize, char **msg);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 1f3380e..13e5e82 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -278,6 +278,7 @@ struct gdbarch
   int has_global_solist;
   int has_global_breakpoints;
   gdbarch_has_shared_address_space_ftype *has_shared_address_space;
+  gdbarch_has_memory_protection_ftype *has_memory_protection;
   gdbarch_fast_tracepoint_valid_at_ftype *fast_tracepoint_valid_at;
   gdbarch_auto_charset_ftype *auto_charset;
   gdbarch_auto_wide_charset_ftype *auto_wide_charset;
@@ -451,6 +452,7 @@ struct gdbarch startup_gdbarch =
   0,  /* has_global_solist */
   0,  /* has_global_breakpoints */
   default_has_shared_address_space,  /* has_shared_address_space */
+  default_has_memory_protection,  /* has_memory_protection */
   default_fast_tracepoint_valid_at,  /* fast_tracepoint_valid_at */
   default_auto_charset,  /* auto_charset */
   default_auto_wide_charset,  /* auto_wide_charset */
@@ -546,6 +548,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->displaced_step_location = NULL;
   gdbarch->relocate_instruction = NULL;
   gdbarch->has_shared_address_space = default_has_shared_address_space;
+  gdbarch->has_memory_protection = default_has_memory_protection;
   gdbarch->fast_tracepoint_valid_at = default_fast_tracepoint_valid_at;
   gdbarch->auto_charset = default_auto_charset;
   gdbarch->auto_wide_charset = default_auto_wide_charset;
@@ -757,6 +760,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of has_global_solist, invalid_p == 0 */
   /* Skip verify of has_global_breakpoints, invalid_p == 0 */
   /* Skip verify of has_shared_address_space, invalid_p == 0 */
+  /* Skip verify of has_memory_protection, invalid_p == 0 */
   /* Skip verify of fast_tracepoint_valid_at, invalid_p == 0 */
   /* Skip verify of auto_charset, invalid_p == 0 */
   /* Skip verify of auto_wide_charset, invalid_p == 0 */
@@ -1078,6 +1082,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: has_global_solist = %s\n",
                       plongest (gdbarch->has_global_solist));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: has_memory_protection = <%s>\n",
+                      host_address_to_string (gdbarch->has_memory_protection));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: has_shared_address_space = <%s>\n",
                       host_address_to_string (gdbarch->has_shared_address_space));
   fprintf_unfiltered (file,
@@ -4240,6 +4247,23 @@ set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch,
 }
 
 int
+gdbarch_has_memory_protection (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->has_memory_protection != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_has_memory_protection called\n");
+  return gdbarch->has_memory_protection (gdbarch);
+}
+
+void
+set_gdbarch_has_memory_protection (struct gdbarch *gdbarch,
+                                   gdbarch_has_memory_protection_ftype has_memory_protection)
+{
+  gdbarch->has_memory_protection = has_memory_protection;
+}
+
+int
 gdbarch_fast_tracepoint_valid_at (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 013f071..ade07d7 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1176,6 +1176,12 @@ typedef int (gdbarch_has_shared_address_space_ftype) (struct gdbarch *gdbarch);
 extern int gdbarch_has_shared_address_space (struct gdbarch *gdbarch);
 extern void set_gdbarch_has_shared_address_space (struct gdbarch *gdbarch, gdbarch_has_shared_address_space_ftype *has_shared_address_space);
 
+/* True if the target has memory protection. */
+
+typedef int (gdbarch_has_memory_protection_ftype) (struct gdbarch *gdbarch);
+extern int gdbarch_has_memory_protection (struct gdbarch *gdbarch);
+extern void set_gdbarch_has_memory_protection (struct gdbarch *gdbarch, gdbarch_has_memory_protection_ftype *has_memory_protection);
+
 /* True if a fast tracepoint can be set at an address. */
 
 typedef int (gdbarch_fast_tracepoint_valid_at_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr, int *isize, char **msg);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 04ca38c..080a5ec 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -932,6 +932,9 @@ v:int:has_global_breakpoints:::0:0::0
 # True if inferiors share an address space (e.g., uClinux).
 m:int:has_shared_address_space:void:::default_has_shared_address_space::0
 
+# True if the target has memory protection.
+m:int:has_memory_protection:void:::default_has_memory_protection::0
+
 # True if a fast tracepoint can be set at an address.
 m:int:fast_tracepoint_valid_at:CORE_ADDR addr, int *isize, char **msg:addr, isize, msg::default_fast_tracepoint_valid_at::0
 
diff --git a/gdb/target.c b/gdb/target.c
index 37db36b..955a292 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -168,10 +168,10 @@ struct target_ops current_target;
 
 static struct cmd_list_element *targetlist = NULL;
 
-/* Nonzero if we should trust readonly sections from the
+/* If AUTO_BOOLEAN_TRUE, we should trust readonly sections from the
    executable when reading memory.  */
 
-static int trust_readonly = 0;
+static enum auto_boolean trust_readonly = AUTO_BOOLEAN_AUTO;
 
 /* Nonzero if we should show true memory content including
    memory breakpoint inserted by gdb.  */
@@ -1437,6 +1437,22 @@ memory_xfer_live_readonly_partial (struct target_ops *ops,
   return 0;
 }
 
+/* Return true if GDB trusts readonly sections.  Return false
+   otherwise.  */
+
+static int
+trust_readonly_p (void)
+{
+  if (trust_readonly != AUTO_BOOLEAN_AUTO)
+    return trust_readonly == AUTO_BOOLEAN_TRUE;
+
+  /* We only trust readonly sections when GDB is dong remote debugging
+     on the targets which have memory protection.  */
+  return (gdbarch_has_memory_protection (target_gdbarch ())
+	  && (strcmp (target_shortname, "remote") == 0
+	      || strcmp (target_shortname, "extended-remote") == 0));
+}
+
 /* Perform a partial memory transfer.
    For docs see target.h, to_xfer_partial.  */
 
@@ -1475,7 +1491,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
      files.  When GDB writes to a readonly section of the inferior,
      emit a warning that option 'trust-readonly-sections' should be
      turned off.  */
-  if (trust_readonly)
+  if (trust_readonly_p ())
     {
       struct target_section *secp;
 
@@ -1502,7 +1518,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	    {
 	      if (query (_("Address is read-only and trust-readonly-sections\
  is set to \"on\".\nSet it to \"off\" and write to a read-only section? ")))
-		trust_readonly = 0;
+		trust_readonly = AUTO_BOOLEAN_FALSE;
 	      else
 		return -1;
 	    }
@@ -5103,16 +5119,19 @@ command."),
 			     show_targetdebug,
 			     &setdebuglist, &showdebuglist);
 
-  add_setshow_boolean_cmd ("trust-readonly-sections", class_support,
-			   &trust_readonly, _("\
+  add_setshow_auto_boolean_cmd ("trust-readonly-sections", class_support,
+				&trust_readonly, _("\
 Set mode for reading from readonly sections."), _("\
 Show mode for reading from readonly sections."), _("\
 When this mode is on, memory reads from readonly sections (such as .text)\n\
 will be read from the object file instead of from the target.  This will\n\
-result in significant performance improvement for remote targets."),
-			   NULL,
-			   show_trust_readonly,
-			   &setlist, &showlist);
+result in significant performance improvement for remote targets.\n\
+When this mode is auto, GDB will decide based on the target memory\n\
+protection features whether to read readonly sections from object file\n\
+instead of from the inferior's memory in remote debugging.\n"),
+				NULL,
+				show_trust_readonly,
+				&setlist, &showlist);
 
   add_com ("monitor", class_obscure, do_monitor_command,
 	   _("Send a command to the remote monitor (remote targets only)."));
-- 
1.7.7.6

[PATCH 6/7] Linux has memory protection.

[Yao Qi]

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* linux-tdep.c (linux_has_memory_protection): New function.
	(linux_init_abi): Register linux_has_memory_protection
	to gdbarch 'has_memory_protection'.
---
 gdb/linux-tdep.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index eb8ea2b..36e7119 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -259,6 +259,15 @@ linux_has_shared_address_space (struct gdbarch *gdbarch)
   return linux_is_uclinux ();
 }
 
+/* This is the implementation of gdbarch method has_memory_protection.  */
+
+static int
+linux_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Normal linux has memory protection, while uClinux doesn't.  */
+  return !linux_is_uclinux ();
+}
+
 /* This is how we want PTIDs from core files to be printed.  */
 
 static char *
@@ -1783,6 +1792,7 @@ linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 				      linux_gdb_signal_from_target);
   set_gdbarch_gdb_signal_to_target (gdbarch,
 				    linux_gdb_signal_to_target);
+  set_gdbarch_has_memory_protection (gdbarch, linux_has_memory_protection);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
-- 
1.7.7.6

[PATCH 1/7] Emit a query when writing to a readonly section and trust_readonly is true

[Yao Qi]

If users write readonly section, such as .text, the contents of the inferior
and of the executable become out of sync.  It is better to emit a query
to ask users to "set trust-readonly-sections off".

(gdb) set trust-readonly-sections on
(gdb) p /x* (char *) 0x080484c1 = 0xcc
Address is read-only and trust-readonly-sections is set to "on".
Set it to "off" and write to a read-only section? (y or n) y
$1 = 0xcc

gdb:

2013-09-20  Yao Qi  <yao@codesourcery.com>

	* target.c (memory_xfer_partial_1): Emit a query if GDB
	writes to a readonly section and 'trust_readonly' is true.

gdb/testsuite:

2013-09-20  Yao Qi  <yao@codesourcery.com>

	* gdb.base/trust-readonly.exp: New.
---
 gdb/target.c                              |   35 +++++++++---
 gdb/testsuite/gdb.base/trust-readonly.exp |   85 +++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/trust-readonly.exp

diff --git a/gdb/target.c b/gdb/target.c
index d55712d..37db36b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1471,11 +1471,13 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	}
     }
 
-  /* Try the executable files, if "trust-readonly-sections" is set.  */
-  if (readbuf != NULL && trust_readonly)
+  /* If "trust-readonly-sections" is set, read from the executable
+     files.  When GDB writes to a readonly section of the inferior,
+     emit a warning that option 'trust-readonly-sections' should be
+     turned off.  */
+  if (trust_readonly)
     {
       struct target_section *secp;
-      struct target_section_table *table;
 
       secp = target_section_by_addr (ops, memaddr);
       if (secp != NULL
@@ -1483,12 +1485,27 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 				     secp->the_bfd_section)
 	      & SEC_READONLY))
 	{
-	  table = target_get_section_table (ops);
-	  return section_table_xfer_memory_partial (readbuf, writebuf,
-						    memaddr, len,
-						    table->sections,
-						    table->sections_end,
-						    NULL);
+
+	  if (readbuf != NULL)
+	    {
+	      struct target_section_table *table;
+
+	      table = target_get_section_table (ops);
+	      return section_table_xfer_memory_partial (readbuf, writebuf,
+							memaddr, len,
+							table->sections,
+							table->sections_end,
+							NULL);
+	    }
+
+	  if (writebuf != NULL)
+	    {
+	      if (query (_("Address is read-only and trust-readonly-sections\
+ is set to \"on\".\nSet it to \"off\" and write to a read-only section? ")))
+		trust_readonly = 0;
+	      else
+		return -1;
+	    }
 	}
     }
 
diff --git a/gdb/testsuite/gdb.base/trust-readonly.exp b/gdb/testsuite/gdb.base/trust-readonly.exp
new file mode 100644
index 0000000..8627800
--- /dev/null
+++ b/gdb/testsuite/gdb.base/trust-readonly.exp
@@ -0,0 +1,85 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile start.c
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Could not compile $binfile."
+    return -1
+}
+
+clean_restart ${binfile}
+
+if ![runto_main] then {
+    return -1
+}
+
+set main_address ""
+set test "Extract main address"
+gdb_test_multiple "print *main" $test {
+    -re "\} ($hex) .*$gdb_prompt $" {
+	pass $test
+	set main_address $expect_out(1,string)
+    }
+    -re ".*$gdb_prompt $" {
+	# Unable to extract address of main.  Bail out.
+	fail $test
+	return -1
+    }
+}
+
+set main_contents ""
+set test "Extract main contents"
+gdb_test_multiple "print/x *(char *) $main_address" $test {
+    -re "\\$$decimal = ($hex).*$gdb_prompt $" {
+	pass $test
+	set main_contents $expect_out(1,string)
+    }
+    -re ".*$gdb_prompt $" {
+	# Unable to extract contents of main.  Bail out.
+	fail $test
+	return -1
+    }
+}
+
+# Set option "trust-readonly-sections" off, write one byte and read it
+# back.
+
+gdb_test_no_output "set trust-readonly-sections off" ""
+gdb_test "print /x* (char *) $main_address = 0xcc" "\\$$decimal = 0xcc" \
+    "write to main (off)"
+gdb_test "print /x * (char *) $main_address" "\\$$decimal = 0xcc" \
+    "read from main (off) 1"
+
+gdb_test_no_output "set trust-readonly-sections on" ""
+gdb_test "print /x* (char *) $main_address = 0x11" "\\$$decimal = 0x11" \
+    "write to main (on)" \
+    "Address is read-only and trust-readonly-sections is set to \"on\"\..*y or n. $" \
+    "y"
+
+# Option "trust-readonly-sections" is turned off as answer "y" is
+# entered above.
+gdb_test "show trust-readonly-sections" \
+    "Mode for reading from readonly sections is off\."
+
+gdb_test "print /x * (char *) $main_address" "\\$$decimal = 0x11" \
+    "read from main (off) 2"
+
+# Set option "trust-readonly-sections" on, and test GDB read data from
+# the executable file instead of the inferior.
+
+gdb_test_no_output "set trust-readonly-sections on" ""
+gdb_test "print /x * (char *) $main_address" "\\$$decimal = ${main_contents}" \
+    "read from main (on)"
-- 
1.7.7.6

[PATCH 7/7] Windows has memory protection

[Yao Qi]

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* windows-tdep.c (windows_has_memory_protection): New function.
	(windows_init_abi): Install windows_has_memory_protection to
	gdbarch method 'has_memory_protection'.
---
 gdb/windows-tdep.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 50c5795..93d6fe6 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -481,6 +481,18 @@ init_w32_command_list (void)
     }
 }
 
+/* This is the implementation of gdbarch method has_memory_protection.  */
+
+static int
+windows_has_memory_protection (struct gdbarch *gdbarch)
+{
+  /* Modern Windows has memory protection.  However, some ancient
+     versions of Windows don't have full memory protection, such as
+     Windows 9x.  If necessary, GDB should detect the version of
+     Windows and return false if the target OS is Windows 9x.  */
+  return 1;
+}
+
 /* To be called from the various GDB_OSABI_CYGWIN handlers for the
    various Windows architectures and machine types.  */
 
@@ -493,6 +505,9 @@ windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   set_gdbarch_iterate_over_objfiles_in_search_order
     (gdbarch, windows_iterate_over_objfiles_in_search_order);
+
+  set_gdbarch_has_memory_protection (gdbarch,
+				     windows_has_memory_protection);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
-- 
1.7.7.6

[PATCH 5/7] DOC and NEWS

[Yao Qi]

 -V3: Mention "in remote debugging".

gdb:

2013-09-20  Yao Qi  <yao@codesourcery.com>

	* NEWS: Describe the default option of
	"trust-readonly-sections" becomes "auto" and the related
	changes.

gdb/doc:

2013-09-20  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (File): Mention that user has
	'set trust-readonly-sections off' to get updated contents if
	they are modified.  Explain the default option of
	"trust-readonly-sections" is "auto".
---
 gdb/NEWS            |    5 +++++
 gdb/doc/gdb.texinfo |   12 ++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 5eb046b..5a70adc 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,11 @@
 
 *** Changes since GDB 7.6
 
+* The default value of option "trust-readonly-sections" is "auto".  In
+  remote debugging, GDB will decide based on the target memory
+  protection features whether to read readonly sections from object
+  file instead of from the inferior's memory.
+
 * The "maintenance print objfiles" command now takes an optional regexp.
 
 * The "catch syscall" command now works on arm*-linux* targets.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a8c854e..a26160e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -16673,12 +16673,20 @@ out of the object file, rather than from the target program.
 For some targets (notably embedded ones), this can be a significant
 enhancement to debugging performance.
 
-The default is off.
+@item set trust-readonly-sections auto
+This is the default mode.  In remote debugging, @value{GDBN} will
+decide based on the target memory protection features whether to
+read readonly sections from object file instead of from the inferior's
+memory, because the contents of the section in the inferior can't
+change.
 
 @item set trust-readonly-sections off
 Tell @value{GDBN} not to trust readonly sections.  This means that
 the contents of the section might change while the program is running,
-and must therefore be fetched from the target when needed.
+and must therefore be fetched from the target when needed.  If you
+modified the code in the target program, you have to
+@code{set trust-readonly-sections off} to guarantee @value{GDBN} reads
+updated contents from the target program instead of object file.
 
 @item show trust-readonly-sections
 Show the current setting of trusting readonly sections.
-- 
1.7.7.6

[PATCH 2/7] set trust-readonly-sections off in test cases

[Yao Qi]

In the test cases, if GDB modified code in the inferior, it should
'set trust-readonly-sections off'.

gdb/testsuite:

2013-09-20  Yao Qi  <yao@codesourcery.com>

	* gdb.base/break-always.exp: Set trust-readonly-sections off.
	* gdb.mi/mi-fill-memory.exp: Likewise.
---
 gdb/testsuite/gdb.base/break-always.exp |    4 ++++
 gdb/testsuite/gdb.mi/mi-fill-memory.exp |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.base/break-always.exp b/gdb/testsuite/gdb.base/break-always.exp
index b0a9faf..16f6d47 100644
--- a/gdb/testsuite/gdb.base/break-always.exp
+++ b/gdb/testsuite/gdb.base/break-always.exp
@@ -69,6 +69,10 @@ gdb_test "p /x \$shadow = *(char *) $bp_address" \
 # and still leave the breakpoint insn planted.  Try twice with
 # different values, in case we happen to be writting exactly what was
 # there already.
+# Turn option "trust-readonly-sections" off to force GDB to read
+# contents from the target instead of the object file.
+gdb_test_no_output "set trust-readonly-sections off"
+
 gdb_test "p /x *(char *) $bp_address = 0" \
     " = 0x0" \
     "write 0 to breakpoint's address"
diff --git a/gdb/testsuite/gdb.mi/mi-fill-memory.exp b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
index 5f68cbb..2b7408a 100644
--- a/gdb/testsuite/gdb.mi/mi-fill-memory.exp
+++ b/gdb/testsuite/gdb.mi/mi-fill-memory.exp
@@ -35,6 +35,11 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}.c" "${binfile}" executable {d
 mi_run_to_main
 mi_next_to "main" "" "mi-read-memory.c" "20" "next at main"
 
+# Turn option "trust-readonly-sections" off to force GDB to read
+# contents from the target instead of the object file.
+mi_gdb_test "-gdb-set trust-readonly-sections off" "\\\^done" \
+    "set trust-readonly-sections off"
+
 mi_gdb_test "1-data-write-memory-bytes"\
 	"1\\\^error,msg=\"Usage: ADDR DATA \\\[COUNT\\\]\.\""\
 	"no arguments"
-- 
1.7.7.6

[PATCH 3/7] New function windows_init_abi

[Yao Qi]

Hi,
When I look for a function, which does all abi-related initialization in
windows-tdep.c, it doesn't exist.  It would be good to create such
function, similar to linux_init_abi, and put all windows abi related
initialization in it.

This patch adds a new function windows_init_abi and move these two
functions calls in it,

  set_gdbarch_has_dos_based_file_system (gdbarch, 1);

  set_gdbarch_iterate_over_objfiles_in_search_order
    (gdbarch, windows_iterate_over_objfiles_in_search_order);

amd64-windows doesn't set gdbarch flag has_dos_based_file_system, so
with this patch applied, this flag is set for amd64-windows.
I build GDB with "--host=i686-pc-linux-gnu --target=x86_64-mingw32",
and start GDB with a x86_64 PE executable,

Without this patch, we get,
(gdb) show target-file-system-kind
The assumed file system kind for target reported file names is "auto" (currently "unix").

With this path, we get,
(gdb) show target-file-system-kind
The assumed file system kind for target reported file names is "auto" (currently "dos-based").

As the result of this patch, windows_iterate_over_objfiles_in_search_order
can be made static too.

I don't have a x86_64-mingw32 env to test this patch.

gdb:

2013-09-08  Yao Qi  <yao@codesourcery.com>

	* amd64-windows-tdep.c (amd64_windows_init_abi): Don't call
	set_gdbarch_iterate_over_objfiles_in_search_order.  Call
	windows_init_abi instead.
	* i386-cygwin-tdep.c (i386_cygwin_init_abi): Don't call
	set_gdbarch_has_dos_based_file_system and
	set_gdbarch_iterate_over_objfiles_in_search_order.  Call
	windows_init_abi instead.
	* windows-tdep.c (windows_init_abi): New function.
	(windows_iterate_over_objfiles_in_search_order): Make it
	static.
	* windows-tdep.h (windows_init_abi): Declare.
	(windows_iterate_over_objfiles_in_search_order): Remove
	declaration.
---
 gdb/amd64-windows-tdep.c |    5 ++---
 gdb/i386-cygwin-tdep.c   |    9 ++-------
 gdb/windows-tdep.c       |   16 +++++++++++++++-
 gdb/windows-tdep.h       |    6 ++----
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 4e750a1..0dfc906 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -972,6 +972,8 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   amd64_init_abi (info, gdbarch);
 
+  windows_init_abi (info, gdbarch);
+
   /* On Windows, "long"s are only 32bit.  */
   set_gdbarch_long_bit (gdbarch, 32);
 
@@ -987,9 +989,6 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_skip_trampoline_code (gdbarch,
 				    amd64_windows_skip_trampoline_code);
 
-  set_gdbarch_iterate_over_objfiles_in_search_order
-    (gdbarch, windows_iterate_over_objfiles_in_search_order);
-
   set_gdbarch_skip_prologue (gdbarch, amd64_windows_skip_prologue);
 
   set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charset);
diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-cygwin-tdep.c
index a3e4e7c..01fc15b 100644
--- a/gdb/i386-cygwin-tdep.c
+++ b/gdb/i386-cygwin-tdep.c
@@ -233,6 +233,8 @@ i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
+  windows_init_abi (info, gdbarch);
+
   set_gdbarch_skip_trampoline_code (gdbarch, i386_cygwin_skip_trampoline_code);
 
   set_gdbarch_skip_main_prologue (gdbarch, i386_skip_main_prologue);
@@ -253,13 +255,6 @@ i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_core_pid_to_str (gdbarch, i386_windows_core_pid_to_str);
 
   set_gdbarch_auto_wide_charset (gdbarch, i386_cygwin_auto_wide_charset);
-
-  /* Canonical paths on this target look like
-     `c:\Program Files\Foo App\mydll.dll', for example.  */
-  set_gdbarch_has_dos_based_file_system (gdbarch, 1);
-
-  set_gdbarch_iterate_over_objfiles_in_search_order
-    (gdbarch, windows_iterate_over_objfiles_in_search_order);
 }
 
 static enum gdb_osabi
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index f90323f..50c5795 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -427,7 +427,7 @@ windows_xfer_shared_library (const char* so_name, CORE_ADDR load_addr,
    to print the value of another global variable defined with the same
    name, but in a different DLL.  */
 
-void
+static void
 windows_iterate_over_objfiles_in_search_order
   (struct gdbarch *gdbarch,
    iterate_over_objfiles_in_search_order_cb_ftype *cb,
@@ -481,6 +481,20 @@ init_w32_command_list (void)
     }
 }
 
+/* To be called from the various GDB_OSABI_CYGWIN handlers for the
+   various Windows architectures and machine types.  */
+
+void
+windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  /* Canonical paths on this target look like
+     `c:\Program Files\Foo App\mydll.dll', for example.  */
+  set_gdbarch_has_dos_based_file_system (gdbarch, 1);
+
+  set_gdbarch_iterate_over_objfiles_in_search_order
+    (gdbarch, windows_iterate_over_objfiles_in_search_order);
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_windows_tdep;
 
diff --git a/gdb/windows-tdep.h b/gdb/windows-tdep.h
index 99136fa..7b04bb2 100644
--- a/gdb/windows-tdep.h
+++ b/gdb/windows-tdep.h
@@ -30,8 +30,6 @@ extern void windows_xfer_shared_library (const char* so_name,
 					 struct gdbarch *gdbarch,
 					 struct obstack *obstack);
 
-extern void windows_iterate_over_objfiles_in_search_order
-  (struct gdbarch *gdbarch,
-   iterate_over_objfiles_in_search_order_cb_ftype *cb,
-   void *cb_data, struct objfile *current_objfile);
+extern void windows_init_abi (struct gdbarch_info info,
+			      struct gdbarch *gdbarch);
 #endif
-- 
1.7.7.6

Re: [PATCH 5/7] DOC and NEWS

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 20 Sep 2013 10:47:04 +0800
> 
> 
>  -V3: Mention "in remote debugging".
> 
> gdb:
> 
> 2013-09-20  Yao Qi  <yao@codesourcery.com>
> 
> 	* NEWS: Describe the default option of
> 	"trust-readonly-sections" becomes "auto" and the related
> 	changes.
> 
> gdb/doc:
> 
> 2013-09-20  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (File): Mention that user has
> 	'set trust-readonly-sections off' to get updated contents if
> 	they are modified.  Explain the default option of
> 	"trust-readonly-sections" is "auto".

This is OK, but should we perhaps mention that an attempt to write to
a read-only section will cause the prompt to turn this option off?  I
would mention that prompt as part of this text:

>  @item set trust-readonly-sections off
>  Tell @value{GDBN} not to trust readonly sections.  This means that
>  the contents of the section might change while the program is running,
> -and must therefore be fetched from the target when needed.
> +and must therefore be fetched from the target when needed.  If you
> +modified the code in the target program, you have to
> +@code{set trust-readonly-sections off} to guarantee @value{GDBN} reads
> +updated contents from the target program instead of object file.

Thanks.

Re: [PATCH 0/7 V3] Trust readonly sections if target has memory protection

[Yao Qi]

On 09/20/2013 10:46 AM, Yao Qi wrote:
> Here is the V3, to address two comments to V2.
>
>   - Give a yes-or-no query when read-only address is modified and
>     trust-readonly-sections is on.  See patch 1/7.  A test case
>     gdb.base/trust-readonly.exp is added too.
>   - Enable trust-readonly-sections on all linux and windows targets
>     for remote debugging only.  It is different from what I proposed,
>     {x86, x86_64}-{linux,mingw,cygwin}, because it looks odd to enable
>     this feature only on x86*-linux targets and disable it on the rest of
>     linux targets.  See patch 4/7.
>
> Patch 2/7, 3/7, 6/7 and 7/7 are unchanged.  Since the condition on
> trusting readonly-sections is restricted (only on remote debugging),
> so the doc (patch 5/7) is updated as well.

Ping.  https://sourceware.org/ml/gdb-patches/2013-09/msg00714.html

-- 
Yao (齐尧)

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Pedro Alves]

On 09/13/2013 09:16 AM, Yao Qi wrote:
> On 09/12/2013 05:49 PM, Mark Kettenis wrote:
>> I'm certainly not outright rejecting it.  But you'll certainly need to
>> rethink in which contexts it is safe/acceptable that "auto" actually
>> turns on the trust-readonly-sections feature.  That decision should
>> probably be done on a per-architecture, per-OS basis, and only for
>> remote debugging.
> 
> Yeah, that is reasonable to me.  Then, the proposed scope could be
> {x86, x86_64}-{linux,mingw,cygwin} for remote debugging only.  What do
> you think?

This makes me extremely nervous.

I think it's completely wrong to distinguish local/remote debugging.
Local or remote doesn't make the feature more or less safe.  We should
aim for local/remote feature (and experience/safeness) parity.  At some
point, native debugging may well always go through a "remote" server
behind the scenes (like connecting to a local gdbserver that gdb itself
spawns), so we could see this enablement now for remote targets as
a (unintended) "trojan" for having it silently enabled for native
targets at some point.  Having the debug session behave differently
with a native target vs a remote target is just fundamentally wrong, IMO.
I don't want to have to say or explain to people that they should
"debug natively to be safer".

I've been background-thinking about taking a step back and understand
why each use case that is sped up with this patch is slow to begin with.
That is, the series jumps to a solution, but we haven't done our due diligence
with analyzing the problem thoroughly, IMO.  E.g., for the disassembly use
case, presented in the v1 series, I'd think that the problem is that GDB is
fetching data off the target instruction by instruction, instead of requesting
a block of memory and working with that.  More aggressive over fetching
could be a better/safer approach.

We have similar infrastructure already, in dcache.c -- we use
it for stack memory nowadays, and if the memory region is marked as
cacheable.  We used to support caching more than just stack, but
that was never enabled by default because it may not be safe to
read memory outside of the range the caller is specifying, because of
things like memory mapped registers, etc.  (In the case of stack, we assume
stack is allocated in page chunks, so that dcache never steps on memory is
should not).  But in cases like disassembly, we're being driven by debug
info or user input.  As GDB knows upfront the whole range of memory it'll
be fetching, accessing a bigger chunk upfront, as long as it doesn't
step out of the range we read piecemeal anyway, should have no effect
on correctness.

Another orthogonal idea (we could/should have both) would be to check
whether sections/blocks/pages of memory haven't been changed in the target
with  target_verify_memory/qCRC packet, and iff not, fall back to reading
from the file.  IOW, don't fallback to reading from file if the memory has
been changed in the target (or if we can't tell).

These things could in addition be predicated on whether the target
is completely stopped (we have a crude version of that today in
prepare_execute_command).

-- 
Pedro Alves

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Pedro Alves]

On 09/30/2013 06:50 PM, Pedro Alves wrote:

> But in cases like disassembly, we're being driven by debug
> info or user input.  As GDB knows upfront the whole range of memory it'll
> be fetching, accessing a bigger chunk upfront, as long as it doesn't
> step out of the range we read piecemeal anyway, should have no effect
> on correctness.

Hmm, wait, I'm having a a déjà vu experience.  I recalled I have
once reviewed a patch that does exactly this.  But, I'm not finding
it in the tree, or in the archives.  Maybe it was a CS local patch
that was never pushed?

-- 
Pedro Alves

Re: [PATCH 3/7] New function windows_init_abi

[Pedro Alves]

On 09/20/2013 03:47 AM, Yao Qi wrote:

> 2013-09-08  Yao Qi  <yao@codesourcery.com>
> 
> 	* amd64-windows-tdep.c (amd64_windows_init_abi): Don't call
> 	set_gdbarch_iterate_over_objfiles_in_search_order.  Call
> 	windows_init_abi instead.
> 	* i386-cygwin-tdep.c (i386_cygwin_init_abi): Don't call
> 	set_gdbarch_has_dos_based_file_system and
> 	set_gdbarch_iterate_over_objfiles_in_search_order.  Call
> 	windows_init_abi instead.
> 	* windows-tdep.c (windows_init_abi): New function.
> 	(windows_iterate_over_objfiles_in_search_order): Make it
> 	static.
> 	* windows-tdep.h (windows_init_abi): Declare.
> 	(windows_iterate_over_objfiles_in_search_order): Remove
> 	declaration.

This looks like a good idea on its own, and could go in independently
of the rest of the series.

arm-wince-pe.c:arm_wince_init_abi should call windows_init_abi too.
That fact that that doesn't call
set_gdbarch_iterate_over_objfiles_in_search_order today looks like
exactly the sort of issue that a patch like this one would prevent.

The patch looks OK to me with that change.

['set_solib_ops (gdbarch, &solib_target_so_ops)' should be able to
move to windows_init_abi too.]

-- 
Pedro Alves

Re: [PATCH 3/7] New function windows_init_abi

[Yao Qi]

On 10/01/2013 02:23 AM, Pedro Alves wrote:
> arm-wince-pe.c:arm_wince_init_abi should call windows_init_abi too.
> That fact that that doesn't call
> set_gdbarch_iterate_over_objfiles_in_search_order today looks like
> exactly the sort of issue that a patch like this one would prevent.

windows-tdep.o is not linked for arm-wince-pe target.  In
configure.target:

arm*-wince-pe | arm*-*-mingw32ce*)
	# Target: ARM based machine running Windows CE (win32)
	gdb_target_obs="arm-tdep.o arm-wince-tdep.o"
	build_gdbserver=yes

I am not sure the TIB stuff in windows-tdep.c can be applicable to ARM
wince target.  It was discussed in the review to the patch

  [RFC-v2] Add windows Thread Information Block
  https://sourceware.org/ml/gdb-patches/2009-07/msg00010.html

windows-tdep.c looks quite target-independent, except some comments,
in which "fs" and "gs" is mentioned.  How about this patch below?
windows-tdep.o is linked in target arm*-wince-pe and
arm*-*-mingw32ce*, supposing set_solib_ops has been moved to
windows_init_abi.
> 
> The patch looks OK to me with that change.
> 
> ['set_solib_ops (gdbarch, &solib_target_so_ops)' should be able to
> move to windows_init_abi too.]

OK.  I'll move it to windows_init_abi in the updated patch.

-- 
Yao (齐尧)

gdb:

2013-10-01  Yao Qi  <yao@codesourcery.com>

	* arm-wince-tdep.c: Remove inclusion of "solib.h" and
	"solib-target.h".  Include "windows-tdep.h".
	(arm_wince_init_abi): Call windows_init_abi.  Remove call to
	set_solib_ops and set_gdbarch_has_dos_based_file_system.
	* configure.tgt (arm*-wince-pe | arm*-*-mingw32ce*): Append
	windows-tdep.o to gdb_target_obs.
---
 gdb/arm-wince-tdep.c |   11 +++--------
 gdb/configure.tgt    |    2 +-
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/gdb/arm-wince-tdep.c b/gdb/arm-wince-tdep.c
index 61569e3..163ec73 100644
--- a/gdb/arm-wince-tdep.c
+++ b/gdb/arm-wince-tdep.c
@@ -22,13 +22,12 @@
 #include "osabi.h"
 #include "gdbcore.h"
 #include "target.h"
-#include "solib.h"
-#include "solib-target.h"
 #include "frame.h"
 
 #include "gdb_string.h"
 
 #include "arm-tdep.h"
+#include "windows-tdep.h"
 
 static const gdb_byte arm_wince_le_breakpoint[] = { 0x10, 0x00, 0x00, 0xe6 };
 static const gdb_byte arm_wince_thumb_le_breakpoint[] = { 0xfe, 0xdf };
@@ -116,6 +115,8 @@ arm_wince_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
+  windows_init_abi (info, gdbarch);
+
   tdep->arm_breakpoint = arm_wince_le_breakpoint;
   tdep->arm_breakpoint_size = sizeof (arm_wince_le_breakpoint);
   tdep->thumb_breakpoint = arm_wince_thumb_le_breakpoint;
@@ -130,8 +131,6 @@ arm_wince_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* On ARM WinCE char defaults to signed.  */
   set_gdbarch_char_signed (gdbarch, 1);
 
-  /* Shared library handling.  */
-  set_solib_ops (gdbarch, &solib_target_so_ops);
   set_gdbarch_skip_trampoline_code (gdbarch, arm_pe_skip_trampoline_code);
 
   /* Single stepping.  */
@@ -139,10 +138,6 @@ arm_wince_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   /* Skip call to __gccmain that gcc places in main.  */
   set_gdbarch_skip_main_prologue (gdbarch, arm_wince_skip_main_prologue);
-
-  /* Canonical paths on this target look like `\Windows\coredll.dll',
-     for example.  */
-  set_gdbarch_has_dos_based_file_system (gdbarch, 1);
 }
 
 static enum gdb_osabi
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 95c7217..ea0faf1 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -83,7 +83,7 @@ am33_2.0*-*-linux*)
 
 arm*-wince-pe | arm*-*-mingw32ce*)
 	# Target: ARM based machine running Windows CE (win32)
-	gdb_target_obs="arm-tdep.o arm-wince-tdep.o"
+	gdb_target_obs="arm-tdep.o arm-wince-tdep.o windows-tdep.o"
 	build_gdbserver=yes
 	;;
 arm*-*-linux*)
-- 
1.7.7.6

Re: [PATCH 3/7] New function windows_init_abi

[Pedro Alves]

On 10/01/2013 07:46 AM, Yao Qi wrote:
> On 10/01/2013 02:23 AM, Pedro Alves wrote:
>> arm-wince-pe.c:arm_wince_init_abi should call windows_init_abi too.
>> That fact that that doesn't call
>> set_gdbarch_iterate_over_objfiles_in_search_order today looks like
>> exactly the sort of issue that a patch like this one would prevent.
> 
> windows-tdep.o is not linked for arm-wince-pe target.  In
> configure.target:
> 
> arm*-wince-pe | arm*-*-mingw32ce*)
> 	# Target: ARM based machine running Windows CE (win32)
> 	gdb_target_obs="arm-tdep.o arm-wince-tdep.o"
> 	build_gdbserver=yes
> 
> I am not sure the TIB stuff in windows-tdep.c can be applicable to ARM
> wince target.  It was discussed in the review to the patch
> 
>   [RFC-v2] Add windows Thread Information Block
>   https://sourceware.org/ml/gdb-patches/2009-07/msg00010.html
> 
> windows-tdep.c looks quite target-independent, except some comments,
> in which "fs" and "gs" is mentioned.  How about this patch below?

Yeah, %fs and %gs of course are only applicable to x86/amd64.  I don't
recall if the TIB structure on x86 WinCE is the same as the x86 desktop
Windows , and/or whether the ARM/x86 WinCE versions have the same
layout (ISTR not), or even what's the story for the new ARM WinRT (which I'm
assuming will a GDB port to soon enough).  In any case, it's windows_get_tlb_type
that will probably need to evolve and be moved to arch tdep files.  Making all
the Windows builds link in windows-tdep.c is definitely the correct
thing to do.

> windows-tdep.o is linked in target arm*-wince-pe and
> arm*-*-mingw32ce*, supposing set_solib_ops has been moved to
> windows_init_abi.
>>
>> The patch looks OK to me with that change.
>>
>> ['set_solib_ops (gdbarch, &solib_target_so_ops)' should be able to
>> move to windows_init_abi too.]
> 
> OK.  I'll move it to windows_init_abi in the updated patch.
> 

> -  /* Shared library handling.  */

Leave this one.  "skip_trampoline_code" is also related to shared library
handling.

> -  set_solib_ops (gdbarch, &solib_target_so_ops);
>    set_gdbarch_skip_trampoline_code (gdbarch, arm_pe_skip_trampoline_code);

Otherwise OK.

Thanks!

-- 
Pedro Alves

Re: [PATCH 3/7] New function windows_init_abi

[Yao Qi]

On 10/01/2013 05:35 PM, Pedro Alves wrote:
>> -  /* Shared library handling.  */
> Leave this one.  "skip_trampoline_code" is also related to shared library
> handling.

OK.

>
>> >-  set_solib_ops (gdbarch, &solib_target_so_ops);
>> >    set_gdbarch_skip_trampoline_code (gdbarch, arm_pe_skip_trampoline_code);
> Otherwise OK.

Patch is committed.

-- 
Yao (齐尧)

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Stan Shebs]

On 9/30/13 11:08 AM, Pedro Alves wrote:
> On 09/30/2013 06:50 PM, Pedro Alves wrote:
> 
>> But in cases like disassembly, we're being driven by debug
>> info or user input.  As GDB knows upfront the whole range of memory it'll
>> be fetching, accessing a bigger chunk upfront, as long as it doesn't
>> step out of the range we read piecemeal anyway, should have no effect
>> on correctness.
> 
> Hmm, wait, I'm having a a déjà vu experience.  I recalled I have
> once reviewed a patch that does exactly this.  But, I'm not finding
> it in the tree, or in the archives.  Maybe it was a CS local patch
> that was never pushed?
> 

There are a couple possibilities - for instance there we had a request
to check that the code underneath a breakpoint had not changed behind
our backs, or something like that.  There was also the check of readonly
areas for memory_xfer_partial reading from a traceframe, but
that was your code, and it went into FSF in February 2011. :-)

Stan
stan@codesourcery.com

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Pedro Alves]

On 10/07/2013 11:29 PM, Stan Shebs wrote:
> On 9/30/13 11:08 AM, Pedro Alves wrote:
>> On 09/30/2013 06:50 PM, Pedro Alves wrote:
>>
>>> But in cases like disassembly, we're being driven by debug
>>> info or user input.  As GDB knows upfront the whole range of memory it'll
>>> be fetching, accessing a bigger chunk upfront, as long as it doesn't
>>> step out of the range we read piecemeal anyway, should have no effect
>>> on correctness.
>>
>> Hmm, wait, I'm having a a déjà vu experience.  I recalled I have
>> once reviewed a patch that does exactly this.  But, I'm not finding
>> it in the tree, or in the archives.  Maybe it was a CS local patch
>> that was never pushed?
>>
> 
> There are a couple possibilities - for instance there we had a request
> to check that the code underneath a breakpoint had not changed behind
> our backs, or something like that.  There was also the check of readonly
> areas for memory_xfer_partial reading from a traceframe, but
> that was your code, and it went into FSF in February 2011. :-)

Hmm, yeah, but not, that's not really what I'm recalling.
I distinctly remember reviewing this for the disassemble command, exactly
as I was suggesting.  I now recall better because I also hacked on the
patch while iterating on multiple iterations of patch reviews.  The change
was contained to  just gdb/disasm.c (or the gross of it at least), IIRC.
Bleh, if we can't find it, we can always rewrite it.  :-P.

-- 
Pedro Alves

RE: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Abid, Hafiz]

> I distinctly remember reviewing this for the disassemble command, exactly
> as I was suggesting.  I now recall better because I also hacked on the patch
> while iterating on multiple iterations of patch reviews.  The change was
> contained to  just gdb/disasm.c (or the gross of it at least), IIRC.
> Bleh, if we can't find it, we can always rewrite it.  :-P.

Taimoor wrote a patch about disassembly improvement in April 2011. Pedro review this patch but I think it stayed in our internal repo and never went upstream. If there is interest, I can try to resurrect it.

Regards,
Abid

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Pedro Alves]

On 10/08/2013 01:47 PM, Abid, Hafiz wrote:
>> I distinctly remember reviewing this for the disassemble command, exactly
>> as I was suggesting.  I now recall better because I also hacked on the patch
>> while iterating on multiple iterations of patch reviews.  The change was
>> contained to  just gdb/disasm.c (or the gross of it at least), IIRC.
>> Bleh, if we can't find it, we can always rewrite it.  :-P.
> 
> Taimoor wrote a patch about disassembly improvement in April 2011. Pedro review this patch but I think it stayed in our internal repo and never went upstream. If there is interest, I can try to resurrect it.

Ah!  Thanks for digging that up.  Yeah, that'd be great.

Thanks,
-- 
Pedro Alves

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[tmirza]

On 10/08/2013 05:47 PM, Abid, Hafiz wrote:
>> I distinctly remember reviewing this for the disassemble command, exactly
>> as I was suggesting.  I now recall better because I also hacked on the patch
>> while iterating on multiple iterations of patch reviews.  The change was
>> contained to  just gdb/disasm.c (or the gross of it at least), IIRC.
>> Bleh, if we can't find it, we can always rewrite it.  :-P.
> Taimoor wrote a patch about disassembly improvement in April 2011. Pedro review this patch but I think it stayed in our internal repo and never went upstream. If there is interest, I can try to resurrect it.
Yeah, I remember it very well as it was my first patch and Pedro helped 
me a lot. A cache based strategy was implemented to improve performance 
of disassembler.

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Doug Evans]

On Mon, Sep 30, 2013 at 10:50 AM, Pedro Alves <palves@redhat.com> wrote:
> [...]
> We have similar infrastructure already, in dcache.c -- we use
> it for stack memory nowadays, and if the memory region is marked as
> cacheable.  We used to support caching more than just stack, but
> that was never enabled by default because it may not be safe to
> read memory outside of the range the caller is specifying, because of
> things like memory mapped registers, etc.  (In the case of stack, we assume
> stack is allocated in page chunks, so that dcache never steps on memory is
> should not).  But in cases like disassembly, we're being driven by debug
> info or user input.  As GDB knows upfront the whole range of memory it'll
> be fetching, accessing a bigger chunk upfront, as long as it doesn't
> step out of the range we read piecemeal anyway, should have no effect
> on correctness.

For reference sake,
ISTR prologue parsing being another place where we saw pain (not
often, but painful when it happens).

We also saw paradoxical slowdowns from using trust-readonly.
The culprit was target_section_by_addr.
Should be fixable of course, but heads up.

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Yao Qi]

On 10/01/2013 01:50 AM, Pedro Alves wrote:
> I've been background-thinking about taking a step back and understand
> why each use case that is sped up with this patch is slow to begin with.
> That is, the series jumps to a solution, but we haven't done our due diligence
> with analyzing the problem thoroughly, IMO.  E.g., for the disassembly use

Due diligence was done, but in a different way :).  Option 
"trust-readonly-sections" was mentioned to improve the performance of 
remote debugging and "turning it on in default" was discussed when this 
option was added.  We didn't turn it on in default because of bad code 
on non-memory protection systems.  V1 follows this idea to let GDB to 
know which targets are memory protection systems.

> case, presented in the v1 series, I'd think that the problem is that GDB is
> fetching data off the target instruction by instruction, instead of requesting
> a block of memory and working with that.  More aggressive over fetching
> could be a better/safer approach.

I agree.

>
> We have similar infrastructure already, in dcache.c -- we use
> it for stack memory nowadays, and if the memory region is marked as
> cacheable.  We used to support caching more than just stack, but
> that was never enabled by default because it may not be safe to
> read memory outside of the range the caller is specifying, because of
> things like memory mapped registers, etc.  (In the case of stack, we assume
> stack is allocated in page chunks, so that dcache never steps on memory is
> should not).  But in cases like disassembly, we're being driven by debug
> info or user input.  As GDB knows upfront the whole range of memory it'll
> be fetching, accessing a bigger chunk upfront, as long as it doesn't
> step out of the range we read piecemeal anyway, should have no effect
> on correctness.

We have to improve dcache.c at first.  Nowadays, dcache requests one 
cache line from the target in one time, regardless the actual size of 
memory requested.

-- 
Yao (齐尧)

Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Yao Qi]

On 10/09/2013 10:24 AM, Doug Evans wrote:
> We also saw paradoxical slowdowns from using trust-readonly.
> The culprit was target_section_by_addr.
> Should be fixable of course, but heads up.

Doug,
could you elaborate?  target_section_by_addr iterates sections in table, 
which shouldn't be expensive, unless the table is extremely long.

-- 
Yao (齐尧)