This is the mail archive of the
gdb@sourceware.org
mailing list for the GDB project.
Re: [PATCH, MIPS] Extract PID from core dump file
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Djordje Todorovic <djordje dot todorovic at rt-rk dot com>
- Cc: <binutils at sourceware dot org>, <gdb at sourceware dot org>, <nemanja dot popov at rt-rk dot com>, <nikola dot prica at rt-rk dot com>, <petar dot jovanovic at rt-rk dot com>, <asowda at cisco dot com>, <ibaev at cisco dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 28 Sep 2017 12:55:53 +0100
- Subject: Re: [PATCH, MIPS] Extract PID from core dump file
- Authentication-results: sourceware.org; auth=none
- References: <593520EC.3050803@rt-rk.com> <alpine.DEB.2.00.1706160019330.23046@tp.orcam.me.uk> <5947CFF6.2070703@rt-rk.com> <alpine.DEB.2.00.1708161122210.17596@tp.orcam.me.uk> <e8c0319f-f4b7-e4f2-351b-f3b58b14f471@rt-rk.com>
Hi Djordje,
Thank you for your patch update and apologies for the delay -- I had to
understand how core files are handled in BFD/GDB, which is an area I
haven't dealt with before.
Since you have created multiple patches now to cover independent separate
issues (which is good), next time please submit them as a proper patch
series, i.e. one patch per email rather than all grouped in a single
e-mail, as this has been the established practice, making it easier to
review changes proposed. You should be able to get assistance with that
from GIT with `git format-patch'.
See also: <https://sourceware.org/gdb/wiki/ContributionChecklist>.
So as not to hinder progress I've reviewed your submission as it is now
though.
> > 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:
Great, thanks!
> 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.
Traditionally we write such entries as "New file.", please adjust.
> 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 @@
You need a copyright notice at the beginning of a new source file, i.e.:
/* This test is part of GDB, the GNU debugger.
Copyright 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/>. */
> +#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);
> + }
> +
> +}
Please reformat according to the GNU Coding Standard and its GDB
clarification:
<https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Formatting_Your_Code>
<https://www.gnu.org/prep/standards/standards.html#Formatting>
Specifically using indentation by 2 spaces including curly braces and with
the opening brace on the next line, the function name aligned to the first
column, and a space between a function name and the opening parenthesis
and also around operators and after a cast, like:
int
foo (int x, int y)
{
if (x)
{
int z;
z = bar (x) + bar ((unsigned int) y);
if (z)
return z;
}
return y;
}
> +
Also delete the empty line at the end, we don't want them.
> 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.
As this is a new addition only 2017 should be specified for the copyright
year.
> +
> +# 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
Add a space missing at the beginning and also capitalise the sentence and
add a full stop at the end (following the rules for comments specified by
the standards referred above). Likewise throughout.
> 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.
I suspected that it could be the case.
In native testing we could try to arrange for the kernel to dump core by
sending an appropriate signal to the debuggee (I reckon we have some
previous art for issuing a signal in the test suite), subject to `ulimit
-c', however that would be unreliable exactly because of the latter
constraint.
> 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.
I think s/MIPS32/o32 MIPS/ will make it clearer.
Also n32 uses exactly the same `struct elf_prpsinfo' layout as o32 does,
so why didn't you need to handle both? And what about n64? -- you didn't
mention anything about it.
So I went ahead and looked into it and realised that the problem with
incorrect `struct elf_prpsinfo' layout actually applied to multiple Linux
targets and decided that it would not be appropriate to burden you with
such wide and also tedious to make a change on your first submission. So
I have now covered this part of the issue with a small patch series which
I posted last week, cc-ing you to make you aware, that I am going to apply
as soon as the GDB side has been approved by a general maintainer.
This covers writing the PRPSINFO note, however your change is still
needed, to handle the PRSTATUS note; see below.
> bfd/ChangeLog:
>
> * bfd/elf-bfd.h (elfcore_write_mips_linux_prpsinfo32): New
> function declaration.
"New prototype" would do (although this will obviously go now, see
below).
> * 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));
> +}
As noted above with all Linux targets handled correctly by
`elfcore_write_linux_prpsinfo32' and `elfcore_write_linux_prpsinfo64' a
MIPS target-specific handler is no longer needed. Please discard the
parts above then.
> +
> +static char *
> +elf32_mips_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type,
> ...)
> +{
This is a new function, so you need to document it. Please write an
appropriate description in a comment immediately above it (with an empty
line separating).
> + 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));
> + }
> +
See commit b3ac9c77560a ("Put more info in NT_PRPSINFO Linux notes"),
<https://sourceware.org/ml/binutils/2013-02/msg00024.html>, which made
`linux_make_corefile_notes' stop calling `elfcore_write_prpsinfo', leaving
`fbsd_make_corefile_notes' in gdb/fbsd-tdep.c and
`procfs_make_note_section' in gdb/procfs.c as the only callers requesting
a NT_PRPSINFO note. These callers are only used with FreeBSD and Solaris
systems respectively, so there is no need to handle the Linux layout.
We also need to make sure the FreeBSD target is not affected (there's no
MIPS port of Solaris, so we don't have to worry about it). See below for
further details as to how to avoid this function being called for FreeBSD.
With that in place the function will not be supposed to be called for
NT_PRPSINFO, and to make sure a future change does not cause it to be by
accident, please replace the NT_PRPSINFO code with:
case NT_PRPSINFO:
BFD_FAIL ();
return NULL;
as a safety check.
> + 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));
> + }
> + }
> +}
Conversely `elfcore_write_prstatus' is still called to create NT_PRSTATUS
notes even for Linux targets, from `linux_collect_regset_section_cb' (and
similarly with FreeBSD and Solaris). I think eventually it should be
converted similarly to how NT_PRPSINFO notes have, however for the time
being we need to handle NT_PRSTATUS notes correctly with whatever
infrastructure we have.
This implementation looks right to me, however you still have to provide
n32 and n64 variants, whose `struct elf_prstatus' members' data types have
different sizes (and consequently their offsets change too).
> @@ -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
As noted above we need to make sure the FreeBSD target is not affected.
You actually placed this definition in the general part of the backend,
which would be fine if it was to apply to all MIPS targets. We want it
for Linux only though, so please move it down, like this:
#define ELF_MAXPAGESIZE 0x10000
#define ELF_COMMONPAGESIZE 0x1000
#define elf32_bed elf32_tradbed
+
+#define elf_backend_write_core_note elf32_mips_write_core_note
/* Include the target file again for this target. */
#include "elf32-target.h"
so that it does not apply for IRIX, and then undefine it for FreeBSD (and
consequently VxWorks):
#undef elf32_bed
#define elf32_bed elf32_fbsd_tradbed
+
+#undef elf_backend_write_core_note
#include "elf32-target.h"
/* Implement elf_backend_final_write_processing for VxWorks. */
Likewise for n32 and n64.
> 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.
With `set_gdbarch_elfcore_write_linux_prpsinfo' gone and the PRPSINFO
note handled in generic code across all Linux targets this patch is now
not needed.
> 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);
You haven't corrected the offset, which was requested in the previous
iteration.
> 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);
Likewise.
Please resubmit as a proper patch series, with the issues pointed out
corrected, or ask if you find anything unclear or have any other
questions.
Maciej