From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28699 invoked by alias); 10 Jan 2013 18:26:30 -0000 Received: (qmail 28648 invoked by uid 22791); 10 Jan 2013 18:26:27 -0000 X-SWARE-Spam-Status: No, hits=-6.4 required=5.0 tests=AWL,BAYES_50,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Thu, 10 Jan 2013 18:26:21 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0AIQI8H009936 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 10 Jan 2013 13:26:18 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r0AIQGuD001999; Thu, 10 Jan 2013 13:26:17 -0500 Message-ID: <50EF07C8.3050307@redhat.com> Date: Thu, 10 Jan 2013 18:26:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Sergio Durigan Junior CC: Jan Kratochvil , Binutils Development , GDB Patches , "H.J. Lu" Subject: Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils References: <20121218173747.GA24546@host2.jankratochvil.net> <20121218193104.GA29194@host2.jankratochvil.net> <20130101143027.GA17408@host2.jankratochvil.net> <20130103134431.GA4099@host2.jankratochvil.net> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2013-01/txt/msg00200.txt.bz2 On 01/04/2013 04:39 AM, Sergio Durigan Junior wrote:> +++ b/bfd/elf-psinfo.h > @@ -0,0 +1,124 @@ > +/* Definitions for PRPSINFO structures under ELF on GNU/Linux. What about other OSs, like e.g., Solaris? > +/* External 64-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. > + > + Differently from the 32-bit version, the PowerPC guys made our lives better > + and used the same size as the other architectures. In the current revision, this comment appears out of the blue. Better would be to instead add a comment further up, above the 32-bit version stating that these are used by most architectures, although some define their own versions (like e.g., PPC). > + > + This structure will ultimately be written in the corefile's note section, > + as the PRPSINFO. */ > + > +struct elf_external_prpsinfo64 > +#define PRPSINFO32_COPY_FIELDS(abfd, from, to) \ > + do \ Personally, I'd prefer calling these SWAP instead of COPY, as you're not really doing straight copying, and "swap" is what bfd calls this idiom about everywhere. ... > +/* Process info. In the end we do provide typedefs for them. */ > + > +typedef struct elf_external_prpsinfo32 prpsinfo32; > +typedef struct elf_external_prpsinfo64 prpsinfo64; What's the point of these typedefs? To me, they just obscure things, and are easily confused with the host types... > @@ -8165,7 +8166,7 @@ elfcore_grok_arm_vfp (bfd *abfd, Elf_Internal_Note *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; > +typedef prpsinfo32 elfcore_psinfo32_t; ... like here. HAVE_PRPSINFO32_T/prpsinfo32_t were about a host type. prpsinfo32 is always defined (it's a bfd type). This in fact looks suspiciously wrong, and a potential breakage if in fact Sparc32 has a different psinfo32_t (what OS was that?), or in the other spots, some other arcane port/OS/arch having a different psinfo32_t than the Linux one. Either the #ifdef can be removed and this made unconditional, and host independent (the best), or the type should remain the host type (until all ports convert to implement elf_backend_write_core_note&co host-independent hooks). > #endif > #endif > > + The reason why we have a different structure only for PPC is because > + on this architecture (and *only* here!) the size of 32-bit structure This "only here" bit of the comment should go away, because it'll generate confusion whenever some other arch needs its own structure, making this outdated, because nobody will remember to update it. > @@ -415,14 +417,13 @@ elf_x86_64_grok_psinfo (bfd *abfd, Elf_Internal_Note *note) > return TRUE; > } > > -#ifdef CORE_HEADER > static char * > elf_x86_64_write_core_note (bfd *abfd, char *buf, int *bufsiz, > int note_type, ...) > { ... > case NT_PRSTATUS: > +#ifdef CORE_HEADER > va_start (ap, note_type); > pid = va_arg (ap, long); > cursig = va_arg (ap, int); > @@ -498,10 +501,13 @@ elf_x86_64_write_core_note (bfd *abfd, char *buf, int *bufsiz, > return elfcore_write_note (abfd, buf, bufsiz, "CORE", note_type, > &prstat, sizeof (prstat)); > } > +#else > + return NULL; > +#endif /* CORE_HEADER */ > } > /* NOTREACHED */ > } > -#endif > + Do you plan on following up with similar treatment for prstatus? Thanks for doing this, -- Pedro Alves