On 06/29/2017 02:59 AM, Ian Lance Taylor wrote:
> 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
>

Hello Ian,
thanks for review.
I've updated the patch, can you please take a look.
Thanks.