From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47953 invoked by alias); 14 Mar 2017 17:44:44 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 47909 invoked by uid 89); 14 Mar 2017 17:44:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=HContent-type:plain, H*MI:sk:2d27607 X-HELO: mailout4.w1.samsung.com Received: from mailout4.w1.samsung.com (HELO mailout4.w1.samsung.com) (210.118.77.14) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Mar 2017 17:44:37 +0000 Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OMT00K51FY9Q300@mailout4.w1.samsung.com> for gcc-patches@gcc.gnu.org; Tue, 14 Mar 2017 17:44:33 +0000 (GMT) Received: from eusmges2.samsung.com (unknown [203.254.199.241]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20170314174432eucas1p2dc6ef3e033b4cd09a6eeb11abee6f689~r0Jyf8uJx0271702717eucas1p25; Tue, 14 Mar 2017 17:44:32 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges2.samsung.com (EUCPMTA) with SMTP id 77.1D.30614.00C28C85; Tue, 14 Mar 2017 17:44:32 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20170314174432eucas1p2cae8109492de5378cb692e0db909b1bf~r0JxuK2du0611906119eucas1p2t; Tue, 14 Mar 2017 17:44:32 +0000 (GMT) Received: from eusync3.samsung.com ( [203.254.199.213]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 1D.C1.06687.56C28C85; Tue, 14 Mar 2017 17:46:13 +0000 (GMT) Received: from [106.109.129.179] by eusync3.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0OMT00186FY2ZI50@eusync3.samsung.com>; Tue, 14 Mar 2017 17:44:32 +0000 (GMT) Subject: Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace To: Ian Lance Taylor Cc: gcc-patches From: Denis Khalikov Message-id: <28c3da40-7f7a-3940-a631-9ae0d9e999e1@partner.samsung.com> Date: Tue, 14 Mar 2017 17:44:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170314174432eucas1p2cae8109492de5378cb692e0db909b1bf X-Msg-Generator: CA X-Sender-IP: 182.198.249.179 X-Local-Sender: =?UTF-8?B?RGVuaXMgS2hhbGlrb3YbU1JSLVNXIFRvb2xzIExhYhvsgrw=?= =?UTF-8?B?7ISx7KCE7J6QG0VuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?RGVuaXMgS2hhbGlrb3YbU1JSLVNXIFRvb2xzIExhYhtTYW1z?= =?UTF-8?B?dW5nIEVsZWN0cm9uaWNzG0VuZ2luZWVy?= X-Sender-Code: =?UTF-8?B?QzEwG0NJU0hRG0MxMEdEMDFHRDAxMDE1Nw==?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170313171637eucas1p2d70c4ed7cbd6d088c8c58dc76e1ef722 X-RootMTR: 20170313171637eucas1p2d70c4ed7cbd6d088c8c58dc76e1ef722 References: <2d276071-3570-0a00-d046-29d5b84f2b7f@partner.samsung.com> <078366ad-3b20-93ac-82cc-ed88e98bb15b@partner.samsung.com> X-SW-Source: 2017-03/txt/msg00761.txt.bz2 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 > 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 >>> 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 >>> >>> >>> >> > > >