[PATCH v2 1/3] make_corefile_notes: have caller free returned memory

[PATCH v2 1/3] make_corefile_notes: have caller free returned memory

[Markus Metzger]

The various make_corefile_notes implementations for gdbarch as well as target
currently make an xfree cleanup on the data they return.  This causes problems
when trying to put a TRY_CATCH around the make_corefile_notes call.
Specifically, we get a stale cleanup error in restore_my_cleanups.

Omit the make_cleanup and have the caller free the memory.

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

	* fbsd-nat.c (fbsd_make_corefile_notes): Remove make_cleanup call.
	* gcore.c (write_gcore_file): Free memory returned from
	make_corefile_notes.
	* linux-tdep.c (linux_make_corefile_notes): Remove make_cleanup call.
	* procfs.c (procfs_make_note_section): Remove make_cleanup call.
---
 gdb/fbsd-nat.c   | 1 -
 gdb/gcore.c      | 5 +++++
 gdb/linux-tdep.c | 1 -
 gdb/procfs.c     | 1 -
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 9f30edf..4e115b2 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -214,6 +214,5 @@ fbsd_make_corefile_notes (struct target_ops *self, bfd *obfd, int *note_size)
 					  fname, psargs);
     }
 
-  make_cleanup (xfree, note_data);
   return note_data;
 }
diff --git a/gdb/gcore.c b/gdb/gcore.c
index e225080..7a4ded7 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -68,6 +68,7 @@ create_gcore_bfd (const char *filename)
 void
 write_gcore_file (bfd *obfd)
 {
+  struct cleanup *cleanup;
   void *note_data = NULL;
   int note_size = 0;
   asection *note_sec = NULL;
@@ -84,6 +85,8 @@ write_gcore_file (bfd *obfd)
   if (note_data == NULL || note_size == 0)
     error (_("Target does not support core file generation."));
 
+  cleanup = make_cleanup (xfree, note_data);
+
   /* Create the note section.  */
   note_sec = bfd_make_section_anyway_with_flags (obfd, "note0",
 						 SEC_HAS_CONTENTS
@@ -104,6 +107,8 @@ write_gcore_file (bfd *obfd)
   /* Write out the contents of the note section.  */
   if (!bfd_set_section_contents (obfd, note_sec, note_data, 0, note_size))
     warning (_("writing note section (%s)"), bfd_errmsg (bfd_get_error ()));
+
+  do_cleanups (cleanup);
 }
 
 static void
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index ca19cf4..d0f1106 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1502,7 +1502,6 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
   note_data = linux_make_mappings_corefile_notes (gdbarch, obfd,
 						  note_data, note_size);
 
-  make_cleanup (xfree, note_data);
   return note_data;
 }
 
diff --git a/gdb/procfs.c b/gdb/procfs.c
index cbb44ce..4caaf7b 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -5522,7 +5522,6 @@ procfs_make_note_section (struct target_ops *self, bfd *obfd, int *note_size)
       xfree (auxv);
     }
 
-  make_cleanup (xfree, note_data);
   return note_data;
 }
 #else /* !Solaris */
-- 
1.8.3.1

[PATCH v2 2/3] gcore, target: allow target to prepare/cleanup for/after core file generation

[Markus Metzger]

Add new target functions to_prepare_to_generate_core and
to_done_generating_core that are called before and after generating a core
file, respectively.

This allows targets to prepare for core file generation and to clean up
afterwards.

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

	* target.h (target_ops) <to_prepare_to_generate_core>
	<to_done_generating_core>: New.
	(target_prepare_to_generate_core, target_done_generating_core): New.
	* target.c (target_prepare_to_generate_core)
	(target_done_generating_core): New.
	* target-delegates.c: Regenerate.
	* gcore.c: (write_gcore_file): Rename to ...
	(write_gcore_file_1): ...this.
	(write_gcore_file): Call target_prepare_to_generate_core
	and target_done_generating_core.
---
 gdb/gcore.c            | 27 ++++++++++++++++++++++-----
 gdb/target-delegates.c | 30 ++++++++++++++++++++++++++++++
 gdb/target.c           | 16 ++++++++++++++++
 gdb/target.h           | 14 ++++++++++++++
 4 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/gdb/gcore.c b/gdb/gcore.c
index 7a4ded7..44acbe3 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -61,12 +61,10 @@ create_gcore_bfd (const char *filename)
   return obfd;
 }
 
-/* write_gcore_file -- helper for gcore_command (exported).
-   Compose and write the corefile data to the core file.  */
-
+/* write_gcore_file_1 -- do the actual work of write_gcore_file.  */
 
-void
-write_gcore_file (bfd *obfd)
+static void
+write_gcore_file_1 (bfd *obfd)
 {
   struct cleanup *cleanup;
   void *note_data = NULL;
@@ -111,6 +109,25 @@ write_gcore_file (bfd *obfd)
   do_cleanups (cleanup);
 }
 
+/* write_gcore_file -- helper for gcore_command (exported).
+   Compose and write the corefile data to the core file.  */
+
+void
+write_gcore_file (bfd *obfd)
+{
+  volatile struct gdb_exception except;
+
+  target_prepare_to_generate_core ();
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    write_gcore_file_1 (obfd);
+
+  target_done_generating_core ();
+
+  if (except.reason < 0)
+    throw_exception (except);
+}
+
 static void
 do_bfd_delete_cleanup (void *arg)
 {
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 4eefae8..eaab916 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -1625,6 +1625,30 @@ delegate_decr_pc_after_break (struct target_ops *self, struct gdbarch *arg1)
 }
 
 static void
+delegate_prepare_to_generate_core (struct target_ops *self)
+{
+  self = self->beneath;
+  self->to_prepare_to_generate_core (self);
+}
+
+static void
+tdefault_prepare_to_generate_core (struct target_ops *self)
+{
+}
+
+static void
+delegate_done_generating_core (struct target_ops *self)
+{
+  self = self->beneath;
+  self->to_done_generating_core (self);
+}
+
+static void
+tdefault_done_generating_core (struct target_ops *self)
+{
+}
+
+static void
 install_delegators (struct target_ops *ops)
 {
   if (ops->to_post_attach == NULL)
@@ -1897,6 +1921,10 @@ install_delegators (struct target_ops *ops)
     ops->to_get_tailcall_unwinder = delegate_get_tailcall_unwinder;
   if (ops->to_decr_pc_after_break == NULL)
     ops->to_decr_pc_after_break = delegate_decr_pc_after_break;
+  if (ops->to_prepare_to_generate_core == NULL)
+    ops->to_prepare_to_generate_core = delegate_prepare_to_generate_core;
+  if (ops->to_done_generating_core == NULL)
+    ops->to_done_generating_core = delegate_done_generating_core;
 }
 
 static void
@@ -2037,4 +2065,6 @@ install_dummy_methods (struct target_ops *ops)
   ops->to_get_unwinder = tdefault_get_unwinder;
   ops->to_get_tailcall_unwinder = tdefault_get_tailcall_unwinder;
   ops->to_decr_pc_after_break = default_target_decr_pc_after_break;
+  ops->to_prepare_to_generate_core = tdefault_prepare_to_generate_core;
+  ops->to_done_generating_core = tdefault_done_generating_core;
 }
diff --git a/gdb/target.c b/gdb/target.c
index 075425d..f384ed0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3606,6 +3606,22 @@ target_decr_pc_after_break (struct gdbarch *gdbarch)
   return current_target.to_decr_pc_after_break (&current_target, gdbarch);
 }
 
+/* See target.h.  */
+
+void
+target_prepare_to_generate_core (void)
+{
+  current_target.to_prepare_to_generate_core (&current_target);
+}
+
+/* See target.h.  */
+
+void
+target_done_generating_core (void)
+{
+  current_target.to_done_generating_core (&current_target);
+}
+
 static void
 debug_to_files_info (struct target_ops *target)
 {
diff --git a/gdb/target.h b/gdb/target.h
index e563f2f..96d5cb1 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1109,6 +1109,14 @@ struct target_ops
 					 struct gdbarch *gdbarch)
       TARGET_DEFAULT_FUNC (default_target_decr_pc_after_break);
 
+    /* Prepare to generate a core file.  */
+    void (*to_prepare_to_generate_core) (struct target_ops *)
+      TARGET_DEFAULT_IGNORE ();
+
+    /* Cleanup after generating a core file.  */
+    void (*to_done_generating_core) (struct target_ops *)
+      TARGET_DEFAULT_IGNORE ();
+
     int to_magic;
     /* Need sub-structure for target machine related rather than comm related?
      */
@@ -2261,4 +2269,10 @@ extern CORE_ADDR forward_target_decr_pc_after_break (struct target_ops *ops,
 /* See to_decr_pc_after_break.  */
 extern CORE_ADDR target_decr_pc_after_break (struct gdbarch *gdbarch);
 
+/* See to_prepare_to_generate_core.  */
+extern void target_prepare_to_generate_core (void);
+
+/* See to_done_generating_core.  */
+extern void target_done_generating_core (void);
+
 #endif /* !defined (TARGET_H) */
-- 
1.8.3.1

Re: [PATCH v2 2/3] gcore, target: allow target to prepare/cleanup for/after core file generation

[Pedro Alves]

On 06/24/2014 09:51 AM, Markus Metzger wrote:
> Add new target functions to_prepare_to_generate_core and
> to_done_generating_core that are called before and after generating a core
> file, respectively.
> 
> This allows targets to prepare for core file generation and to clean up
> afterwards.
> 
> 2014-06-24  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* target.h (target_ops) <to_prepare_to_generate_core>
> 	<to_done_generating_core>: New.
> 	(target_prepare_to_generate_core, target_done_generating_core): New.
> 	* target.c (target_prepare_to_generate_core)
> 	(target_done_generating_core): New.
> 	* target-delegates.c: Regenerate.
> 	* gcore.c: (write_gcore_file): Rename to ...
> 	(write_gcore_file_1): ...this.
> 	(write_gcore_file): Call target_prepare_to_generate_core
> 	and target_done_generating_core.

OK.

Thanks,
-- 
Pedro Alves

[PATCH v2 3/3] btrace: pretend we're not replaying when generating a core file

[Markus Metzger]

When generating a core file using the "generate-core-file" command while
replaying with the btrace record target, we won't be able to access all
registers and all memory.  This leads to an assertion.

Pretend that we are not replaying while generating a core file.  This will
forward fetch and store registers as well as xfer memory calls to the target
beneath.

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

	* record-btrace.c (record_btrace_generating_corefile)
	(record_btrace_prepare_to_generate_core)
	(record_btrace_done_generating_core): New.
	(record_btrace_xfer_partial, record_btrace_fetch_registers)
	(record_btrace_store_registers, record_btrace_prepare_to_store):
	Forward request when generating a core file.
	(init_record_btrace_ops): Set to_prepare_to_generate_core and
	to_done_generating_core.

testsuite/
	* gdb.btrace/gcore.exp: New.
---
 gdb/record-btrace.c                | 28 +++++++++++++++++++++++---
 gdb/testsuite/gdb.btrace/gcore.exp | 41 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/gcore.exp

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 6a9bfe1..a47aa95 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -68,6 +68,9 @@ static enum exec_direction_kind record_btrace_resume_exec_dir = EXEC_FORWARD;
 /* The async event handler for reverse/replay execution.  */
 static struct async_event_handler *record_btrace_async_inferior_event_handler;
 
+/* A flag indicating that we are currently generating a core file.  */
+static int record_btrace_generating_corefile;
+
 /* Print a record-btrace debug message.  Use do ... while (0) to avoid
    ambiguities when used in if statements.  */
 
@@ -854,6 +857,7 @@ record_btrace_xfer_partial (struct target_ops *ops, enum target_object object,
 
   /* Filter out requests that don't make sense during replay.  */
   if (replay_memory_access == replay_memory_access_read_only
+      && !record_btrace_generating_corefile
       && record_btrace_is_replaying (ops))
     {
       switch (object)
@@ -969,7 +973,7 @@ record_btrace_fetch_registers (struct target_ops *ops,
   gdb_assert (tp != NULL);
 
   replay = tp->btrace.replay;
-  if (replay != NULL)
+  if (replay != NULL && !record_btrace_generating_corefile)
     {
       const struct btrace_insn *insn;
       struct gdbarch *gdbarch;
@@ -1010,7 +1014,7 @@ record_btrace_store_registers (struct target_ops *ops,
 {
   struct target_ops *t;
 
-  if (record_btrace_is_replaying (ops))
+  if (!record_btrace_generating_corefile && record_btrace_is_replaying (ops))
     error (_("This record target does not allow writing registers."));
 
   gdb_assert (may_write_registers != 0);
@@ -1033,7 +1037,7 @@ record_btrace_prepare_to_store (struct target_ops *ops,
 {
   struct target_ops *t;
 
-  if (record_btrace_is_replaying (ops))
+  if (!record_btrace_generating_corefile && record_btrace_is_replaying (ops))
     return;
 
   for (t = ops->beneath; t != NULL; t = t->beneath)
@@ -1939,6 +1943,22 @@ record_btrace_execution_direction (struct target_ops *self)
   return record_btrace_resume_exec_dir;
 }
 
+/* The to_prepare_to_generate_core target method.  */
+
+static void
+record_btrace_prepare_to_generate_core (struct target_ops *self)
+{
+  record_btrace_generating_corefile = 1;
+}
+
+/* The to_done_generating_core target method.  */
+
+static void
+record_btrace_done_generating_core (struct target_ops *self)
+{
+  record_btrace_generating_corefile = 0;
+}
+
 /* Initialize the record-btrace target ops.  */
 
 static void
@@ -1983,6 +2003,8 @@ init_record_btrace_ops (void)
   ops->to_can_execute_reverse = record_btrace_can_execute_reverse;
   ops->to_decr_pc_after_break = record_btrace_decr_pc_after_break;
   ops->to_execution_direction = record_btrace_execution_direction;
+  ops->to_prepare_to_generate_core = record_btrace_prepare_to_generate_core;
+  ops->to_done_generating_core = record_btrace_done_generating_core;
   ops->to_stratum = record_stratum;
   ops->to_magic = OPS_MAGIC;
 }
diff --git a/gdb/testsuite/gdb.btrace/gcore.exp b/gdb/testsuite/gdb.btrace/gcore.exp
new file mode 100644
index 0000000..1ff4f27
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/gcore.exp
@@ -0,0 +1,41 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2014 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <markus.t.metzger@intel.com>
+#
+# 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/>.
+
+# check for btrace support
+if { [skip_btrace_tests] } { return -1 }
+
+# start inferior
+standard_testfile x86-record_goto.S
+if [prepare_for_testing gcore.exp $testfile $srcfile] {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# trace the call to the test function
+gdb_test_no_output "record btrace"
+gdb_test "next" ".*main\.3.*"
+
+# start replaying
+gdb_test "record goto begin" ".*main\.2.*"
+
+# generate a core file - this used to assert
+gdb_test "generate-core-file core" "Saved corefile core"
-- 
1.8.3.1

Re: [PATCH v2 3/3] btrace: pretend we're not replaying when generating a core file

[Pedro Alves]

Hi Markus,

On 06/24/2014 09:51 AM, Markus Metzger wrote:
> When generating a core file using the "generate-core-file" command while
> replaying with the btrace record target, we won't be able to access all
> registers and all memory.  This leads to an assertion.

Please paste the gdb output showing the assertion in question here in
the commit log for future reference.

> Pretend that we are not replaying while generating a core file.  This will
> forward fetch and store registers as well as xfer memory calls to the target
> beneath.



>  
> +/* The to_prepare_to_generate_core target method.  */
> +
> +static void
> +record_btrace_prepare_to_generate_core (struct target_ops *self)
> +{
> +  record_btrace_generating_corefile = 1;
> +}
> +
> +/* The to_done_generating_core target method.  */
> +
> +static void
> +record_btrace_done_generating_core (struct target_ops *self)
> +{
> +  record_btrace_generating_corefile = 0;

This flag should also be cleared in record_btrace_open,
in case the target was previously abruptly closed between
the record_btrace_prepare_to_generate_core
and record_btrace_done_generating_core calls.

OK with that change.

Thank you.

-- 
Pedro Alves

Re: [PATCH v2 1/3] make_corefile_notes: have caller free returned memory

[Pedro Alves]

On 06/24/2014 09:51 AM, Markus Metzger wrote:
> The various make_corefile_notes implementations for gdbarch as well as target
> currently make an xfree cleanup on the data they return.  This causes problems
> when trying to put a TRY_CATCH around the make_corefile_notes call.
> Specifically, we get a stale cleanup error in restore_my_cleanups.
> 
> Omit the make_cleanup and have the caller free the memory.
> 
> 2014-06-24  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* fbsd-nat.c (fbsd_make_corefile_notes): Remove make_cleanup call.
> 	* gcore.c (write_gcore_file): Free memory returned from
> 	make_corefile_notes.
> 	* linux-tdep.c (linux_make_corefile_notes): Remove make_cleanup call.
> 	* procfs.c (procfs_make_note_section): Remove make_cleanup call.

OK.

-- 
Pedro Alves

Re: [PATCH v2 1/3] make_corefile_notes: have caller free returned memory

[Tom Tromey]

>>>>> "Markus" == Markus Metzger <markus.t.metzger@intel.com> writes:

Markus>    if (note_data == NULL || note_size == 0)
Markus>      error (_("Target does not support core file generation."));
 
Markus> +  cleanup = make_cleanup (xfree, note_data);

I wonder if it is possible for note_data!=NULL but note_size==0.

If it is possible, then the cleanup ought to be moved earlier.
(FWIW it's ok to have an xfree cleanup with a NULL argument.)

Or if it is not possible, then I suppose the code just above is in
error... not your problem but if you happen to know, I would like to fix
it up.

thanks,
Tom

RE: [PATCH v2 1/3] make_corefile_notes: have caller free returned memory

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Tom Tromey
> Sent: Tuesday, June 24, 2014 3:45 PM
> To: Metzger, Markus T
> Cc: palves@redhat.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 1/3] make_corefile_notes: have caller free returned
> memory
> 
> >>>>> "Markus" == Markus Metzger <markus.t.metzger@intel.com> writes:
> 
> Markus>    if (note_data == NULL || note_size == 0)
> Markus>      error (_("Target does not support core file generation."));
> 
> Markus> +  cleanup = make_cleanup (xfree, note_data);
> 
> I wonder if it is possible for note_data!=NULL but note_size==0.

I don't know.  I have not found such a case, but that only means
that I have not looked hard enough.

I found a case where note_data == NULL but note_size could be > 0
in linux_spu_make_corefile_notes in linux-tdep.c, which is similarly
odd.

Other code in the same file is only checking note_data, not note_size.


> If it is possible, then the cleanup ought to be moved earlier.
> (FWIW it's ok to have an xfree cleanup with a NULL argument.)

I'll do that.  Thanks for pointing out this bug.


> Or if it is not possible, then I suppose the code just above is in
> error... not your problem but if you happen to know, I would like to fix
> it up.

Replacing note_data and note_size with struct buffer would be nice,
but the change would be quite big, I'm afraid.

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