From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16263 invoked by alias); 2 Jan 2013 23:32:31 -0000 Received: (qmail 16247 invoked by uid 22791); 2 Jan 2013 23:32:30 -0000 X-SWARE-Spam-Status: No, hits=-4.8 required=5.0 tests=AWL,BAYES_50,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,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; Wed, 02 Jan 2013 23:32:19 +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 r02NWING004533 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 2 Jan 2013 18:32:18 -0500 Received: from psique (ovpn-113-50.phx2.redhat.com [10.3.113.50]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r02NWDxt032185; Wed, 2 Jan 2013 18:32:15 -0500 From: Sergio Durigan Junior To: Jan Kratochvil Cc: Binutils Development , GDB Patches , Pedro Alves , "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> X-URL: http://www.redhat.com Date: Wed, 02 Jan 2013 23:32:00 -0000 In-Reply-To: <20130101143027.GA17408@host2.jankratochvil.net> (Jan Kratochvil's message of "Tue, 1 Jan 2013 15:30:27 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2013-01/txt/msg00031.txt.bz2 On Tuesday, January 01 2013, Jan Kratochvil wrote: > thanks for the updated, I have just seen some more minor issues: Thanks for the comments. I have some questions. > On Sun, 30 Dec 2012 02:49:36 +0100, Sergio Durigan Junior wrote: >> --- a/bfd/Makefile.in >> +++ b/bfd/Makefile.in >> @@ -1068,7 +1068,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..90ced7a 100644 >> --- a/bfd/elf-bfd.h >> +++ b/bfd/elf-bfd.h >> @@ -1722,6 +1722,38 @@ struct elf_obj_tdata >> (elf_known_obj_attributes (bfd) [OBJ_ATTR_PROC]) >> #define elf_other_obj_attributes_proc(bfd) \ >> (elf_other_obj_attributes (bfd) [OBJ_ATTR_PROC]) >> + >> +#ifndef ELF_PRARGSZ >> +/* Maximum size of the arguments that can be stored in a PRPSINFO >> + structure. */ >> + >> +#define ELF_PRARGSZ (80) >> +#endif > > This should be unrelated to the system definition, BFD now defines everything > about PRPSINFO on its own and different system definiton of it can just break > it. Host system may be some exotic kind with no core files support by > BFD. Ok, I understand your argument, but it is not clear what you are suggesting. This definition should be present here and in the elf-psinfo.h file. >> +/* 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 */ > > This comment seems off-topic here. It fully represents the core file > structure. I can remove the comment, but it doesn't say that the structure does not represent a corefile struct. It just explains the reason why it is called "internal". >> + >> + 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. */ >> + >> +#ifndef ELF_PRARGSZ >> +/* Maximum size of the arguments that can be stored in a PRPSINFO >> + structure. */ >> + >> +#define ELF_PRARGSZ (80) >> +#endif > > The same comment as above. The same question as above. >> +/* Process info. In the end we do provide typedefs for them. */ >> + >> +typedef struct elf_external_prpsinfo32 prpsinfo32_t; >> +typedef struct elf_external_prpsinfo64 prpsinfo64_t; > > There was some comment that *_t types are reserved for system (POSIX?) use. > So when BFD now now longer uses the system ones and fully defines them on its > own they should have some different namespace, not *_t. Ok, but renaming these types will require renaming the files that use them as well. Thanks, -- Sergio