From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26810 invoked by alias); 16 Jan 2019 17:15:42 -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 26034 invoked by uid 89); 16 Jan 2019 17:15:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.9 required=5.0 tests=BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_2,RCVD_IN_DNSWL_NONE,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.2 spammy= X-HELO: mail-yb1-f194.google.com Received: from mail-yb1-f194.google.com (HELO mail-yb1-f194.google.com) (209.85.219.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Jan 2019 17:15:15 +0000 Received: by mail-yb1-f194.google.com with SMTP id e1so2682338ybn.11 for ; Wed, 16 Jan 2019 09:15:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=i+9qU9N1GOSxXabfVjNs/6hXwMwAALiPx5ZWhus6XWU=; b=n9yArPTC+t0TvYxe2gCExzhdtkc/WuoiN40ogoGs+m3x2zzoOUTCRjCUBHZ0VEXHZ5 SMSeec8kvNCFDA7JmL9X3534tCn9kOpdPuyV3twH1cdmGLF9sLMS4hQRwCHms/8rbLkI Dn1xn3pjI+qwVkXXW+IkP2NM7LE+WDdsKx35lZ1uBHgFHkDihgZTEkhPI+NwiPXLbrlO PDTHv7XabWI/w2zfXkPp9SuncF1X9dQmtQFWhMbrqS9u56hzw/+DEPMjzRUmez3iZFCQ bJCRTliWd7KSqAH8rLeItV+GSe4NHhux39oBrHN9ps3SEO/LUYxOmfjDVsDFIshYyxLc 91jw== MIME-Version: 1.0 References: <20181211101411.7067-1-tdevries@suse.de> <20181211101411.7067-2-tdevries@suse.de> <1f6b644a-2cde-b101-2435-f11fa24820f4@suse.de> In-Reply-To: <1f6b644a-2cde-b101-2435-f11fa24820f4@suse.de> From: "Ian Lance Taylor via gcc-patches" Reply-To: Ian Lance Taylor Date: Wed, 16 Jan 2019 17:15:00 -0000 Message-ID: Subject: Re: [PATCH 1/9] [libbacktrace] Read .gnu_debugaltlink To: Tom de Vries Cc: gcc-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00932.txt.bz2 On Wed, Jan 16, 2019 at 8:26 AM Tom de Vries wrote: > > On 16-01-19 01:56, Ian Lance Taylor wrote: > > On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries wrote: > >> > >> Read the elf file pointed at by the .gnu_debugaltlink section, and verify that > >> the build id matches. > >> > >> 2018-11-11 Tom de Vries > >> > >> * elf.c (elf_add): Add and handle with_buildid_data and > >> with_buildid_size parameters. Handle .gnu_debugaltlink section. > >> (phdr_callback, backtrace_initialize): Add arguments to elf_add calls. > >> --- > > > > > > > > @@ -2899,6 +2918,27 @@ elf_add (struct backtrace_state *state, const > > char *filename, int descriptor, > >> } > >> } > >> > >> + if (!debugaltlink_view_valid > >> + && strcmp (name, ".gnu_debugaltlink") == 0) > >> + { > >> + const char *debugaltlink_data; > >> + size_t debugaltlink_name_len; > >> + > >> + if (!backtrace_get_view (state, descriptor, shdr->sh_offset, > >> + shdr->sh_size, error_callback, data, > >> + &debugaltlink_view)) > >> + goto fail; > >> + > >> + debugaltlink_view_valid = 1; > >> + debugaltlink_data = (const char *) debugaltlink_view.data; > >> + debugaltlink_name = debugaltlink_data; > >> + debugaltlink_name_len = strnlen (debugaltlink_data, shdr->sh_size); > >> + debugaltlink_buildid_data = (debugaltlink_data > >> + + debugaltlink_name_len > >> + + 1); > >> + debugaltlink_buildid_size = shdr->sh_size - debugaltlink_name_len - 1; > >> + } > >> + > > > > This doesn't look quite right. debugaltlink_buildid_size is unsigned. > > If there is some misunderstanding of the format it's possible for > > strnlen to return shdr->sh_size. If it does, > > debugaltlink_buildid_size will be set to a very large value. > > > > I see, thanks for finding that. > > Fixed like this: > ... > debugaltlink_name_len = strnlen (debugaltlink_data, shdr->sh_size); > if (debugaltlink_name_len < shdr->sh_size) > { > /* Include terminating zero. */ > debugaltlink_name_len =+ 1; > > debugaltlink_buildid_data > = debugaltlink_data + debugaltlink_name_len; > debugaltlink_buildid_size > = shdr->sh_size - debugaltlink_name_len; > } > ... > > >> + if (debugaltlink_name != NULL) > >> + { > >> + int d; > >> + > >> + d = elf_open_debugfile_by_debuglink (state, filename, debugaltlink_name, > >> + 0, error_callback, data); > >> + if (d >= 0) > >> + { > >> + int ret; > >> + > >> + ret = elf_add (state, filename, d, base_address, error_callback, data, > >> + fileline_fn, found_sym, found_dwarf, 0, 1, > >> + debugaltlink_buildid_data, debugaltlink_buildid_size); > >> + backtrace_release_view (state, &debugaltlink_view, error_callback, > >> + data); > >> + debugaltlink_view_valid = 0; > >> + if (ret < 0) > >> + { > >> + backtrace_close (d, error_callback, data); > >> + return ret; > >> + } > >> + } > >> + else > >> + { > >> + error_callback (data, > >> + "Could not open .gnu_debugaltlink", 0); > >> + /* Don't goto fail, but try continue without the info in the > >> + .gnu_debugaltlink. */ > >> + } > >> + } > > > > The strings passed to error_callback always start with a lowercase > > letter (unless they start with something like ELF) because the > > callback will most likely print them with some prefix. > > > > Fixed. > > > More seriously, we don't call error_callback in any cases that > > correspond to this. We just carry on. > > Indeed, I noticed that, and filed PR88244 - "[libbacktrace] Failure to > open .gnu_debuglink is silent". > > [ The scenario there is: an executable has a .gnu_debuglink, but the > file the .gnu_debuglink is pointing to is absent, because f.i. it has > been removed, or moved to a different location. If a user runs this > executable and a backtrace is triggered, the information relating to the > functions in the executable will be missing in the backtrace, but > there's no error explaining to the user why that information is missing. > Note: there is a default error "no debug info in ELF executable" in > elf_nodebug, but AFAIU this is not triggered if debug info for one of > the shared libraries is present. ] > > BTW, though in the code above an error_callback is called, we don't > error out, but do carry on afterwards (as the comment explicitly states). > > > Is there any reason to call > > error_callback here? > > A similar scenario: an executable has a .gnu_altdebuglink, but the file > the .gnu_debugaltlink is pointing to is absent, because f.i. it has been > removed, or moved to a different location. If a user runs this > executable and a backtrace is triggered, the information stored in the > .gnu_debugaltlink file will be missing in the backtrace, but there's no > error explaining to the user why that information is missing. The problem is that libbacktrace is often run in cases where it needs to present best effort information. But the error_callback is currently only called for cases where it can't present any information at all. You are suggesting that we call it and then carry on. But currently we don't do that (as far as I know); we call it and then fail. For example, in libgo, if the error_callback function is called, the program will print the error and then crash. So while I understand your desire to present a warning message to the user about a missing file, I don't think we should use the existing error_callback mechanism to do so. I think we'll need to introduce some way to let error_callback know that this is a warning rather than an error. Unfortunately changing the actual API at this point would be somewhat painful. Still, we should either do that, or introduce some convention like "if MSG starts with a '-' then this is just a warning." Any thoughts? Ian