This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]