From: "Ian Lance Taylor via gcc-patches" <gcc-patches@gcc.gnu.org>
To: Denis Khalikov <d.khalikov@partner.samsung.com>
Cc: gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Date: Tue, 14 Mar 2017 16:26:00 -0000 [thread overview]
Message-ID: <CAKOQZ8wtL2LrXDvsFQ86vLf+hsnexFjEK3i8w6zWLi15jXrLTQ@mail.gmail.com> (raw)
In-Reply-To: <078366ad-3b20-93ac-82cc-ed88e98bb15b@partner.samsung.com>
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
>>
>>
>>
>
next prev parent reply other threads:[~2017-03-14 16:26 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20170313171637eucas1p2d70c4ed7cbd6d088c8c58dc76e1ef722@eucas1p2.samsung.com>
2017-03-13 17:16 ` Denis Khalikov
2017-03-14 8:27 ` Richard Biener
2017-03-14 9:22 ` Matthias Klose
2017-03-14 10:21 ` Denis Khalikov
2017-03-14 13:21 ` Ian Lance Taylor via gcc-patches
2017-03-14 13:46 ` Denis Khalikov
2017-03-14 13:49 ` Ian Lance Taylor via gcc-patches
2017-03-14 14:30 ` Denis Khalikov
2017-03-14 16:26 ` Ian Lance Taylor via gcc-patches [this message]
2017-03-14 17:44 ` Denis Khalikov
2017-03-22 15:29 ` Denis Khalikov
2017-04-12 22:08 ` Jeff Law
2017-05-18 14:23 ` Denis Khalikov
[not found] <CGME20170616153942eucas1p1945271f893265484bbb3991a368bcd92@eucas1p1.samsung.com>
2017-06-16 15:39 ` Denis Khalikov
2017-06-18 14:09 ` Matthias Klose
2017-06-19 8:48 ` Denis Khalikov
2017-06-28 23:59 ` Ian Lance Taylor via gcc-patches
2017-07-01 21:38 ` Denis Khalikov
2017-07-29 20:42 ` Denis Khalikov
2017-09-10 21:11 ` Ian Lance Taylor via gcc-patches
2017-09-11 10:06 ` Denis Khalikov
2017-09-12 14:06 ` Ian Lance Taylor via gcc-patches
2017-09-20 8:54 ` Maxim Ostapenko
2017-09-20 12:33 ` Ian Lance Taylor via gcc-patches
2017-09-20 21:09 ` Ian Lance Taylor via gcc-patches
2017-10-02 11:12 ` Martin Liška
2017-10-02 11:32 ` Jakub Jelinek
2017-10-02 14:01 ` Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKOQZ8wtL2LrXDvsFQ86vLf+hsnexFjEK3i8w6zWLi15jXrLTQ@mail.gmail.com \
--to=gcc-patches@gcc.gnu.org \
--cc=d.khalikov@partner.samsung.com \
--cc=iant@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).