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

[Denis Khalikov <d.khalikov@partner.samsung.com>]

Thanks for answer, i got it now.
I will also delete all readlink code. Looks like is no reason
to use it instead just one call of realpath(char*,char*). Binutils
using realpath in the same cases.

On 03/14/2017 07:26 PM, Ian Lance Taylor wrote:
> On Tue, Mar 14, 2017 at 7:30 AM, Denis Khalikov
> <d.khalikov@partner.samsung.com> wrote:
>> Thanks for review, got all of my mistakes, except one.
>>
>>
>>> -      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?
>>
>> Should i move code below
>>
>>   /* check if debug section does exist */
>>  debug_link_data
>>     = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe,
>> &debug_link_data_len);
>>
>> from backtrace_open_debugfile .
>
> As far as I know all the debuglink code is ELF-specific.  I would do
> it all in elf.c.  While reading the sections of the executable, look
> for a debuglink section, and use it if present.  Keep the readlink
> code in posix.c, I suppose.  Apologies if this doesn't make sense.
>
> Ian
>
>
>
>> On 03/14/2017 04:49 PM, Ian Lance Taylor wrote:
>>>
>>> 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
>>>
>>>
>>>
>>
>
>
>