From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 746 invoked by alias); 18 Dec 2012 17:38:14 -0000 Received: (qmail 590 invoked by uid 22791); 18 Dec 2012 17:38:12 -0000 X-SWARE-Spam-Status: No, hits=-4.8 required=5.0 tests=AWL,BAYES_50,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 18 Dec 2012 17:37:57 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBIHbuGo030879 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 18 Dec 2012 12:37:56 -0500 Received: from host2.jankratochvil.net (ovpn-116-39.ams2.redhat.com [10.36.116.39]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qBIHbmLi014405 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 18 Dec 2012 12:37:51 -0500 Date: Tue, 18 Dec 2012 17:38:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: Binutils Development , GDB Patches , Pedro Alves , "H.J. Lu" Subject: Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils Message-ID: <20121218173747.GA24546@host2.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-12/txt/msg00650.txt.bz2 Hi Sergio, I do not feel too confident in bfd/ but at least there may be more response to my wrong comments. > diff --git a/bfd/Makefile.in b/bfd/Makefile.in > index 92d9d08..e12deb5 100644 > --- a/bfd/Makefile.in > +++ b/bfd/Makefile.in > @@ -1050,7 +1050,7 @@ BUILD_CFILES = \ > CFILES = $(SOURCE_CFILES) $(BUILD_CFILES) > SOURCE_HFILES = \ > aout-target.h aoutf1.h aoutx.h coffcode.h coffswap.h ecoffswap.h \ > - elf-bfd.h elf-hppa.h elf32-hppa.h \ > + elf-bfd.h elf-psinfo.h elf-hppa.h elf32-hppa.h \ > elf64-hppa.h elfcode.h elfcore.h \ > freebsd.h genlink.h go32stub.h \ > libaout.h libbfd.h libcoff.h libecoff.h libhppa.h libieee.h \ > diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h > index b8d82b1..d314291 100644 > --- a/bfd/elf-bfd.h > +++ b/bfd/elf-bfd.h > @@ -2234,11 +2234,15 @@ extern bfd_boolean bfd_elf_lookup_section_flags > extern Elf_Internal_Phdr * _bfd_elf_find_segment_containing_section > (bfd * abfd, asection * section); > > +/* Forward declaration of prpsinfo. See `elf-psinfo.h' for more details. */ > + > +struct elf_internal_prpsinfo; > + > /* Exported interface for writing elf corefile notes. */ > extern char *elfcore_write_note > (bfd *, char *, int *, const char *, int, const void *, int); > extern char *elfcore_write_prpsinfo > - (bfd *, char *, int *, const char *, const char *); > + (bfd *, char *, int *, const struct elf_internal_prpsinfo *); > extern char *elfcore_write_prstatus > (bfd *, char *, int *, long, int, const void *); > extern char * elfcore_write_pstatus > diff --git a/bfd/elf-psinfo.h b/bfd/elf-psinfo.h > new file mode 100644 > index 0000000..61b38e1 > --- /dev/null > +++ b/bfd/elf-psinfo.h > @@ -0,0 +1,215 @@ > +/* Declarations for PRPSINFO structures under ELF on GNU/Linux. > + Copyright 2012 Free Software Foundation, Inc. > + > + This file is part of BFD, the Binary File Descriptor library. > + > + 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, write to the Free Software > + Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, > + MA 02110-1301, USA. */ > + > +#undef HAVE_PRPSINFO32_T > +#define HAVE_PRPSINFO32_T > +#undef HAVE_PRPSINFO32_T_PR_PID > +#define HAVE_PRPSINFO32_T_PR_PID This does not seem to be useful, these are auto-detected by configure and elf-psinfo.h is included unconditionally. Therefore they could be completely removed. But in reality they should not be removed as your elf-core definitions do not cover very every target supported by bfd, only some of them. > + > +/* Maximum size of the arguments that can be stored in a PRPSINFO > + structure. */ > + > +#define ELF_PRARGSZ (80) > + > +/* Internal structure which holds information to be included in the > + PRPSINFO section of the corefile. > + > + This is an "internal" structure in the sense that it should be used to > + pass information to BFD (via the `elfcore_write_prpsinfo', for example), > + so things like endianess shouldn't be an issue. This structure will > + eventually be converted in one of the `elf_external_*' structures > + below. */ > + > +struct elf_internal_prpsinfo > + { > + char pr_state; /* Numeric process state. */ > + char pr_sname; /* Char for pr_state. */ > + char pr_zomb; /* Zombie. */ > + char pr_nice; /* Nice val. */ > + unsigned long pr_flag; /* Flags. */ > + unsigned int pr_uid; > + unsigned int pr_gid; > + int pr_pid, pr_ppid, pr_pgrp, pr_sid; > + /* Lots missing */ > + char pr_fname[16]; /* Filename of executable. */ > + char pr_psargs[ELF_PRARGSZ]; /* Initial part of arg list. */ > + }; > + > +/* External 32-bit structure for PRPSINFO. This structure is ABI-defined, > + thus we choose to use char arrays here in order to avoid dealing with > + different types in different architectures. > + > + This structure will ultimately be written in the corefile's note section, > + as the PRPSINFO. */ > + > +struct elf_external_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[2]; > + char pr_gid[2]; > + char pr_pid[4]; > + char pr_ppid[4]; > + char pr_pgrp[4]; > + char pr_sid[4]; > + /* Lots missing */ > + char pr_fname[16]; /* Filename of executable. */ > + char pr_psargs[ELF_PRARGSZ]; /* Initial part of arg list. */ > + }; As it does not match very every arch I believe you should just copy it into elf*-*.c files containing the *_elf_write_core_note function for archs where you have verified (by comparing some headers, not just by experiment) it does match. > + > +/* External 32-bit PPC structure for PRPSINFO. If it is arch-specific then it should not be in a generic file. I believe elf32-ppc.c and elf64-ppc.c files are fine for it. In the end I do not see a use for this header file. > diff --git a/bfd/elf.c b/bfd/elf.c > index a92dd5d..a39b88c 100644 > --- a/bfd/elf.c > +++ b/bfd/elf.c > @@ -44,6 +44,7 @@ SECTION > #include "elf-bfd.h" > #include "libiberty.h" > #include "safe-ctype.h" > +#include "elf-psinfo.h" > > #ifdef CORE_HEADER > #include CORE_HEADER > @@ -8161,13 +8162,6 @@ elfcore_grok_arm_vfp (bfd *abfd, Elf_Internal_Note *note) > return elfcore_make_note_pseudosection (abfd, ".reg-arm-vfp", note); > } > > -#if defined (HAVE_PRPSINFO_T) > -typedef prpsinfo_t elfcore_psinfo_t; > -#if defined (HAVE_PRPSINFO32_T) /* Sparc64 cross Sparc32 */ > -typedef prpsinfo32_t elfcore_psinfo32_t; > -#endif > -#endif > - This is arch-independent file (elf.c). Some targets are unverified they match your new ABI elfcore definition. As they may provide correct system prpsinfo_t defintion you should use it there, if available. > #if defined (HAVE_PSINFO_T) > typedef psinfo_t elfcore_psinfo_t; > #if defined (HAVE_PSINFO32_T) /* Sparc64 cross Sparc32 */ > @@ -8201,17 +8195,17 @@ _bfd_elfcore_strndup (bfd *abfd, char *start, size_t max) > return dups; > } > > -#if defined (HAVE_PRPSINFO_T) || defined (HAVE_PSINFO_T) All these changes are like the comment above. > static bfd_boolean > elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note) > { > +#if defined (HAVE_PSINFO_T) > if (note->descsz == sizeof (elfcore_psinfo_t)) > { > elfcore_psinfo_t psinfo; > > memcpy (&psinfo, note->descdata, sizeof (psinfo)); > > -#if defined (HAVE_PSINFO_T_PR_PID) || defined (HAVE_PRPSINFO_T_PR_PID) > +#if defined (HAVE_PSINFO_T_PR_PID) > elf_tdata (abfd)->core_pid = psinfo.pr_pid; > #endif > elf_tdata (abfd)->core_program > @@ -8222,7 +8216,7 @@ elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note) > = _bfd_elfcore_strndup (abfd, psinfo.pr_psargs, > sizeof (psinfo.pr_psargs)); > } > -#if defined (HAVE_PRPSINFO32_T) || defined (HAVE_PSINFO32_T) > +#if defined (HAVE_PSINFO32_T) > else if (note->descsz == sizeof (elfcore_psinfo32_t)) > { > /* 64-bit host, 32-bit corefile */ > @@ -8230,7 +8224,7 @@ elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note) > > memcpy (&psinfo, note->descdata, sizeof (psinfo)); > > -#if defined (HAVE_PSINFO32_T_PR_PID) || defined (HAVE_PRPSINFO32_T_PR_PID) > +#if defined (HAVE_PSINFO32_T_PR_PID) > elf_tdata (abfd)->core_pid = psinfo.pr_pid; > #endif > elf_tdata (abfd)->core_program > @@ -8261,10 +8255,14 @@ elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note) > if (0 < n && command[n - 1] == ' ') > command[n - 1] = '\0'; > } > +#else /* defined (HAVE_PSINFO_T) */ > + /* Avoid compiler warning about "unused variables". */ > + (void) abfd; > + (void) note; > > return TRUE; > +#endif > } > -#endif /* defined (HAVE_PRPSINFO_T) || defined (HAVE_PSINFO_T) */ > > #if defined (HAVE_PSTATUS_T) > static bfd_boolean > @@ -9062,16 +9060,16 @@ char * > elfcore_write_prpsinfo (bfd *abfd, > char *buf, > int *bufsiz, > - const char *fname, > - const char *psargs) > + const struct elf_internal_prpsinfo *input) > { > const struct elf_backend_data *bed = get_elf_backend_data (abfd); > > if (bed->elf_backend_write_core_note != NULL) > { > char *ret; > + > ret = (*bed->elf_backend_write_core_note) (abfd, buf, bufsiz, > - NT_PRPSINFO, fname, psargs); > + NT_PRPSINFO, input); > if (ret != NULL) > return ret; > } > @@ -9089,8 +9087,8 @@ elfcore_write_prpsinfo (bfd *abfd, > #endif > > memset (&data, 0, sizeof (data)); > - strncpy (data.pr_fname, fname, sizeof (data.pr_fname)); > - strncpy (data.pr_psargs, psargs, sizeof (data.pr_psargs)); > + strncpy (data.pr_fname, input->pr_fname, sizeof (data.pr_fname)); > + strncpy (data.pr_psargs, input->pr_psargs, sizeof (data.pr_psargs)); > return elfcore_write_note (abfd, buf, bufsiz, > "CORE", note_type, &data, sizeof (data)); > } > @@ -9101,13 +9099,13 @@ elfcore_write_prpsinfo (bfd *abfd, > psinfo_t data; > int note_type = NT_PSINFO; > #else > - prpsinfo_t data; > + prpsinfo64_t data; > int note_type = NT_PRPSINFO; > #endif > > memset (&data, 0, sizeof (data)); > - strncpy (data.pr_fname, fname, sizeof (data.pr_fname)); > - strncpy (data.pr_psargs, psargs, sizeof (data.pr_psargs)); > + strncpy (data.pr_fname, input->pr_fname, sizeof (data.pr_fname)); > + strncpy (data.pr_psargs, input->pr_psargs, sizeof (data.pr_psargs)); > return elfcore_write_note (abfd, buf, bufsiz, > "CORE", note_type, &data, sizeof (data)); > } > diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c > index fd7d26a..cacc895 100644 > --- a/bfd/elf32-arm.c > +++ b/bfd/elf32-arm.c > @@ -30,6 +30,7 @@ > #include "elf-nacl.h" > #include "elf-vxworks.h" > #include "elf/arm.h" > +#include "elf-psinfo.h" > > /* Return the relocation section associated with NAME. HTAB is the > bfd's elf32_arm_link_hash_entry. */ > @@ -2004,17 +2005,19 @@ elf32_arm_nabi_write_core_note (bfd *abfd, char *buf, int *bufsiz, > > case NT_PRPSINFO: > { > - char data[124]; > + const struct elf_internal_prpsinfo *prpsinfo; > + struct elf_external_prpsinfo32 data; > va_list ap; > > va_start (ap, note_type); > - memset (data, 0, sizeof (data)); > - strncpy (data + 28, va_arg (ap, const char *), 16); > - strncpy (data + 44, va_arg (ap, const char *), 80); > + prpsinfo = va_arg (ap, const struct elf_internal_prpsinfo *); > va_end (ap); > > + memset (&data, 0, sizeof (data)); > + PRPSINFO32_COPY_FIELDS (abfd, prpsinfo, data); > + > return elfcore_write_note (abfd, buf, bufsiz, > - "CORE", note_type, data, sizeof (data)); > + "CORE", note_type, &data, sizeof (data)); > } > > case NT_PRSTATUS: > diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c > index a188cec..5b81baa 100644 > --- a/bfd/elf32-i386.c > +++ b/bfd/elf32-i386.c > @@ -31,12 +31,20 @@ > #include "objalloc.h" > #include "hashtab.h" > #include "dwarf2.h" > +#include "elf-bfd.h" > +#include "elf-psinfo.h" > + > +#include > > /* 386 uses REL relocations instead of RELA. */ > #define USE_REL 1 > > #include "elf/i386.h" > > +#ifdef CORE_HEADER > +#include CORE_HEADER > +#endif I believe new CORE_HEADER should not be introduced, they are rather deprecated as they depend on system which is not compatible for cross-build core handling. (But I may be completely wrong here.) > + > static reloc_howto_type elf_howto_table[]= > { > HOWTO(R_386_NONE, 0, 0, 0, FALSE, 0, complain_overflow_bitfield, > @@ -500,6 +508,67 @@ elf_i386_grok_psinfo (bfd *abfd, Elf_Internal_Note *note) > > return TRUE; > } > + > +static char * > +elf_i386_write_core_note (bfd *abfd, char *buf, int *bufsiz, > + int note_type, ...) > +{ > + const struct elf_backend_data *bed = get_elf_backend_data (abfd); > + va_list ap; > + const struct elf_internal_prpsinfo *prpsinfo; > + long pid; > + int cursig; > + const void *gregs; > + struct elf_external_prpsinfo32 data; > + > + switch (note_type) > + { > + default: > + return NULL; > + > + case NT_PRPSINFO: > + va_start (ap, note_type); > + prpsinfo = va_arg (ap, const struct elf_internal_prpsinfo *); > + va_end (ap); Maybe to use a union pointer instead? But nothing serious, fine with va_arg. > + > + memset (&data, 0, sizeof (data)); > + PRPSINFO32_COPY_FIELDS (abfd, prpsinfo, data); > + > + return elfcore_write_note (abfd, buf, bufsiz, "CORE", note_type, > + &data, sizeof (data)); > + /* NOTREACHED */ Thanks, Jan