On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov <d.khali...@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
