Re: [PATCH 3/4] BFD: Fix reading Linux core PRSTATUS note for MIPS n32

["Maciej W. Rozycki" <macro at imgtec dot com>]

On Fri, 6 Oct 2017, Djordje Todorovic wrote:

> The kernel struct elf_prstatus which GDB MIPS n32 uses is defined as
> following:
> 
> (top-gdb-mipsN32) ptype struct elf_prstatus
> type = struct elf_prstatus {
>     struct elf_siginfo pr_info;
>     short pr_cursig;
>     unsigned long long pr_sigpend;
>     unsigned long long pr_sighold;
>     __pid_t pr_pid;
>     __pid_t pr_ppid;
>     __pid_t pr_pgrp;
>     __pid_t pr_sid;
>     struct timeval pr_utime;
>     struct timeval pr_stime;
>     struct timeval pr_cutime;
>     struct timeval pr_cstime;
>     elf_gregset_t pr_reg;
>     int pr_fpvalid;
> }
> 
> and the size of the structure is not right in the current source code,
> because:
> 
> (top-gdb-mipsN32) p sizeof(struct elf_prstatus)
> $1 = 448

 Well, `struct elf_prstatus' is for n64 only and n32 and o32 both use 
`struct elf_prstatus32', although a different one each, and 440 is the 
correct n32 size AFAICT:

(gdb) list arch/mips/kernel/binfmt_elfn32.c:40,62
40	#include <linux/module.h>
41	#include <linux/elfcore.h>
42	#include <linux/compat.h>
43	#include <linux/math64.h>
44
45	#define elf_prstatus elf_prstatus32
46	struct elf_prstatus32
47	{
48		struct elf_siginfo pr_info;	/* Info associated with signal */
49		short	pr_cursig;		/* Current signal */
50		unsigned int pr_sigpend;	/* Set of pending signals */
51		unsigned int pr_sighold;	/* Set of held signals */
52		pid_t	pr_pid;
53		pid_t	pr_ppid;
54		pid_t	pr_pgrp;
55		pid_t	pr_sid;
56		struct compat_timeval pr_utime; /* User time */
57		struct compat_timeval pr_stime; /* System time */
58		struct compat_timeval pr_cutime;/* Cumulative user time */
59		struct compat_timeval pr_cstime;/* Cumulative system time */
60		elf_gregset_t pr_reg;	/* GP registers */
61		int pr_fpvalid;		/* True if math co-processor being used.  */
62	};
(gdb) ptype struct elf_prstatus32
type = struct elf_prstatus32 {
    struct elf_siginfo pr_info;
    short pr_cursig;
    unsigned int pr_sigpend;
    unsigned int pr_sighold;
    pid_t pr_pid;
    pid_t pr_ppid;
    pid_t pr_pgrp;
    pid_t pr_sid;
    struct compat_timeval pr_utime;
    struct compat_timeval pr_stime;
    struct compat_timeval pr_cutime;
    struct compat_timeval pr_cstime;
    elf_gregset_t pr_reg;
    int pr_fpvalid;
}
(gdb) print sizeof(struct elf_prstatus32)
$1 = 440
(gdb) print sizeof((struct elf_prstatus32 *)0)->pr_reg
$2 = 360
(gdb) print /d &((struct elf_prstatus32 *)0)->pr_fpvalid
$3 = 432
(gdb) 

> Also, offset of the pr_pid and pr_reg have to be corrected:
> 
> (top-gdb-mipsN32) print /d &((struct elf_prstatus *)0)->pr_reg
> $2 = 80
> (top-gdb-mipsN32) print /d &((struct elf_prstatus *)0)->pr_pid
> $3 = 32

 Nope:

(gdb) print /d &((struct elf_prstatus32 *)0)->pr_reg
$4 = 72
(gdb) print /d &((struct elf_prstatus32 *)0)->pr_pid
$5 = 24
(gdb)

 Have I missed anything?

> Also, it is detected that on MIPS n32 platform, GDB has never called functions
> for reading Linux core PRPSINFO and PRSTATUS note defined in
> bfd/elfn32-mips.c, but GDB MIPS n32 currently uses functions from
> bfd/elf32-mips.c. I am not sure if it is expected, but
> 'elf32_mips_grok_psinfo' from bfd/elfn32-mips.c is exactly the same as one
> from bfd/elf32-mips.c, because GDB MIPS n32 uses exactly the same struct
> elf_prpsinfo and there is no problem for end users. But, when GDB MIPS n32
> comes into 'elf32_mips_grok_prstatus' from bfd/elf32-mips.c, it would never go
> into 'case 256' of the 'switch' because the size of struct elf_prstatus is
> different on MIPS n32.

 Well, GDB indeed does not use this stuff, however BFD does, to create 
artificial sections, as you can see in the very functions you've looked 
at.  So if you run say `objdump -h' over a core file, then you'll see 
sections which aren't really there, as easily verified with `readelf -S'.

 It would be good to have this feature verified in a target-dependent 
manner in the binutils test suite, however that's a separate matter.

> diff --git a/bfd/elfn32-mips.c b/bfd/elfn32-mips.c
> index 5287da3..07793b6 100644
> --- a/bfd/elfn32-mips.c
> +++ b/bfd/elfn32-mips.c
> @@ -3530,15 +3530,15 @@ elf32_mips_grok_prstatus (bfd *abfd, Elf_Internal_Note
> *note)
>        default:
>  	return FALSE;
> 
> -      case 440:		/* Linux/MIPS N32 */
> +      case 448:		/* Linux/MIPS N32 */
>  	/* pr_cursig */
>  	elf_tdata (abfd)->core->signal = bfd_get_16 (abfd, note->descdata +
> 12);
> 
>  	/* pr_pid */
> -	elf_tdata (abfd)->core->lwpid = bfd_get_32 (abfd, note->descdata +
> 24);
> +	elf_tdata (abfd)->core->lwpid = bfd_get_32 (abfd, note->descdata +
> 32);
> 
>  	/* pr_reg */
> -	offset = 72;
> +	offset = 80;
>  	size = 360;
> 
>  	break;

 So as I noted above unless you can prove me wrong, your proposed change 
looks invalid to me.

  Maciej