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 Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov
<d.khalikov@partner.samsung.com> wrote:
> Hello everyone, i have a patch for this issue.
>
> List of implemented functionality:
>
> 1.Reading .gnu_debuglink section from ELF file:
>  a. Reading name of debug info file.
>  b. Verifying crc32 sum.
>
> 2. Searching for separate debug info file from paths:
>  a. /usr/lib/debug/path/to/executable
>  b. /path/to/executable
>  c. /path/to/executable/.debug
>
> Assumed that debug info file generated by objcopy from binutils.
>
> objcopy --only-keep-debug foo foo.debug
> strip -g foo
> objcopy --add-gnu-debuglink=foo.debug foo

> +clean_separate:glinktest.debug
> + rm -f glinktest.debug
> +
> +separate: glinktest
> + if test -n "$(OBJCOPY)" &&  test -n "$(STRIP)";  \
> + then \
> + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\
> + $(STRIP) glinktest;\
> + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\
> + fi;

As far as I know "separate" is not a special thing in automake.  We
should always run (and clean) this test if the necessary objcopy
option is available.

> +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h

Missing '$'  in "$(INCDIR)".

> +/* Return 1 if header is valid and -1 on fail */

This comment does not explain what the function actually does.

> +/* Return the pointer to char array with data from .gnudebuglink section inside.  */

Line too long, we use 80 column lines.

> +unsigned char *
> +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor,
> +   backtrace_error_callback error_callback, void *data,
> +   int exe, int *gnulink_data_len_out)

This should be static.  I see that you are calling it elsewhere, but
it doesn't make sense to call an "elf" function outside of elf.c.
This library is used on non-ELF systems.

> +  /* Look for for the .gnu_debuglink section  */

Period at end of sentence.

>    /* To translate PC to file/line when using DWARF, we need to find
> -     the .debug_info and .debug_line sections.  */
> +   the .debug_info and .debug_line sections.  */

Why change the indentation like this?  I think the original was correct.

> -      descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
> -   pd->data, &does_not_exist);
> +      descriptor
> + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data,
> +    &debugfile_does_not_exist, pd->state, 0);
> +      if (descriptor < 0)
> +  descriptor = backtrace_open (info->dlpi_name, pd->error_callback,
> +       pd->data, &does_not_exist);

This seems like unnecessary work.  Shouldn't we only try to open the
debug file if we find a .gnu_debuglink section?

> +/*Just a simple test copied from btest.c, but in this case we don't have debug
> + * info in the executable and test should verify that we can read debug info
> + * from separate file. See Makefile check-TESTS target. */

No leading '*' on subsequent lines of multi-line comments, see existing code.

I'm not sure I see the point of glinktest.c.  Why don't we just use btest.c?

> +#define MAX_PATH_LEN 4096 /*  from linux/limits.h   */

This library works on systems other than GNU/Linux.  We should
dynamically allocate the buffer.

> +  while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1))))

while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1]))

or just call basename.  I'm not sure why you bother with the count
variable; doesn't full_filename_len - count exactly == len?

> +  sign_byte = 0x00;
> +  count = 0;
> +  while (*(buffer + count) != sign_byte && size > count)
> +    ++count;
> +  return count;

This looks like `return strnlen(buffer, size)`.

Ian