From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37828 invoked by alias); 14 Mar 2017 16:26:24 -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 37777 invoked by uid 89); 14 Mar 2017 16:26:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-yw0-f181.google.com Received: from mail-yw0-f181.google.com (HELO mail-yw0-f181.google.com) (209.85.161.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Mar 2017 16:26:19 +0000 Received: by mail-yw0-f181.google.com with SMTP id p77so82501553ywg.1 for ; Tue, 14 Mar 2017 09:26:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Ia2xDNdcEPBZXzw1Dfs4NwZRBwbMhJ+7Ukk3/LXoAPY=; b=ipz39EbbkBjf/Zz01rpOQJ7q4TXbBFUW4m2O2wIMRz3YB0mnhNn2XNPSCNbs6rTfKf XjKNNfwSKD6PLcKBtiNREKL+BxMcuwHFwUa7Fw213UjMzLTAY8f5n1oJqmz1ePQ6rp9K z9qhWaVYcxo8ulpbihcGOHNo5Uz4wD1OAxpiHWs0gRpRirqu+Shpx8Izfkx0U/qWkE9I +wSnSHGzTekm0xB2oLoBu24OJOeCgr6SgQXT9Id/HcOJE8vpkHODY60n6HWmVAKlQZzN aU1PnkCgC4GWmMx3Y9WP5YxF0/oZSJUE/FZgW3Wks3cAopB8pLskJr1SGmfDbL1EvjSP 8+NQ== X-Gm-Message-State: AMke39k1lFlEspnGOJ2ltnOXp6MNf+/4VAGBgqpk3qbz/ZhGy5BFWzPlrr9yU4uTQlbPiRzovGM1LiS5B49dkPb6 X-Received: by 10.129.85.137 with SMTP id j131mr22865383ywb.80.1489508778369; Tue, 14 Mar 2017 09:26:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.129.88.193 with HTTP; Tue, 14 Mar 2017 09:26:17 -0700 (PDT) In-Reply-To: <078366ad-3b20-93ac-82cc-ed88e98bb15b@partner.samsung.com> References: <2d276071-3570-0a00-d046-29d5b84f2b7f@partner.samsung.com> <078366ad-3b20-93ac-82cc-ed88e98bb15b@partner.samsung.com> From: "Ian Lance Taylor via gcc-patches" Reply-To: Ian Lance Taylor Date: Tue, 14 Mar 2017 16:26:00 -0000 Message-ID: Subject: Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace To: Denis Khalikov Cc: gcc-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00756.txt.bz2 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 >> >> >> >