From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92018 invoked by alias); 13 Sep 2019 21:03:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 92009 invoked by uid 89); 13 Sep 2019 21:03:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=aaron, tromeycom, UD:tromey.com, tromey.com X-HELO: gateway36.websitewelcome.com Received: from gateway36.websitewelcome.com (HELO gateway36.websitewelcome.com) (192.185.196.23) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 Sep 2019 21:03:53 +0000 Received: from cm12.websitewelcome.com (cm12.websitewelcome.com [100.42.49.8]) by gateway36.websitewelcome.com (Postfix) with ESMTP id 15B8C4118152B for ; Fri, 13 Sep 2019 15:30:42 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id 8sjPiGtz0iQer8sjPiXCyK; Fri, 13 Sep 2019 16:03:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=1TbR1EXollgbi1vzGbnrdiptusdwVHVe2XpKaDwreYY=; b=p4eTSdsTpwZRqDbIMvIoJyzscD xAyrNX0oGeJV/45ejp0QR7C98DyidPrThlbkfx8AY4c6X0cN8fEfqnTmmO0MxuOKFSTyW+LBWMMDl ppi40wVE6xZ+y2qdgx2mgzRLw; Received: from [198.168.27.218] (port=55122 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92) (envelope-from ) id 1i8sjP-000BPm-IB; Fri, 13 Sep 2019 16:03:51 -0500 From: Tom Tromey To: Aaron Merey Cc: gdb-patches@sourceware.org Subject: Re: [RFC PATCH] Support debuginfo and source file fetching via debuginfo server References: <20190820202809.25367-1-amerey@redhat.com> Date: Fri, 13 Sep 2019 21:03:00 -0000 In-Reply-To: <20190820202809.25367-1-amerey@redhat.com> (Aaron Merey's message of "Tue, 20 Aug 2019 16:28:09 -0400") Message-ID: <8736gz3o3t.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-09/txt/msg00243.txt.bz2 >>>>> "Aaron" == Aaron Merey writes: Aaron> Debuginfo server is a lightweight web service that indexes debuginfo Aaron> and source files by build-id and serves them over HTTP. Thank you for the patch. I think the idea is fine for gdb, so all that's left is some nits in the patch. Aaron> +#if HAVE_LIBDBGSERVER Aaron> + else Aaron> + { Aaron> + const struct bfd_build_id *build_id; Aaron> + char *debugfile_path; Aaron> + Aaron> + build_id = build_id_bfd_get (objfile->obfd); Aaron> + int fd = dbgserver_find_debuginfo (build_id->data, Aaron> + build_id->size, Aaron> + &debugfile_path); I was wondering what the fd represents. If it's open on the file, can we simply reuse it rather than trying to reopen the file? Instead of "int", using scoped_fd would be better. Aaron> + symbol_file_add_separate(debug_bfd.get (), debugfile_path, GNU style is a space before parens - there are a few instances. Aaron> + free(debugfile_path); xfree instead of free. Aaron> +#if HAVE_LIBDBGSERVER Aaron> + if (fd.get() < 0) Aaron> + { Aaron> + if (SYMTAB_COMPUNIT(s) != NULL) Aaron> + { Aaron> + const struct bfd_build_id *build_id; Aaron> + const objfile *ofp = COMPUNIT_OBJFILE (SYMTAB_COMPUNIT (s)); Aaron> + Aaron> + /* prefix the comp_dir to relative file names */ Aaron> + const char* dirname = SYMTAB_DIRNAME (s); Aaron> + int suffname_len = strlen(dirname) + strlen(s->filename) + 2; Aaron> + char *suffname = (char *) alloca(suffname_len); I think it's better to just use std::string for this kind of thing. Probably this area will need some refactoring since other patches have touched this. Aaron> + char *name_in_cache; Aaron> + int dbgsrv_rc = dbgserver_find_source (build_id->data, Aaron> + build_id->size, Aaron> + suffname, Aaron> + &name_in_cache); Aaron> + if (dbgsrv_rc >= 0) Aaron> + { Aaron> + fullname.reset (xstrdup(name_in_cache)); Aaron> + free (name_in_cache); It seems like you could just use fullname.reset (name_in_cache); here. Aaron> + } Aaron> + else if (dbgsrv_rc == -ENOSYS) Aaron> + { Aaron> + /* -ENOSYS indicates that libdbgserver could not find Aaron> + any dbgserver URLs to query due to $DBGSERVER_URLS Aaron> + not being defined. Replace -ENOSYS with -ENOENT so Aaron> + that users who have not configured dbgserver see the Aaron> + usual error message when a source file cannot be found. */ Aaron> + dbgsrv_rc = -ENOENT; This assignment doesn't seem useful here. Tom