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

"Maciej W. Rozycki" <[email protected]>
Newsgroups gmane.comp.gdb.patches,gmane.comp.gnu.binutils,gmane.comp.gdb.devel
Message-ID <[email protected]> 
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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.