This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: Binutils Development <binutils at sourceware dot org>, GDB Patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>, "H.J. Lu" <hjl dot tools at gmail dot com>
- Date: Wed, 02 Jan 2013 21:32:12 -0200
- Subject: Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils
- References: <m3k3shv0g5.fsf@redhat.com> <20121218173747.GA24546@host2.jankratochvil.net> <m3y5gvqic5.fsf@redhat.com> <20121218193104.GA29194@host2.jankratochvil.net> <m3623kgvgv.fsf@redhat.com> <20130101143027.GA17408@host2.jankratochvil.net>
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