From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30424 invoked by alias); 3 Jan 2013 13:44:50 -0000 Received: (qmail 30406 invoked by uid 22791); 3 Jan 2013 13:44:49 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,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; Thu, 03 Jan 2013 13:44:39 +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 r03Did9w010820 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 3 Jan 2013 08:44:39 -0500 Received: from host2.jankratochvil.net (ovpn-116-24.ams2.redhat.com [10.36.116.24]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r03DiVQo015381 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 3 Jan 2013 08:44:34 -0500 Date: Thu, 03 Jan 2013 13:44: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: <20130103134431.GA4099@host2.jankratochvil.net> References: <20121218173747.GA24546@host2.jankratochvil.net> <20121218193104.GA29194@host2.jankratochvil.net> <20130101143027.GA17408@host2.jankratochvil.net> 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: 2013-01/txt/msg00037.txt.bz2 On Thu, 03 Jan 2013 00:32:12 +0100, Sergio Durigan Junior wrote: > On Tuesday, January 01 2013, Jan Kratochvil wrote: > > On Sun, 30 Dec 2012 02:49:36 +0100, Sergio Durigan Junior wrote: > >> --- 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. Suggesting for example instead of + char pr_psargs[ELF_PRARGSZ]; /* Initial part of arg list. */ to just use + char pr_psargs[80]; /* Initial part of arg list. */ although as I also suggested: On Tue, 01 Jan 2013 15:30:27 +0100, Jan Kratochvil wrote: > As this is only internal representation the sizes can be +1 including the > terminating '\0' for a more convenient use by the callers. it could be (no longer requiring those strncpy around): + char pr_psargs[81]; /* Initial part of arg list. */ If you prefer it could be (note the different bfd-only #define name): +#define BFD_ELF_PRARGSZ (80) + char pr_psargs[BFD_ELF_PRARGSZ + 1]; /* Initial part of arg list. */ but I do not see a reason for such #define. The goal is not to clash with the existing system symbol ELF_PRARGSZ as that's symbol meaning is different (irrelevant for the internal BFD representation). > >> +/* 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". As /usr/include/linux/elfcore.h (=from Linux kernel) contains: struct elf_prpsinfo { [...] pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid; /* Lots missing */ char pr_fname[16]; /* filename of executable */ [...] }; we should look at it from the point of Linux kernel. It probably made sense as Linux kernel could store some more info there. But currently neither Linux kernel nor BFD can change the binary format anymore so (1) I do not want to discuss whether it should be removed from Linux kernel or not, that is offtopic, (2) in BFD it seems irrelevant to me, BFD only needs to mimic the Linux kernel binary structure, there is nothing missing in BFD as BFD decodes very every field Linux kernel stores there. > >> +/* 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. Yes. Thanks, Jan