Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace

["Ian Lance Taylor via gcc-patches" <gcc-patches at gcc dot gnu dot org>]

On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
<d.khalikov@partner.samsung.com> wrote:
> Hello everyone,
>
> This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631
>
> Can some one please review attached patch.

Sorry to take so long about this.  It's a lot to look at.



> diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
> index 096ceb6..4bd97f3 100644
> --- a/libbacktrace/ChangeLog
> +++ b/libbacktrace/ChangeLog

It's best if you don't include the ChangeLog as part of the patch.
The patch never applies cleanly anyhow.  Just put the ChangeLog entry
in the e-mail or as a separate attachment.  Actually I guess you did
that part, just leave ChangeLog out of the patch.  Thanks.

> +AC_CHECK_HEADERS(limits.h)
> +
> +AC_CHECK_HEADERS(sys/param.h)

May as well put these in a single AC_CHECK_HEADERS.  But actually it
looks like you only want these to determine PATH_MAX, and I don't
think it's worth it.  Just use 4096.

> +  LINK = 1,
> +  REGULAR = 2

These names, and also DYN, INVALID, and EXEC, are a little too short
and too likely to conflict with some sloppy system header file.  The
enums in this code all use a prefix for each element; do that here
too.

> +/* Cast from void pointer to uint32_t.  */
> +
> +static uint32_t
> +getl32 (void *p)
> +{
> +  char *addr = (char *) p;
> +  uint32_t v = 0;
> +  v = *((uint32_t *) addr);
> +  return v;
> +}

The comment here doesn't look right; this isn't a cast.  And the
argument should really be char*.  But more importantly, the name
suggests that you want a little-endian read, but this won't fetch a
little-endian value on a big-endian system.  Either do a proper
little-endian fetch byte by byte, as the DWARF code already does, or
clarify the function name.  I don't know what you actually need.

> +/* Function that produce a crc32 value for input buffer.  */

Let's move the CRC32 stuff into a different file.  Add a comment
explaining how the table was generated.

> +  static const unsigned long crc32_table[256]

Seems like this should be uint32_t.  In any case not `unsigned long`,
which is 64 bits.  In general the CRC code seems to use `unsigned
long` where I would expect `uint32_t`.

> +  unsigned char buffer[8 * 1024];

This code is called from signal handlers and on threads; this array is
too long to put on the stack.  Instead of looping and calling read
like this, use backtrace_get_view.

> +/* Get length of the path.  */
> +
> +static int
> +pathlen (const char *buffer)

This function doesn't seem to get the length of the path, it seems to
get the length of the base name.

> +  if (offset >= section_size)
> +    return 0;
> +  crc32_debug_link = getl32 (debug_link + offset);

Seems like you should compare offset + 4 to section_size to avoid
running off the end.

> +      error_callback (data, "executable file is not ELF", 0);

This and other error messages no longer seem correct, as this function
is now used for files other than the executable file.

It doesn't seem like elf_get_section_by_name should call
process_elf_header, it seems like that will do unnecessary extra work.
For that matter elf_get_section_by_name shouldn't read the section
headers and names each time, we should only do that once.

> +static int
> +backtrace_readlink

This function should return type_of_file, and the enum should have an
error value.

+      if (buffer[0] == '/')

Use IS_ABSOLUTE_PATH.

> +  debug_descriptor
> +    = backtrace_open_debugfile (descriptor, filename, error_callback,
> + data, state);

If you're going to call this from fileline.c, then the function needs
to be defined in pecoff.c also.  But calling it in fileline.c doesn't
seem right; why doesn't backtrace_initialize call it?

Ian