[rfc] btrace: control memory access during replay

[rfc] btrace: control memory access during replay

[Markus Metzger]

The btrace record target does not trace data.  We therefore do not allow
accessing read-write memory during replay.

In some cases, this might be useful to advanced users, though, who we assume
to know what they are doing.

Add a set|show command pair to turn this memory access restriction off.

CC: Eli Zaretskii <eliz@gnu.org>

2014-04-04  Markus Metzger  <markus.t.metzger@intel.com>

	* record-btrace.c (set_record_btrace_cmdlist)
	(show_record_btrace_cmdlist, cmd_set_record_btrace)
	(cmd_show_record_btrace): New.
	(_initialize_record_btrace): Add commands.
	* NEWS: Announce it.

testsuite/
	* gdb.btrace/data.exp: Test it.

doc/
	* gdb.texinfo: Document it.


---
 gdb/NEWS                          |  4 ++++
 gdb/doc/gdb.texinfo               | 13 ++++++++++++
 gdb/record-btrace.c               | 43 +++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.btrace/data.exp |  8 ++++++++
 4 files changed, 68 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index df233fc..dbd6889 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -50,6 +50,10 @@ maint ada show ignore-descriptive-types
   the user manual for more details on descriptive types and the intended
   usage of this option.
 
+set record btrace allow-memory-access (on|off)
+show record btrace allow-memory-access
+  Control access to read-write memory during replay.
+
 * New features in the GDB remote stub, GDBserver
 
   ** New option --debug-format=option1[,option2,...] allows one to add
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b218769..08ebb74 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6398,6 +6398,19 @@ results.
 @item show record full memory-query
 Show the current setting of @code{memory-query}.
 
+@kindex set record btrace
+@item set record btrace allow-memory-access
+Control the behavior of the @code{btrace} recording method when
+accessing memory during replay.  If ON, @value{GDBN} will allow
+arbitrary memory accesses.  The accessed memory corresponds to the end
+of the recorded execution trace.  It does not necessarily correspond
+to the current replay position.  If OFF (the default), @value{GDBN}
+will only allow accesses to read-only memory.
+
+@kindex show record btrace
+@item show record btrace allow-memory-access
+Show the current setting of @code{allow-memory-access}.
+
 @kindex info record
 @item info record
 Show various statistics about the recording depending on the recording
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index bcac165..9cf1ff8 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -45,6 +45,10 @@ static struct observer *record_btrace_thread_observer;
 /* Temporarily allow memory accesses.  */
 static int record_btrace_allow_memory_access;
 
+/* Command lists for "set/show record btrace".  */
+static struct cmd_list_element *set_record_btrace_cmdlist;
+static struct cmd_list_element *show_record_btrace_cmdlist;
+
 /* Print a record-btrace debug message.  Use do ... while (0) to avoid
    ambiguities when used in if statements.  */
 
@@ -1934,6 +1938,22 @@ cmd_record_btrace_start (char *args, int from_tty)
   execute_command ("target record-btrace", from_tty);
 }
 
+/* The "set record btrace" command.  */
+
+static void
+cmd_set_record_btrace (char *args, int from_tty)
+{
+  cmd_show_list (set_record_btrace_cmdlist, from_tty, "");
+}
+
+/* The "show record btrace" command.  */
+
+static void
+cmd_show_record_btrace (char *args, int from_tty)
+{
+  cmd_show_list (show_record_btrace_cmdlist, from_tty, "");
+}
+
 void _initialize_record_btrace (void);
 
 /* Initialize btrace commands.  */
@@ -1946,6 +1966,29 @@ _initialize_record_btrace (void)
 	   &record_cmdlist);
   add_alias_cmd ("b", "btrace", class_obscure, 1, &record_cmdlist);
 
+  add_prefix_cmd ("btrace", class_support, cmd_set_record_btrace,
+		  _("Set record options"), &set_record_btrace_cmdlist,
+		  "set record btrace ", 0, &set_record_cmdlist);
+
+  add_prefix_cmd ("btrace", class_support, cmd_show_record_btrace,
+		  _("Show record options"), &show_record_btrace_cmdlist,
+		  "show record btrace ", 0, &show_record_cmdlist);
+
+  /* Record instructions number limit command.  */
+  add_setshow_boolean_cmd ("allow-memory-access", no_class,
+			   &record_btrace_allow_memory_access, _("\
+Set whether memory accesses are allowed during replay."), _("\
+Show whether memory accesses are allowed during replay."),
+			   _("Default is OFF.\n\n\
+The btrace record target does not trace data.\n\
+The memory therefore corresponds to the end of the trace and not \
+to the current replay position.\n\n\
+When ON, allow accesses to arbitrary memory during replay.\n\
+When OFF, only allow accesses to read-only memory during replay."),
+			   NULL, NULL,
+			   &set_record_btrace_cmdlist,
+			   &show_record_btrace_cmdlist);
+
   init_record_btrace_ops ();
   add_target (&record_btrace_ops);
 
diff --git a/gdb/testsuite/gdb.btrace/data.exp b/gdb/testsuite/gdb.btrace/data.exp
index 64c5443..cfb1395 100644
--- a/gdb/testsuite/gdb.btrace/data.exp
+++ b/gdb/testsuite/gdb.btrace/data.exp
@@ -40,6 +40,14 @@ gdb_test "reverse-step" ".*test\.4.*"
 gdb_test "print glob" "unavailable\[^\\\r\\\n\]*"
 gdb_test "print loc" "unavailable\[^\\\r\\\n\]*"
 
+# we can read memory if we explicitly allow it.
+gdb_test_no_output "set record btrace allow-memory-access on"
+gdb_test "print glob" "1"
+
+# we can't if we don't explicitly allow it.
+gdb_test_no_output "set record btrace allow-memory-access off"
+gdb_test "print glob" "unavailable\[^\\\r\\\n\]*"
+
 # stop replaying and try again
 gdb_test "record goto end"
 gdb_test "print glob" "1"
-- 
1.8.3.1

Re: [rfc] btrace: control memory access during replay

[Eli Zaretskii]

> From: Markus Metzger <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org, Eli Zaretskii <eliz@gnu.org>
> Date: Fri,  4 Apr 2014 10:56:21 +0200
> 
> The btrace record target does not trace data.  We therefore do not allow
> accessing read-write memory during replay.
> 
> In some cases, this might be useful to advanced users, though, who we assume
> to know what they are doing.
> 
> Add a set|show command pair to turn this memory access restriction off.
> 
> CC: Eli Zaretskii <eliz@gnu.org>
> 
> 2014-04-04  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* record-btrace.c (set_record_btrace_cmdlist)
> 	(show_record_btrace_cmdlist, cmd_set_record_btrace)
> 	(cmd_show_record_btrace): New.
> 	(_initialize_record_btrace): Add commands.
> 	* NEWS: Announce it.
> 
> testsuite/
> 	* gdb.btrace/data.exp: Test it.
> 
> doc/
> 	* gdb.texinfo: Document it.

The ChangeLog entry should state the name(s) of the node(s) in which
you make changes.

Other than that, the documentation parts are approved.  However, I
wonder whether "allow-memory-access" is a good name for a setting
which actually allows access to writable portion of the memory.  IOW,
even when the value is OFF, we do allow access to memory, just not the
writable portion of it.

Thanks.

RE: [rfc] btrace: control memory access during replay

[Metzger, Markus T]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Friday, April 04, 2014 11:17 AM


> The ChangeLog entry should state the name(s) of the node(s) in which
> you make changes.

Thanks, will add it.


> Other than that, the documentation parts are approved.  However, I
> wonder whether "allow-memory-access" is a good name for a setting
> which actually allows access to writable portion of the memory.  IOW,
> even when the value is OFF, we do allow access to memory, just not the
> writable portion of it.

Agreed; allow-access-to-writable-memory-while-replaying is a bit long, though.

Let's hear what others say about the feature in general.  I remember that
Jan requested something like this.  If the feature itself is approved, I'll
try to find a better name.

regards,
markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [rfc] btrace: control memory access during replay

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "palves@redhat.com" <palves@redhat.com>, "jan.kratochvil@redhat.com"
> 	<jan.kratochvil@redhat.com>, "gdb-patches@sourceware.org"
> 	<gdb-patches@sourceware.org>
> Date: Fri, 4 Apr 2014 09:42:46 +0000
> 
> > Other than that, the documentation parts are approved.  However, I
> > wonder whether "allow-memory-access" is a good name for a setting
> > which actually allows access to writable portion of the memory.  IOW,
> > even when the value is OFF, we do allow access to memory, just not the
> > writable portion of it.
> 
> Agreed; allow-access-to-writable-memory-while-replaying is a bit long, though.

How about access-writable-memory?

Re: [rfc] btrace: control memory access during replay

[Pedro Alves]

On 04/04/2014 10:48 AM, Eli Zaretskii wrote:
>> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
>> CC: "palves@redhat.com" <palves@redhat.com>, "jan.kratochvil@redhat.com"
>> 	<jan.kratochvil@redhat.com>, "gdb-patches@sourceware.org"
>> 	<gdb-patches@sourceware.org>
>> Date: Fri, 4 Apr 2014 09:42:46 +0000
>>
>>> Other than that, the documentation parts are approved.  However, I
>>> wonder whether "allow-memory-access" is a good name for a setting
>>> which actually allows access to writable portion of the memory.  IOW,
>>> even when the value is OFF, we do allow access to memory, just not the
>>> writable portion of it.
>>
>> Agreed; allow-access-to-writable-memory-while-replaying is a bit long, though.
> 
> How about access-writable-memory?

Sounds fine to me.

What's the likelihood of another variant appearing?  That is,
I'm mildly wondering if it should be an enum from the get go:

 set record btrace replay-memory-access read-only|read-write|...|...

I also got a little confused with:

"The accessed memory corresponds to the end of the recorded
execution trace."

Maybe we should say "live program" instead ?

Also, I think it'd be good to add an into to the manual explaining
the use case.  Something like:

The btrace record target does not trace data.  As a convenience,
when replaying, GDB reads read-only memory off the live program
directly, assuming that the addresses of the read-only areas
don't change.  This for example makes it possible to disassemble
code while replaying, but not to print variables.
In some cases, being able to inspect variables might be useful.
You can use the following command for that:

and then followed by the docu for the command like you had:

+@kindex set record btrace
+@item set record btrace allow-memory-access
+Control the behavior of the @code{btrace} recording method when
+accessing memory during replay.  If ON, @value{GDBN} will allow
+arbitrary memory accesses.  The accessed memory corresponds to the end
+of the recorded execution trace.  It does not necessarily correspond
+to the current replay position.  If OFF (the default), @value{GDBN}
+will only allow accesses to read-only memory.
...

I actually didn't see anything in the patch that actually makes the
setting work.

Also, please install a show hook in the command, so that i18n
can work.

Thanks,
-- 
Pedro Alves

RE: [rfc] btrace: control memory access during replay

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Wednesday, May 14, 2014 5:35 PM


> On 04/04/2014 10:48 AM, Eli Zaretskii wrote:

> >>> Other than that, the documentation parts are approved.  However, I
> >>> wonder whether "allow-memory-access" is a good name for a setting
> >>> which actually allows access to writable portion of the memory.  IOW,
> >>> even when the value is OFF, we do allow access to memory, just not the
> >>> writable portion of it.
> >>
> >> Agreed; allow-access-to-writable-memory-while-replaying is a bit long,
> though.
> >
> > How about access-writable-memory?
> 
> Sounds fine to me.
> 
> What's the likelihood of another variant appearing?  That is,
> I'm mildly wondering if it should be an enum from the get go:
> 
>  set record btrace replay-memory-access read-only|read-write|...|...

I don't see another variant right now but I also don't see why it
shouldn't be an enum.


> I also got a little confused with:
> 
> "The accessed memory corresponds to the end of the recorded
> execution trace."
> 
> Maybe we should say "live program" instead ?

Would "live program" still be OK for core files?


> Also, I think it'd be good to add an into to the manual explaining
> the use case.  Something like:
> 
> The btrace record target does not trace data.  As a convenience,
> when replaying, GDB reads read-only memory off the live program
> directly, assuming that the addresses of the read-only areas
> don't change.  This for example makes it possible to disassemble
> code while replaying, but not to print variables.
> In some cases, being able to inspect variables might be useful.
> You can use the following command for that:

Will do.


> I actually didn't see anything in the patch that actually makes the
> setting work.

The patch is using an existing variable to guard writable memory
access.  We already allow write-access for breakpoints during
replay.  This patch is now adding a CLI for the guard variable.


> Also, please install a show hook in the command, so that i18n
> can work.

I'm using the default set/show functions with _("") descriptions
for both set and show.  Isn't that enough for i18n?

In case it isn't, would I need a set function, as well?

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [rfc] btrace: control memory access during replay

[Pedro Alves]

On 05/19/2014 08:51 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Wednesday, May 14, 2014 5:35 PM
> 
> 
>> On 04/04/2014 10:48 AM, Eli Zaretskii wrote:
> 
>>>>> Other than that, the documentation parts are approved.  However, I
>>>>> wonder whether "allow-memory-access" is a good name for a setting
>>>>> which actually allows access to writable portion of the memory.  IOW,
>>>>> even when the value is OFF, we do allow access to memory, just not the
>>>>> writable portion of it.
>>>>
>>>> Agreed; allow-access-to-writable-memory-while-replaying is a bit long,
>> though.
>>>
>>> How about access-writable-memory?
>>
>> Sounds fine to me.
>>
>> What's the likelihood of another variant appearing?  That is,
>> I'm mildly wondering if it should be an enum from the get go:
>>
>>  set record btrace replay-memory-access read-only|read-write|...|...
> 
> I don't see another variant right now but I also don't see why it
> shouldn't be an enum.

The kind of variant I was considering was disabling the fallback
of reading read only regions as tagged in the binary from live/core
memory.  But maybe btrace gets completely useless that way.
If we can't think of another useful variant, then I'm fine with
a boolean, if it sounds more natural.  Your choice.

>> I also got a little confused with:
>>
>> "The accessed memory corresponds to the end of the recorded
>> execution trace."
>>
>> Maybe we should say "live program" instead ?
> 
> Would "live program" still be OK for core files?

Hmm.  I didn't think btrace could work with core files,
unlike record full?  Are you adding support for dumping/restoring
the btrace like "record save" does?

>> I actually didn't see anything in the patch that actually makes the
>> setting work.
> 
> The patch is using an existing variable to guard writable memory
> access.  We already allow write-access for breakpoints during
> replay.  This patch is now adding a CLI for the guard variable.

Ah.  Thanks.

>> Also, please install a show hook in the command, so that i18n
>> can work.
> 
> I'm using the default set/show functions with _("") descriptions
> for both set and show.  Isn't that enough for i18n?

Nope.  See deprecated_show_value_hack:

  /* Print doc minus "show" at start.  */
  print_doc_line (gdb_stdout, c->doc + 5);

That can only work in English.

> 
> In case it isn't, would I need a set function, as well?

Nope, just the show function is enough.

-- 
Pedro Alves

RE: [rfc] btrace: control memory access during replay

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, May 19, 2014 7:44 PM
> To: Metzger, Markus T; Eli Zaretskii

> >> What's the likelihood of another variant appearing?  That is,
> >> I'm mildly wondering if it should be an enum from the get go:
> >>
> >>  set record btrace replay-memory-access read-only|read-write|...|...
> >
> > I don't see another variant right now but I also don't see why it
> > shouldn't be an enum.
> 
> The kind of variant I was considering was disabling the fallback
> of reading read only regions as tagged in the binary from live/core
> memory.  But maybe btrace gets completely useless that way.
> If we can't think of another useful variant, then I'm fine with
> a boolean, if it sounds more natural.  Your choice.

We must be able to read code sections.  Without that, btrace
will not work.

One more option that I can think of is reading from object
files.

I'll use an enum for the sake of flexibility.


> Hmm.  I didn't think btrace could work with core files,
> unlike record full?  Are you adding support for dumping/restoring
> the btrace like "record save" does?

At the moment it can't.  I plan to add core file support, though.

Regards,
Markus.

> >> On 04/04/2014 10:48 AM, Eli Zaretskii wrote:
> >
> >>>>> Other than that, the documentation parts are approved.  However, I
> >>>>> wonder whether "allow-memory-access" is a good name for a setting
> >>>>> which actually allows access to writable portion of the memory.  IOW,
> >>>>> even when the value is OFF, we do allow access to memory, just not
> the
> >>>>> writable portion of it.
> >>>>
> >>>> Agreed; allow-access-to-writable-memory-while-replaying is a bit long,
> >> though.
> >>>
> >>> How about access-writable-memory?
> >>
> >> Sounds fine to me.
> >>
> >> What's the likelihood of another variant appearing?  That is,
> >> I'm mildly wondering if it should be an enum from the get go:
> >>
> >>  set record btrace replay-memory-access read-only|read-write|...|...
> >
> > I don't see another variant right now but I also don't see why it
> > shouldn't be an enum.
> 
> The kind of variant I was considering was disabling the fallback
> of reading read only regions as tagged in the binary from live/core
> memory.  But maybe btrace gets completely useless that way.
> If we can't think of another useful variant, then I'm fine with
> a boolean, if it sounds more natural.  Your choice.
> 
> >> I also got a little confused with:
> >>
> >> "The accessed memory corresponds to the end of the recorded
> >> execution trace."
> >>
> >> Maybe we should say "live program" instead ?
> >
> > Would "live program" still be OK for core files?
> 
> Hmm.  I didn't think btrace could work with core files,
> unlike record full?  Are you adding support for dumping/restoring
> the btrace like "record save" does?
> 
> >> I actually didn't see anything in the patch that actually makes the
> >> setting work.
> >
> > The patch is using an existing variable to guard writable memory
> > access.  We already allow write-access for breakpoints during
> > replay.  This patch is now adding a CLI for the guard variable.
> 
> Ah.  Thanks.
> 
> >> Also, please install a show hook in the command, so that i18n
> >> can work.
> >
> > I'm using the default set/show functions with _("") descriptions
> > for both set and show.  Isn't that enough for i18n?
> 
> Nope.  See deprecated_show_value_hack:
> 
>   /* Print doc minus "show" at start.  */
>   print_doc_line (gdb_stdout, c->doc + 5);
> 
> That can only work in English.
> 
> >
> > In case it isn't, would I need a set function, as well?
> 
> Nope, just the show function is enough.
> 
> --
> Pedro Alves
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052