Re: [PATCH 2/2] [nto] Improve ABI sniffing.

[Pedro Alves <palves@redhat.com>]

On 10/20/2015 07:10 PM, Aleksandar Ristovski wrote:
> Use qnx specific notes to figure out the OS.
> 
> gdb/ChangeLog:
> 	* gdb/nto-tdep.c (QNX_NOTE_NAME, QNX_INFO_SECT_NAME): New defines.
> 	(nto_sniff_abi_note_section): New function.
> 	(nto_elf_osabi_sniffer): Use new function to recognize nto specific
> 	binary.
> ---
>  gdb/nto-tdep.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
> index e50d302..48826cb 100644
> --- a/gdb/nto-tdep.c
> +++ b/gdb/nto-tdep.c
> @@ -32,6 +32,9 @@
>  #include "gdbcore.h"
>  #include "objfiles.h"
>  
> +#define QNX_NOTE_NAME	"QNX"
> +#define QNX_INFO_SECT_NAME "QNX_info"
> +
>  #ifdef __CYGWIN__
>  #include <sys/cygwin.h>
>  #endif
> @@ -332,12 +335,51 @@ nto_dummy_supply_regset (struct regcache *regcache, char *regs)
>    /* Do nothing.  */
>  }
>  
> +static void
> +nto_sniff_abi_note_section (bfd *abfd, asection *sect, void *obj)
> +{
> +  const char *sectname;
> +  unsigned int sectsize;
> +  char *note; // buffer holding the section contents

Please use /**/ format for comments, and write full sentences - start
with uppercase, period at end.  Usually that leads to putting the comment
above, e.g.:

 +  /* Buffer holding the section contents.  */
 +  char *note;

> +  unsigned int namelen;
> +  const char *name;
> +
> +  sectname = bfd_get_section_name (abfd, sect);
> +  sectsize = bfd_section_size (abfd, sect);
> +
> +  /* TODO: limit the note size here, for now limit is 128 bytes
> +     (enough to check the name and type).  */

This reads like limiting is left to do, but then it does
implement a limit.  So this TODO comment is confusing.

You should also make sure the section is the at least
the minimum size you expect though.

> +  if (sectsize > 128)
> +    sectsize = 128;
> +
> +  if (sectname && strstr (sectname, QNX_INFO_SECT_NAME) != NULL)
> +    *(enum gdb_osabi *) obj = GDB_OSABI_QNXNTO;
> +

sectname != NULL

> +  if (sectname && strstr (sectname, "note") != NULL)

sectname != NULL


> +    {
> +      note = alloca (sectsize);

For C++, write:

      note = (char *) alloca (sectsize);


> +      bfd_get_section_contents (abfd, sect, note, 0, sectsize);
> +      namelen = (unsigned int) bfd_h_get_32 (abfd, note);
> +      name = note + 12;
> +
> +      if (namelen > 0
> +	  && (0 == strcmp (name, QNX_NOTE_NAME)))
> +        *(enum gdb_osabi *) obj = GDB_OSABI_QNXNTO;
> +    }
> +}
> +
>  enum gdb_osabi
>  nto_elf_osabi_sniffer (bfd *abfd)
>  {
> -  if (nto_is_nto_target)
> +  enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
> +
> +  bfd_map_over_sections (abfd,
> +			 nto_sniff_abi_note_section,
> +			 &osabi);
> +
> +  if (osabi == GDB_OSABI_UNKNOWN && nto_is_nto_target)
>      return nto_is_nto_target (abfd);

...
nto_is_nto_target = procfs_is_nto_target;
...

static enum gdb_osabi
procfs_is_nto_target (bfd *abfd)
{
  return GDB_OSABI_QNXNTO;
}

So that basically could be rewritten as:

  if (osabi == GDB_OSABI_UNKNOWN && nto_is_nto_target)
     return GDB_OSABI_QNXNTO;


But, do we still need this nto_is_nto_target hack here?
Now with proper sniffing, can't we just remove it altogether?

> -  return GDB_OSABI_UNKNOWN;
> +  return osabi;
>  }
>  
>  static const char *nto_thread_state_str[] =
> 


Thanks,
Pedro Alves