From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68795 invoked by alias); 14 Mar 2017 14:30:37 -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 68626 invoked by uid 89); 14 Mar 2017 14:30:24 -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=Period, HContent-type:plain, H*MI:sk:2d27607 X-HELO: mailout1.w1.samsung.com Received: from mailout1.w1.samsung.com (HELO mailout1.w1.samsung.com) (210.118.77.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Mar 2017 14:30:22 +0000 Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OMT009NZ6YI3V60@mailout1.w1.samsung.com> for gcc-patches@gcc.gnu.org; Tue, 14 Mar 2017 14:30:18 +0000 (GMT) Received: from eusmges3.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20170314143018eucas1p2339aa546489ec5c008d7ff78c51fd0f2~rxgMUFBZW0283902839eucas1p2w; Tue, 14 Mar 2017 14:30:18 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges3.samsung.com (EUCPMTA) with SMTP id F7.BF.09557.A7EF7C85; Tue, 14 Mar 2017 14:30:18 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20170314143017eucas1p18085b7c3b6dbfc27472f276453db10a8~rxgLJ4n7I0721307213eucas1p1h; Tue, 14 Mar 2017 14:30:17 +0000 (GMT) Received: from eusync1.samsung.com ( [203.254.199.211]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id C9.D0.10233.58EF7C85; Tue, 14 Mar 2017 14:30:29 +0000 (GMT) Received: from [106.109.129.179] by eusync1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0OMT000IX6YDJ830@eusync1.samsung.com>; Tue, 14 Mar 2017 14:30:16 +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: <078366ad-3b20-93ac-82cc-ed88e98bb15b@partner.samsung.com> Date: Tue, 14 Mar 2017 14:30: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: 20170314143017eucas1p18085b7c3b6dbfc27472f276453db10a8 X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 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> X-SW-Source: 2017-03/txt/msg00743.txt.bz2 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 . 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 > > >