On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
<d.khali...@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
