* [PATCH 0/3] Trust readonly sections if target has memory protection @ 2013-09-06 2:03 Yao Qi 2013-09-06 2:03 ` [PATCH 1/3] set trust-readonly-sections off in test cases Yao Qi ` (5 more replies) 0 siblings, 6 replies; 63+ messages in thread From: Yao Qi @ 2013-09-06 2:03 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/3] set trust-readonly-sections off in test cases 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi @ 2013-09-06 2:03 ` Yao Qi 2013-09-06 5:56 ` Eli Zaretskii 2013-09-06 17:23 ` Pedro Alves 2013-09-06 2:03 ` [PATCH 2/3] Trust readonly sections if target has memory protection Yao Qi ` (4 subsequent siblings) 5 siblings, 2 replies; 63+ messages in thread From: Yao Qi @ 2013-09-06 2:03 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/3] set trust-readonly-sections off in test cases 2013-09-06 2:03 ` [PATCH 1/3] set trust-readonly-sections off in test cases Yao Qi @ 2013-09-06 5:56 ` Eli Zaretskii 2013-09-06 17:23 ` Pedro Alves 1 sibling, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 5:56 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/3] set trust-readonly-sections off in test cases 2013-09-06 2:03 ` [PATCH 1/3] set trust-readonly-sections off in test cases Yao Qi 2013-09-06 5:56 ` Eli Zaretskii @ 2013-09-06 17:23 ` Pedro Alves 1 sibling, 0 replies; 63+ messages in thread From: Pedro Alves @ 2013-09-06 17:23 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 2/3] Trust readonly sections if target has memory protection 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi 2013-09-06 2:03 ` [PATCH 1/3] set trust-readonly-sections off in test cases Yao Qi @ 2013-09-06 2:03 ` Yao Qi 2013-09-06 6:05 ` Eli Zaretskii 2013-09-06 2:03 ` [PATCH 3/3] Linux " Yao Qi ` (3 subsequent siblings) 5 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-06 2:03 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/3] Trust readonly sections if target has memory protection 2013-09-06 2:03 ` [PATCH 2/3] Trust readonly sections if target has memory protection Yao Qi @ 2013-09-06 6:05 ` Eli Zaretskii 2013-09-06 9:07 ` Yao Qi 0 siblings, 1 reply; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 6:05 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/3] Trust readonly sections if target has memory protection 2013-09-06 6:05 ` Eli Zaretskii @ 2013-09-06 9:07 ` Yao Qi 2013-09-06 9:24 ` Eli Zaretskii 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-06 9:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 2/3] Trust readonly sections if target has memory protection 2013-09-06 9:07 ` Yao Qi @ 2013-09-06 9:24 ` Eli Zaretskii 0 siblings, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 9:24 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 3/3] Linux has memory protection. 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi 2013-09-06 2:03 ` [PATCH 1/3] set trust-readonly-sections off in test cases Yao Qi 2013-09-06 2:03 ` [PATCH 2/3] Trust readonly sections if target has memory protection Yao Qi @ 2013-09-06 2:03 ` Yao Qi 2013-09-06 5:57 ` [PATCH 0/3] Trust readonly sections if target " Eli Zaretskii ` (2 subsequent siblings) 5 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-06 2:03 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi ` (2 preceding siblings ...) 2013-09-06 2:03 ` [PATCH 3/3] Linux " Yao Qi @ 2013-09-06 5:57 ` Eli Zaretskii 2013-09-06 8:24 ` Yao Qi 2013-09-06 13:00 ` Joel Brobecker 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi 5 siblings, 1 reply; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 5:57 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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? ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 5:57 ` [PATCH 0/3] Trust readonly sections if target " Eli Zaretskii @ 2013-09-06 8:24 ` Yao Qi 2013-09-06 8:45 ` Eli Zaretskii 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-06 8:24 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 8:24 ` Yao Qi @ 2013-09-06 8:45 ` Eli Zaretskii 2013-09-06 13:03 ` Joel Brobecker 0 siblings, 1 reply; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 8:45 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 8:45 ` Eli Zaretskii @ 2013-09-06 13:03 ` Joel Brobecker 2013-09-06 13:27 ` Yao Qi 2013-09-06 13:32 ` Eli Zaretskii 0 siblings, 2 replies; 63+ messages in thread From: Joel Brobecker @ 2013-09-06 13:03 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Yao Qi, gdb-patches > 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 13:03 ` Joel Brobecker @ 2013-09-06 13:27 ` Yao Qi 2013-09-06 13:32 ` Eli Zaretskii 1 sibling, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-06 13:27 UTC (permalink / raw) To: Joel Brobecker; +Cc: Eli Zaretskii, gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 13:03 ` Joel Brobecker 2013-09-06 13:27 ` Yao Qi @ 2013-09-06 13:32 ` Eli Zaretskii 2013-09-06 14:17 ` Pierre Muller ` (2 more replies) 1 sibling, 3 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 13:32 UTC (permalink / raw) To: Joel Brobecker; +Cc: yao, gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* RE: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 13:32 ` Eli Zaretskii @ 2013-09-06 14:17 ` Pierre Muller [not found] ` <"000d01ceab0b$d53ae600$7fb0b200$@muller"@ics-cnrs.unistra.fr> 2013-09-06 14:52 ` Joel Brobecker 2 siblings, 0 replies; 63+ messages in thread From: Pierre Muller @ 2013-09-06 14:17 UTC (permalink / raw) To: gdb-patches > -----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 ^ permalink raw reply [flat|nested] 63+ messages in thread
[parent not found: <"000d01ceab0b$d53ae600$7fb0b200$@muller"@ics-cnrs.unistra.fr>]
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection [not found] ` <"000d01ceab0b$d53ae600$7fb0b200$@muller"@ics-cnrs.unistra.fr> @ 2013-09-06 14:38 ` Eli Zaretskii 0 siblings, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 14:38 UTC (permalink / raw) To: Pierre Muller; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 13:32 ` Eli Zaretskii 2013-09-06 14:17 ` Pierre Muller [not found] ` <"000d01ceab0b$d53ae600$7fb0b200$@muller"@ics-cnrs.unistra.fr> @ 2013-09-06 14:52 ` Joel Brobecker 2013-09-06 15:56 ` Eli Zaretskii 2 siblings, 1 reply; 63+ messages in thread From: Joel Brobecker @ 2013-09-06 14:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: yao, gdb-patches > > 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 14:52 ` Joel Brobecker @ 2013-09-06 15:56 ` Eli Zaretskii 2013-09-06 18:10 ` Joel Brobecker 0 siblings, 1 reply; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 15:56 UTC (permalink / raw) To: Joel Brobecker; +Cc: yao, gdb-patches > 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? ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 15:56 ` Eli Zaretskii @ 2013-09-06 18:10 ` Joel Brobecker 2013-09-06 18:36 ` Eli Zaretskii 0 siblings, 1 reply; 63+ messages in thread From: Joel Brobecker @ 2013-09-06 18:10 UTC (permalink / raw) To: Eli Zaretskii; +Cc: yao, gdb-patches > > 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 18:10 ` Joel Brobecker @ 2013-09-06 18:36 ` Eli Zaretskii 0 siblings, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-06 18:36 UTC (permalink / raw) To: Joel Brobecker; +Cc: yao, gdb-patches > 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? ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/3] Trust readonly sections if target has memory protection 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi ` (3 preceding siblings ...) 2013-09-06 5:57 ` [PATCH 0/3] Trust readonly sections if target " Eli Zaretskii @ 2013-09-06 13:00 ` Joel Brobecker 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi 5 siblings, 0 replies; 63+ messages in thread From: Joel Brobecker @ 2013-09-06 13:00 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi ` (4 preceding siblings ...) 2013-09-06 13:00 ` Joel Brobecker @ 2013-09-08 12:04 ` Yao Qi 2013-09-08 12:04 ` [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true Yao Qi ` (8 more replies) 5 siblings, 9 replies; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:04 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi @ 2013-09-08 12:04 ` Yao Qi 2013-09-08 15:10 ` Eli Zaretskii 2013-09-08 12:05 ` [PATCH 7/7] Windows has memory protection Yao Qi ` (7 subsequent siblings) 8 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:04 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true 2013-09-08 12:04 ` [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true Yao Qi @ 2013-09-08 15:10 ` Eli Zaretskii 0 siblings, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-08 15:10 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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? ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 7/7] Windows has memory protection 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi 2013-09-08 12:04 ` [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true Yao Qi @ 2013-09-08 12:05 ` Yao Qi 2013-09-08 12:05 ` [PATCH 2/7] set trust-readonly-sections off in test cases Yao Qi ` (6 subsequent siblings) 8 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:05 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 2/7] set trust-readonly-sections off in test cases 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi 2013-09-08 12:04 ` [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true Yao Qi 2013-09-08 12:05 ` [PATCH 7/7] Windows has memory protection Yao Qi @ 2013-09-08 12:05 ` Yao Qi 2013-09-08 12:05 ` [PATCH 4/7] Trust readonly sections if target has memory protection Yao Qi ` (5 subsequent siblings) 8 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:05 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 4/7] Trust readonly sections if target has memory protection 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi ` (2 preceding siblings ...) 2013-09-08 12:05 ` [PATCH 2/7] set trust-readonly-sections off in test cases Yao Qi @ 2013-09-08 12:05 ` Yao Qi 2013-09-08 15:13 ` Eli Zaretskii 2013-09-08 12:05 ` [PATCH 5/7] DOC and NEWS Yao Qi ` (4 subsequent siblings) 8 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:05 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 4/7] Trust readonly sections if target has memory protection 2013-09-08 12:05 ` [PATCH 4/7] Trust readonly sections if target has memory protection Yao Qi @ 2013-09-08 15:13 ` Eli Zaretskii 2013-09-09 7:49 ` Yao Qi 0 siblings, 1 reply; 63+ messages in thread From: Eli Zaretskii @ 2013-09-08 15:13 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 4/7] Trust readonly sections if target has memory protection 2013-09-08 15:13 ` Eli Zaretskii @ 2013-09-09 7:49 ` Yao Qi 2013-09-09 16:25 ` Eli Zaretskii 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-09 7:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 4/7] Trust readonly sections if target has memory protection 2013-09-09 7:49 ` Yao Qi @ 2013-09-09 16:25 ` Eli Zaretskii 0 siblings, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-09 16:25 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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? ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 5/7] DOC and NEWS 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi ` (3 preceding siblings ...) 2013-09-08 12:05 ` [PATCH 4/7] Trust readonly sections if target has memory protection Yao Qi @ 2013-09-08 12:05 ` Yao Qi 2013-09-08 12:05 ` [PATCH 3/7] New function windows_init_abi Yao Qi ` (3 subsequent siblings) 8 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:05 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 3/7] New function windows_init_abi 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi ` (4 preceding siblings ...) 2013-09-08 12:05 ` [PATCH 5/7] DOC and NEWS Yao Qi @ 2013-09-08 12:05 ` Yao Qi 2013-09-08 12:05 ` [PATCH 6/7] Linux has memory protection Yao Qi ` (2 subsequent siblings) 8 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:05 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 6/7] Linux has memory protection. 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi ` (5 preceding siblings ...) 2013-09-08 12:05 ` [PATCH 3/7] New function windows_init_abi Yao Qi @ 2013-09-08 12:05 ` Yao Qi 2013-09-09 19:16 ` [PATCH 0/7 V2] Trust readonly sections if target " Mark Kettenis 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi 8 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-08 12:05 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi ` (6 preceding siblings ...) 2013-09-08 12:05 ` [PATCH 6/7] Linux has memory protection Yao Qi @ 2013-09-09 19:16 ` Mark Kettenis 2013-09-10 4:06 ` Yao Qi 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi 8 siblings, 1 reply; 63+ messages in thread From: Mark Kettenis @ 2013-09-09 19:16 UTC (permalink / raw) To: yao; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-09 19:16 ` [PATCH 0/7 V2] Trust readonly sections if target " Mark Kettenis @ 2013-09-10 4:06 ` Yao Qi 2013-09-12 8:30 ` Yao Qi 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-10 4:06 UTC (permalink / raw) To: Mark Kettenis; +Cc: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-10 4:06 ` Yao Qi @ 2013-09-12 8:30 ` Yao Qi 2013-09-12 9:49 ` Mark Kettenis 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-12 8:30 UTC (permalink / raw) To: Mark Kettenis; +Cc: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-12 8:30 ` Yao Qi @ 2013-09-12 9:49 ` Mark Kettenis 2013-09-13 8:17 ` Yao Qi 0 siblings, 1 reply; 63+ messages in thread From: Mark Kettenis @ 2013-09-12 9:49 UTC (permalink / raw) To: yao; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-12 9:49 ` Mark Kettenis @ 2013-09-13 8:17 ` Yao Qi 2013-09-30 17:50 ` Pedro Alves 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-13 8:17 UTC (permalink / raw) To: Mark Kettenis; +Cc: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-13 8:17 ` Yao Qi @ 2013-09-30 17:50 ` Pedro Alves 2013-09-30 18:08 ` Pedro Alves ` (2 more replies) 0 siblings, 3 replies; 63+ messages in thread From: Pedro Alves @ 2013-09-30 17:50 UTC (permalink / raw) To: Yao Qi; +Cc: Mark Kettenis, gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-30 17:50 ` Pedro Alves @ 2013-09-30 18:08 ` Pedro Alves 2013-10-07 22:29 ` Stan Shebs 2013-10-09 2:24 ` Doug Evans 2013-10-15 0:44 ` Yao Qi 2 siblings, 1 reply; 63+ messages in thread From: Pedro Alves @ 2013-09-30 18:08 UTC (permalink / raw) Cc: Yao Qi, Mark Kettenis, gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-30 18:08 ` Pedro Alves @ 2013-10-07 22:29 ` Stan Shebs 2013-10-08 12:18 ` Pedro Alves 0 siblings, 1 reply; 63+ messages in thread From: Stan Shebs @ 2013-10-07 22:29 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-10-07 22:29 ` Stan Shebs @ 2013-10-08 12:18 ` Pedro Alves 2013-10-08 12:47 ` Abid, Hafiz 0 siblings, 1 reply; 63+ messages in thread From: Pedro Alves @ 2013-10-08 12:18 UTC (permalink / raw) To: Stan Shebs; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* RE: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-10-08 12:18 ` Pedro Alves @ 2013-10-08 12:47 ` Abid, Hafiz 2013-10-08 13:34 ` Pedro Alves 2013-10-08 13:36 ` tmirza 0 siblings, 2 replies; 63+ messages in thread From: Abid, Hafiz @ 2013-10-08 12:47 UTC (permalink / raw) To: Pedro Alves, Stan Shebs, Mirza, Taimoor; +Cc: gdb-patches > 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-10-08 12:47 ` Abid, Hafiz @ 2013-10-08 13:34 ` Pedro Alves 2013-10-08 13:36 ` tmirza 1 sibling, 0 replies; 63+ messages in thread From: Pedro Alves @ 2013-10-08 13:34 UTC (permalink / raw) To: Abid, Hafiz; +Cc: Stan Shebs, Mirza, Taimoor, gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-10-08 12:47 ` Abid, Hafiz 2013-10-08 13:34 ` Pedro Alves @ 2013-10-08 13:36 ` tmirza 1 sibling, 0 replies; 63+ messages in thread From: tmirza @ 2013-10-08 13:36 UTC (permalink / raw) To: Abid, Hafiz; +Cc: Pedro Alves, Stan Shebs, gdb-patches 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-30 17:50 ` Pedro Alves 2013-09-30 18:08 ` Pedro Alves @ 2013-10-09 2:24 ` Doug Evans 2013-10-23 10:16 ` Yao Qi 2013-10-15 0:44 ` Yao Qi 2 siblings, 1 reply; 63+ messages in thread From: Doug Evans @ 2013-10-09 2:24 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, Mark Kettenis, gdb-patches 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-10-09 2:24 ` Doug Evans @ 2013-10-23 10:16 ` Yao Qi 0 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-10-23 10:16 UTC (permalink / raw) To: Doug Evans; +Cc: Pedro Alves, Mark Kettenis, gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection 2013-09-30 17:50 ` Pedro Alves 2013-09-30 18:08 ` Pedro Alves 2013-10-09 2:24 ` Doug Evans @ 2013-10-15 0:44 ` Yao Qi 2 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-10-15 0:44 UTC (permalink / raw) To: Pedro Alves; +Cc: Mark Kettenis, gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 0/7 V3] Trust readonly sections if target has memory protection 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi ` (7 preceding siblings ...) 2013-09-09 19:16 ` [PATCH 0/7 V2] Trust readonly sections if target " Mark Kettenis @ 2013-09-20 2:47 ` Yao Qi 2013-09-20 2:47 ` [PATCH 4/7] Trust readonly sections if target has memory protection and in remote debugging Yao Qi ` (7 more replies) 8 siblings, 8 replies; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 4/7] Trust readonly sections if target has memory protection and in remote debugging 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-20 2:47 ` [PATCH 5/7] DOC and NEWS Yao Qi ` (6 subsequent siblings) 7 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 5/7] DOC and NEWS 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi 2013-09-20 2:47 ` [PATCH 4/7] Trust readonly sections if target has memory protection and in remote debugging Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-20 8:21 ` Eli Zaretskii 2013-09-20 2:47 ` [PATCH 1/7] Emit a query when writing to a readonly section and trust_readonly is true Yao Qi ` (5 subsequent siblings) 7 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches -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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 5/7] DOC and NEWS 2013-09-20 2:47 ` [PATCH 5/7] DOC and NEWS Yao Qi @ 2013-09-20 8:21 ` Eli Zaretskii 0 siblings, 0 replies; 63+ messages in thread From: Eli Zaretskii @ 2013-09-20 8:21 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/7] Emit a query when writing to a readonly section and trust_readonly is true 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi 2013-09-20 2:47 ` [PATCH 4/7] Trust readonly sections if target has memory protection and in remote debugging Yao Qi 2013-09-20 2:47 ` [PATCH 5/7] DOC and NEWS Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-20 2:47 ` [PATCH 2/7] set trust-readonly-sections off in test cases Yao Qi ` (4 subsequent siblings) 7 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 2/7] set trust-readonly-sections off in test cases 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi ` (2 preceding siblings ...) 2013-09-20 2:47 ` [PATCH 1/7] Emit a query when writing to a readonly section and trust_readonly is true Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-20 2:47 ` [PATCH 3/7] New function windows_init_abi Yao Qi ` (3 subsequent siblings) 7 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 3/7] New function windows_init_abi 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi ` (3 preceding siblings ...) 2013-09-20 2:47 ` [PATCH 2/7] set trust-readonly-sections off in test cases Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-30 18:23 ` Pedro Alves 2013-09-20 2:47 ` [PATCH 7/7] Windows has memory protection Yao Qi ` (2 subsequent siblings) 7 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 3/7] New function windows_init_abi 2013-09-20 2:47 ` [PATCH 3/7] New function windows_init_abi Yao Qi @ 2013-09-30 18:23 ` Pedro Alves 2013-10-01 6:47 ` Yao Qi 0 siblings, 1 reply; 63+ messages in thread From: Pedro Alves @ 2013-09-30 18:23 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 3/7] New function windows_init_abi 2013-09-30 18:23 ` Pedro Alves @ 2013-10-01 6:47 ` Yao Qi 2013-10-01 9:35 ` Pedro Alves 0 siblings, 1 reply; 63+ messages in thread From: Yao Qi @ 2013-10-01 6:47 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 3/7] New function windows_init_abi 2013-10-01 6:47 ` Yao Qi @ 2013-10-01 9:35 ` Pedro Alves 2013-10-01 13:23 ` Yao Qi 0 siblings, 1 reply; 63+ messages in thread From: Pedro Alves @ 2013-10-01 9:35 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 3/7] New function windows_init_abi 2013-10-01 9:35 ` Pedro Alves @ 2013-10-01 13:23 ` Yao Qi 0 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-10-01 13:23 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 7/7] Windows has memory protection 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi ` (4 preceding siblings ...) 2013-09-20 2:47 ` [PATCH 3/7] New function windows_init_abi Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-20 2:47 ` [PATCH 6/7] Linux " Yao Qi 2013-09-29 13:51 ` [PATCH 0/7 V3] Trust readonly sections if target " Yao Qi 7 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 6/7] Linux has memory protection. 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi ` (5 preceding siblings ...) 2013-09-20 2:47 ` [PATCH 7/7] Windows has memory protection Yao Qi @ 2013-09-20 2:47 ` Yao Qi 2013-09-29 13:51 ` [PATCH 0/7 V3] Trust readonly sections if target " Yao Qi 7 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-20 2:47 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/7 V3] Trust readonly sections if target has memory protection 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi ` (6 preceding siblings ...) 2013-09-20 2:47 ` [PATCH 6/7] Linux " Yao Qi @ 2013-09-29 13:51 ` Yao Qi 7 siblings, 0 replies; 63+ messages in thread From: Yao Qi @ 2013-09-29 13:51 UTC (permalink / raw) To: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 63+ messages in thread
end of thread, other threads:[~2013-10-23 10:16 UTC | newest] Thread overview: 63+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-09-06 2:03 [PATCH 0/3] Trust readonly sections if target has memory protection Yao Qi 2013-09-06 2:03 ` [PATCH 1/3] set trust-readonly-sections off in test cases Yao Qi 2013-09-06 5:56 ` Eli Zaretskii 2013-09-06 17:23 ` Pedro Alves 2013-09-06 2:03 ` [PATCH 2/3] Trust readonly sections if target has memory protection Yao Qi 2013-09-06 6:05 ` Eli Zaretskii 2013-09-06 9:07 ` Yao Qi 2013-09-06 9:24 ` Eli Zaretskii 2013-09-06 2:03 ` [PATCH 3/3] Linux " Yao Qi 2013-09-06 5:57 ` [PATCH 0/3] Trust readonly sections if target " Eli Zaretskii 2013-09-06 8:24 ` Yao Qi 2013-09-06 8:45 ` Eli Zaretskii 2013-09-06 13:03 ` Joel Brobecker 2013-09-06 13:27 ` Yao Qi 2013-09-06 13:32 ` Eli Zaretskii 2013-09-06 14:17 ` Pierre Muller [not found] ` <"000d01ceab0b$d53ae600$7fb0b200$@muller"@ics-cnrs.unistra.fr> 2013-09-06 14:38 ` Eli Zaretskii 2013-09-06 14:52 ` Joel Brobecker 2013-09-06 15:56 ` Eli Zaretskii 2013-09-06 18:10 ` Joel Brobecker 2013-09-06 18:36 ` Eli Zaretskii 2013-09-06 13:00 ` Joel Brobecker 2013-09-08 12:04 ` [PATCH 0/7 V2] " Yao Qi 2013-09-08 12:04 ` [PATCH 1/7] Emit a warning when writing to a readonly section and trust_readonly is true Yao Qi 2013-09-08 15:10 ` Eli Zaretskii 2013-09-08 12:05 ` [PATCH 7/7] Windows has memory protection Yao Qi 2013-09-08 12:05 ` [PATCH 2/7] set trust-readonly-sections off in test cases Yao Qi 2013-09-08 12:05 ` [PATCH 4/7] Trust readonly sections if target has memory protection Yao Qi 2013-09-08 15:13 ` Eli Zaretskii 2013-09-09 7:49 ` Yao Qi 2013-09-09 16:25 ` Eli Zaretskii 2013-09-08 12:05 ` [PATCH 5/7] DOC and NEWS Yao Qi 2013-09-08 12:05 ` [PATCH 3/7] New function windows_init_abi Yao Qi 2013-09-08 12:05 ` [PATCH 6/7] Linux has memory protection Yao Qi 2013-09-09 19:16 ` [PATCH 0/7 V2] Trust readonly sections if target " Mark Kettenis 2013-09-10 4:06 ` Yao Qi 2013-09-12 8:30 ` Yao Qi 2013-09-12 9:49 ` Mark Kettenis 2013-09-13 8:17 ` Yao Qi 2013-09-30 17:50 ` Pedro Alves 2013-09-30 18:08 ` Pedro Alves 2013-10-07 22:29 ` Stan Shebs 2013-10-08 12:18 ` Pedro Alves 2013-10-08 12:47 ` Abid, Hafiz 2013-10-08 13:34 ` Pedro Alves 2013-10-08 13:36 ` tmirza 2013-10-09 2:24 ` Doug Evans 2013-10-23 10:16 ` Yao Qi 2013-10-15 0:44 ` Yao Qi 2013-09-20 2:47 ` [PATCH 0/7 V3] " Yao Qi 2013-09-20 2:47 ` [PATCH 4/7] Trust readonly sections if target has memory protection and in remote debugging Yao Qi 2013-09-20 2:47 ` [PATCH 5/7] DOC and NEWS Yao Qi 2013-09-20 8:21 ` Eli Zaretskii 2013-09-20 2:47 ` [PATCH 1/7] Emit a query when writing to a readonly section and trust_readonly is true Yao Qi 2013-09-20 2:47 ` [PATCH 2/7] set trust-readonly-sections off in test cases Yao Qi 2013-09-20 2:47 ` [PATCH 3/7] New function windows_init_abi Yao Qi 2013-09-30 18:23 ` Pedro Alves 2013-10-01 6:47 ` Yao Qi 2013-10-01 9:35 ` Pedro Alves 2013-10-01 13:23 ` Yao Qi 2013-09-20 2:47 ` [PATCH 7/7] Windows has memory protection Yao Qi 2013-09-20 2:47 ` [PATCH 6/7] Linux " Yao Qi 2013-09-29 13:51 ` [PATCH 0/7 V3] Trust readonly sections if target " Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).