This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, MIPS] Extract PID from core dump file


Hi Maciej,

Thanks a lot for comments!


>   Would you be able to give me a recipe to reproduce a scenario where the effect of actions taken is not as expected? ...

Yes, since MIPS platforms have unexpected behavior for fetching TLS variable from native core files without reading PID, GDB test for it looks as following:

From ef60f031a8fbc60168998cdde2990da2fe885aaa Mon Sep 17 00:00:00 2001
From: Djordje Todorovic <djordje.todorovic@rt-rk.com>
Date: Thu, 24 Aug 2017 12:00:25 +0200
Subject: [PATCH 3/4] Add test for fetching TLS from core file

gdb/testsuite:

	* gdb.threads/tls-core.c: New.
	* gdb.threads/tls-core.exp: Likewise.
---
 gdb/testsuite/gdb.threads/tls-core.c   | 29 ++++++++++++++++
 gdb/testsuite/gdb.threads/tls-core.exp | 61 ++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/tls-core.c
 create mode 100644 gdb/testsuite/gdb.threads/tls-core.exp

diff --git a/gdb/testsuite/gdb.threads/tls-core.c b/gdb/testsuite/gdb.threads/tls-core.c
new file mode 100644
index 0000000..25f2a84
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/tls-core.c
@@ -0,0 +1,29 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <pthread.h>
+
+#define NUMBER_OF_THREADS 5
+
+int __thread foo = 0xdeadbeef;
+
+void* thread(void *in) {
+        int *tmp = (int *)in;
+        int value = *tmp;
+        foo += *tmp;
+        while (1) {
+                sleep(10);
+        }
+}
+
+int main(void) {
+
+        pthread_t threads[NUMBER_OF_THREADS];
+        int i;
+        for (i=0; i<NUMBER_OF_THREADS; i++) {
+                pthread_create(&threads[i], NULL,
+                               thread,&i);
+        }
+
+}
+
diff --git a/gdb/testsuite/gdb.threads/tls-core.exp b/gdb/testsuite/gdb.threads/tls-core.exp
new file mode 100644
index 0000000..d52fcf9
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/tls-core.exp
@@ -0,0 +1,61 @@
+# Copyright 2009-2017 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
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	 executable { debug }] != "" } {
+    return -1
+}
+
+
+clean_restart ${binfile}
+
+#
+#set breakpoint at line 27
+#
+gdb_breakpoint 27
+gdb_test "run" "Starting program: .*"
+
+#
+#generate corefile
+#
+set corefile [standard_output_file gcore.test]
+set core_supported [gdb_gcore_cmd "$corefile" "save a corefile"]
+if {!$core_supported} {
+  return -1
+}
+
+gdb_exit
+
+#
+#restart gdb and load generated corefile
+#
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set core_loaded [gdb_core_cmd "$corefile" "load generated corefile"]
+if { $core_loaded == -1 } {
+    # No use proceeding from here.
+    return
+}
+
+gdb_test "p/x foo" \
+                    "\\$\[0-9]+ = 0xdeadbeef" \
+                    "Printing thread-local storage variable."
+
+gdb_exit
+
--
2.7.4

Also, it is detected that MIPS32 GDB does not propagate particular information (such as PID) to core files when executing ‘gcore’ GDB command, so please find two patches bellow for that (bfd/ and gdb/). Testing on MIPS32 would not be useful without applying these patches, because reading PID does not make any sense without previously writing it into core file.

BFD side:

From 6c63900619ff7c7c4ef28378cfc3ff7c7ebf33b8 Mon Sep 17 00:00:00 2001
From: Djordje Todorovic <djordje.todorovic@rt-rk.com>
Date: Thu, 24 Aug 2017 11:12:50 +0200
Subject: [PATCH 1/4] [MIPS32] BFD: Write prpsinfo and prstatus into core file

On MIPS32 platform information such as PID was not correctly written
into core file from GDB.

bfd/ChangeLog:

	* bfd/elf-bfd.h (elfcore_write_mips_linux_prpsinfo32): New
	function declaration.
	* bfd/elf32-mips.c (elf_external_mips_linux_prpsinfo32): New
	structure.
	(swap_mips_linux_prpsinfo32_out): New function.
	(elfcore_write_mips_linux_prpsinfo32): Likewise.
	(elf32_mips_write_core_note): Likewise.
	(elf_backend_write_core_note): New macro.
---
 bfd/elf-bfd.h    |   4 +++
 bfd/elf32-mips.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 83958e4..e63da4e 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -2586,6 +2586,10 @@ extern char *elfcore_write_linux_prpsinfo64
 extern char *elfcore_write_ppc_linux_prpsinfo32
   (bfd *, char *, int *, const struct elf_internal_linux_prpsinfo *);

+/* Linux/MIPS32 uses different layout compared to other archs.  */
+extern char *elfcore_write_mips_linux_prpsinfo32
+  (bfd *, char *, int *, const struct elf_internal_linux_prpsinfo *);
+
 extern bfd *_bfd_elf32_bfd_from_remote_memory
   (bfd *templ, bfd_vma ehdr_vma, bfd_size_type size, bfd_vma *loadbasep,
    int (*target_read_memory) (bfd_vma, bfd_byte *, bfd_size_type));
diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index 8c1a68eb..5013cd4 100644
--- a/bfd/elf32-mips.c
+++ b/bfd/elf32-mips.c
@@ -1661,6 +1661,48 @@ static reloc_howto_type elf_mips_eh_howto =
 	 0xffffffff,	        /* dst_mask */
 	 FALSE);		/* pcrel_offset */

+/* External 32-bit MIPS structure for PRPSINFO. */
+
+struct elf_external_mips_linux_prpsinfo32
+  {
+    char pr_state;			/* Numeric process state.  */
+    char pr_sname;			/* Char for pr_state.  */
+    char pr_zomb;			/* Zombie.  */
+    char pr_nice;			/* Nice val.  */
+    char pr_flag[4];			/* Flags.  */
+    char pr_uid[4];
+    char pr_gid[4];
+    char pr_pid[4];
+    char pr_ppid[4];
+    char pr_pgrp[4];
+    char pr_sid[4];
+    char pr_fname[16];			/* Filename of executable.  */
+    char pr_psargs[80];			/* Initial part of arg list.  */
+  };
+
+/* Helper function to copy an elf_internal_linux_prpsinfo in host
+   endian to an elf_external_mips_linux_prpsinfo32 in target endian.  */
+
+static inline void
+swap_mips_linux_prpsinfo32_out (bfd *obfd,
+			       const struct elf_internal_linux_prpsinfo *from,
+			       struct elf_external_mips_linux_prpsinfo32 *to)
+{
+  bfd_put_8 (obfd, from->pr_state, &to->pr_state);
+  bfd_put_8 (obfd, from->pr_sname, &to->pr_sname);
+  bfd_put_8 (obfd, from->pr_zomb, &to->pr_zomb);
+  bfd_put_8 (obfd, from->pr_nice, &to->pr_nice);
+  bfd_put_32 (obfd, from->pr_flag, to->pr_flag);
+  bfd_put_32 (obfd, from->pr_uid, to->pr_uid);
+  bfd_put_32 (obfd, from->pr_gid, to->pr_gid);
+  bfd_put_32 (obfd, from->pr_pid, to->pr_pid);
+  bfd_put_32 (obfd, from->pr_ppid, to->pr_ppid);
+  bfd_put_32 (obfd, from->pr_pgrp, to->pr_pgrp);
+  bfd_put_32 (obfd, from->pr_sid, to->pr_sid);
+  strncpy (to->pr_fname, from->pr_fname, sizeof (to->pr_fname));
+  strncpy (to->pr_psargs, from->pr_psargs, sizeof (to->pr_psargs));
+}
+
 /* Set the GP value for OUTPUT_BFD.  Returns FALSE if this is a
    dangerous relocation.  */

@@ -2373,6 +2415,67 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)

   return TRUE;
 }
+
+char *
+elfcore_write_mips_linux_prpsinfo32
+  (bfd *abfd,
+   char *buf,
+   int *bufsiz,
+   const struct elf_internal_linux_prpsinfo *prpsinfo)
+{
+  struct elf_external_mips_linux_prpsinfo32 data;
+
+  swap_mips_linux_prpsinfo32_out (abfd, prpsinfo, &data);
+  return elfcore_write_note (abfd, buf, bufsiz,
+			     "CORE", NT_PRPSINFO, &data, sizeof (data));
+}
+
+static char *
+elf32_mips_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type, ...)
+{
+  switch (note_type)
+    {
+    default:
+      return NULL;
+
+    case NT_PRPSINFO:
+      {
+	char data[128];
+	va_list ap;
+
+	va_start (ap, note_type);
+	memset (data, 0, sizeof (data));
+	strncpy (data + 32, va_arg (ap, const char *), 16);
+	strncpy (data + 48, va_arg (ap, const char *), 80);
+	va_end (ap);
+	return elfcore_write_note (abfd, buf, bufsiz,
+				   "CORE", note_type, data, sizeof (data));
+      }
+
+    case NT_PRSTATUS:
+      {
+	char data[256];
+	va_list ap;
+	long pid;
+	int cursig;
+	const void *greg;
+
+	va_start (ap, note_type);
+	memset (data, 0, 72);
+	pid = va_arg (ap, long);
+	bfd_put_32 (abfd, pid, data + 24);
+	cursig = va_arg (ap, int);
+	bfd_put_16 (abfd, cursig, data + 12);
+	greg = va_arg (ap, const void *);
+	memcpy (data + 72, greg, 180);
+	memset (data + 252, 0, 4);
+	va_end (ap);
+	return elfcore_write_note (abfd, buf, bufsiz,
+				   "CORE", note_type, data, sizeof (data));
+      }
+    }
+}
+
 
 /* Depending on the target vector we generate some version of Irix
    executables or "normal" MIPS ELF ABI executables.  */
@@ -2479,6 +2582,7 @@ static const struct ecoff_debug_swap mips_elf32_ecoff_debug_swap = {
 					_bfd_mips_elf_copy_indirect_symbol
 #define elf_backend_grok_prstatus	elf32_mips_grok_prstatus
 #define elf_backend_grok_psinfo		elf32_mips_grok_psinfo
+#define elf_backend_write_core_note		elf32_mips_write_core_note
 #define elf_backend_ecoff_debug_swap	&mips_elf32_ecoff_debug_swap

 #define elf_backend_got_header_size	(4 * MIPS_RESERVED_GOTNO)
--
2.7.4

GDB side:

From 246a4e36884eb79a1b7ef208ad7041eb2e5a7100 Mon Sep 17 00:00:00 2001
From: Djordje Todorovic <djordje.todorovic@rt-rk.com>
Date: Thu, 24 Aug 2017 11:40:59 +0200
Subject: [PATCH 2/4] Hook in elfcore_write_mips_linux_prpsinfo32 on MIPS32

gdb/ChangeLog:

	* mips-linux-tdep.c: Include elf-bfd.h.
	(mips_linux_init_abi): Hook in elfcore_write_mips_linux_prpsinfo32
	on MIPS32.
---
 gdb/mips-linux-tdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 001fd40..e9d2712 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -39,6 +39,7 @@
 #include "linux-tdep.h"
 #include "xml-syscall.h"
 #include "gdb_signals.h"
+#include "elf-bfd.h"

 #include "features/mips-linux.c"
 #include "features/mips-dsp-linux.c"
@@ -1636,6 +1637,8 @@ mips_linux_init_abi (struct gdbarch_info info,
 	tramp_frame_prepend_unwinder (gdbarch, &mips_linux_o32_sigframe);
 	tramp_frame_prepend_unwinder (gdbarch, &mips_linux_o32_rt_sigframe);
 	set_xml_syscall_file_name (gdbarch, "syscalls/mips-o32-linux.xml");
+  set_gdbarch_elfcore_write_linux_prpsinfo (gdbarch,
+                elfcore_write_mips_linux_prpsinfo32);
 	break;
       case MIPS_ABI_N32:
 	set_gdbarch_get_longjmp_target (gdbarch,
--
2.7.4

So, finally the patch for reading PID from MIPS core files looks as following:

From 5eaeeae4270bb14874a23d3ecf3687212f60d8e2 Mon Sep 17 00:00:00 2001
From: Djordje Todorovic <djordje.todorovic@rt-rk.com>
Date: Thu, 24 Aug 2017 12:18:56 +0200
Subject: [PATCH 4/4] BFD: Extract PID from MIPS core dump file

On MIPS platforms, PID information was not correctly propagated
from core dump file to internal GDB structures.
This patch fixes that behavior.

bfd/ChangeLog:

     * elf32-mips.c (elf32_mips_grok_psinfo): Extract core->pid.
     * elf64-mips.c (elf64_mips_grok_psinfo): Likewise.
     * elfn32-mips.c (elf32_mips_grok_psinfo): Likewise.
---
 bfd/elf32-mips.c  | 2 ++
 bfd/elf64-mips.c  | 2 ++
 bfd/elfn32-mips.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index 5013cd4..d6891c1 100644
--- a/bfd/elf32-mips.c
+++ b/bfd/elf32-mips.c
@@ -2395,6 +2395,8 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
 	return FALSE;

       case 128:		/* Linux/MIPS elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 16);
 	elf_tdata (abfd)->core->program
 	 = _bfd_elfcore_strndup (abfd, note->descdata + 32, 16);
 	elf_tdata (abfd)->core->command
diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
index 84f2a3f..e45e362 100644
--- a/bfd/elf64-mips.c
+++ b/bfd/elf64-mips.c
@@ -4228,6 +4228,8 @@ elf64_mips_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
 	return FALSE;

       case 136:		/* Linux/MIPS - N64 kernel elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 24);
 	elf_tdata (abfd)->core->program
 	 = _bfd_elfcore_strndup (abfd, note->descdata + 40, 16);
 	elf_tdata (abfd)->core->command
diff --git a/bfd/elfn32-mips.c b/bfd/elfn32-mips.c
index dce7ba1..6d015fd 100644
--- a/bfd/elfn32-mips.c
+++ b/bfd/elfn32-mips.c
@@ -3558,6 +3558,8 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
 	return FALSE;

       case 128:		/* Linux/MIPS elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 16);
 	elf_tdata (abfd)->core->program
 	 = _bfd_elfcore_strndup (abfd, note->descdata + 32, 16);
 	elf_tdata (abfd)->core->command
--
2.7.4

> Any update on progress?  While I think your change can be ...
Yes, it is in process of signing.

Thanks,
Djordje Todorovic

On 16.08.2017. 12:42, Maciej W. Rozycki wrote:
> Hi Djordje,
>
>   Apologies for the long RTT.  Cc-ing `gdb' as this affects GDB processing.
>
>> On MIPS platforms, PID information was not correctly propagated from core dump
>> file to internal GDB structures. This patch fixes that behavior.
>>
>> The bug was found while trying to read TLS variable from core dump file on
>> MIPS platforms, but it was not able because GDB needs correct PID information
>> in order to do that.
>
>   Would you be able to give me a recipe to reproduce a scenario where the
> effect of actions taken is not as expected?  I'd like to have it covered
> by a GDB test suite case if possible.
>
>> This change is tested on MIPS64R2 board with o32, n32 and n64 executables.
>> Test program is forced to crash in order to generate core file. Core file is
>> loaded into GDB and bfd structure is checked if it has correct PID value, the
>> same value executable had before crash.
>
>   AFAICT however GDB prefers LWPID if available, so while I agree that
> technically we ought to retrieve PID as well, in reality it shouldn't
> really matter.  Having a test case demonstrating a user-visible effect
> would indeed help.
>
>> Currently I don't have copyright assignment. Couple a weeks ago my company
>> initiated process of getting copyright assignment  but with no response so
>> far.
>
>   Any update on progress?  While I think your change can be considered
> small enough not to require a copyright assignment for acceptance, it
> would be preferable if you had it.
>
>> diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
>> index 8c1a68eb..9ec2818 100644
>> --- a/bfd/elf32-mips.c
>> +++ b/bfd/elf32-mips.c
>> @@ -2353,6 +2353,8 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_Internal_Note
>> *note)
>>   	return FALSE;
>>
>>         case 128:		/* Linux/MIPS elf_prpsinfo */
>> +	elf_tdata (abfd)->core->pid
>> +	 = bfd_get_32 (abfd, note->descdata + 24);
>>   	elf_tdata (abfd)->core->program
>>   	 = _bfd_elfcore_strndup (abfd, note->descdata + 32, 16);
>>   	elf_tdata (abfd)->core->command
>
>   The offset isn't right however, you're fetching the process group ID
> rather than the process ID:
>
> (gdb) ptype struct elf_prpsinfo
> type = struct elf_prpsinfo {
>      char pr_state;
>      char pr_sname;
>      char pr_zomb;
>      char pr_nice;
>      unsigned long pr_flag;
>      __kernel_uid_t pr_uid;
>      __kernel_gid_t pr_gid;
>      pid_t pr_pid;
>      pid_t pr_ppid;
>      pid_t pr_pgrp;
>      pid_t pr_sid;
>      char pr_fname[16];
>      char pr_psargs[80];
> }
> (gdb) print /d &((struct elf_prpsinfo *)0)->pr_pid
> $1 = 16
> (gdb) print /d &((struct elf_prpsinfo *)0)->pr_pgrp
> $2 = 24
> (gdb)
>
> Same with n32.  The n64 variant is OK.
>
>    Maciej
>

On 16.08.2017. 12:42, Maciej W. Rozycki wrote:
Hi Djordje,

  Apologies for the long RTT.  Cc-ing `gdb' as this affects GDB processing.

On MIPS platforms, PID information was not correctly propagated from core dump
file to internal GDB structures. This patch fixes that behavior.

The bug was found while trying to read TLS variable from core dump file on
MIPS platforms, but it was not able because GDB needs correct PID information
in order to do that.

  Would you be able to give me a recipe to reproduce a scenario where the
effect of actions taken is not as expected?  I'd like to have it covered
by a GDB test suite case if possible.

This change is tested on MIPS64R2 board with o32, n32 and n64 executables.
Test program is forced to crash in order to generate core file. Core file is
loaded into GDB and bfd structure is checked if it has correct PID value, the
same value executable had before crash.

  AFAICT however GDB prefers LWPID if available, so while I agree that
technically we ought to retrieve PID as well, in reality it shouldn't
really matter.  Having a test case demonstrating a user-visible effect
would indeed help.

Currently I don't have copyright assignment. Couple a weeks ago my company
initiated process of getting copyright assignment  but with no response so
far.

  Any update on progress?  While I think your change can be considered
small enough not to require a copyright assignment for acceptance, it
would be preferable if you had it.

diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index 8c1a68eb..9ec2818 100644
--- a/bfd/elf32-mips.c
+++ b/bfd/elf32-mips.c
@@ -2353,6 +2353,8 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_Internal_Note
*note)
  	return FALSE;

        case 128:		/* Linux/MIPS elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 24);
  	elf_tdata (abfd)->core->program
  	 = _bfd_elfcore_strndup (abfd, note->descdata + 32, 16);
  	elf_tdata (abfd)->core->command

  The offset isn't right however, you're fetching the process group ID
rather than the process ID:

(gdb) ptype struct elf_prpsinfo
type = struct elf_prpsinfo {
     char pr_state;
     char pr_sname;
     char pr_zomb;
     char pr_nice;
     unsigned long pr_flag;
     __kernel_uid_t pr_uid;
     __kernel_gid_t pr_gid;
     pid_t pr_pid;
     pid_t pr_ppid;
     pid_t pr_pgrp;
     pid_t pr_sid;
     char pr_fname[16];
     char pr_psargs[80];
}
(gdb) print /d &((struct elf_prpsinfo *)0)->pr_pid
$1 = 16
(gdb) print /d &((struct elf_prpsinfo *)0)->pr_pgrp
$2 = 24
(gdb)

Same with n32.  The n64 variant is OK.

   Maciej



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]