From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id PQtzI1WyBGbo0hgAWB0awg (envelope-from ) for ; Wed, 27 Mar 2024 19:57:09 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ZnWynf8Z; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 7D6451E0C0; Wed, 27 Mar 2024 19:57:09 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 5B0621E08C for ; Wed, 27 Mar 2024 19:57:07 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D1D6F3858286 for ; Wed, 27 Mar 2024 23:57:06 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 89B8B3858D20 for ; Wed, 27 Mar 2024 23:56:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 89B8B3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 89B8B3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711583806; cv=none; b=qTtWuY6hSCDbUqq8Q+Ct78oFejGnkJh+kqaYAxB3EpBMnTdA+zGkBHwgMYPJesZd32BzVG5PJUOntmUe3QX70QKtrzW0euVRPmc2awnGKveyx0ni/XphRxNc1DhSRXHc90duadC+cY1Ckk5rfWbgVcr/NAHbWuqCion86JlV91w= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711583806; c=relaxed/simple; bh=yTv4qxr/7Vt3f6K7r/FFisE4GPlQzoCqoO9PYqEB5EI=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=M9QdQebfc+kkyXRg3xBYgiaF7Z/fRkTbNNNJc5PmzQ8/pA6YZWt99fc6HCwYaXTWR//2JfF7pSBkhsYdopcY02NS4nuFRXicJzjE+kPrRZNtyrAPD8e7JIEZV4duDwJMdpoTXVixKMJnwR9aMNrzkLKB+ttqzLVGlB9mBy4dT8Q= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1711583804; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=m6axVhxQ4ALk/GgW6h04yJQdyXdtemgcDu7cUJWQzKI=; b=ZnWynf8ZXxPdlMukbFr10x8iD9XTICupkKOUgeA9nfMkHm1U3WzCm3mB1aWMgdJWMVJ3Kp JcnvDeDiJ9f7+jSJ+dko13RhN83rk5cuJWJrjm8nB7xk1Q1AaZbYZ61Y6TTZOhAvOXelb+ i1o4ETx7I/vEqYumeofrCjzLgVTWgOc= Received: from mail-pg1-f200.google.com (mail-pg1-f200.google.com [209.85.215.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-201-sRUsEw4TM6mDAEjFfIs8Yg-1; Wed, 27 Mar 2024 19:56:43 -0400 X-MC-Unique: sRUsEw4TM6mDAEjFfIs8Yg-1 Received: by mail-pg1-f200.google.com with SMTP id 41be03b00d2f7-5f034b4db5bso172093a12.0 for ; Wed, 27 Mar 2024 16:56:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711583802; x=1712188602; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=m6axVhxQ4ALk/GgW6h04yJQdyXdtemgcDu7cUJWQzKI=; b=oqhw9AMF5naclOBcsfhLzhLDRtjPI1LY7BAsZ96b1Ls7IBFiTMT+eRZXsZFjdwC4xS HMc7Tx0z11P8AOZGdtZ4V5Pwa7uvWncoVRwoLzQvpXeg+t+LCPapVC8zJI5hGtstgRWU BE2Y29TmQ3plp8yn6OsOdSB6IzC/OCuAR/JN1e4O+jGix9/k74kHnR+ZvsbLKM74uNan e1XI+MRZ+HMWxfkpHGCBMm9UDvJ8MF4Bka8YGG636cVgogr0sUKgXjCwa42F6uFQSpXB 9b+eo5z64O2VF7iClvTjNGMztxB0hz/O4vQ2GnlHrbpPv6BlH2oaQIkyHbB3eZr4E6ib 7v9g== X-Gm-Message-State: AOJu0Yy+wPxRlhTGZQcxRIq8AydnHYXOQPjNCx7iHMazJNuZ9OfETOTc H/nj1JIA3T2uAP9WZmCN6IkamTYVXXy3MYwlad/Qsl6jDytisvHKDhpHX4suA5qyxVIgFgeJU5x Jc59lLGv4KJuicPjXaaLPEZFza26GkkzqPVPWgVrLQwkxrznUM0guB1R+Ld5A2E3g3X/HUA/IMp gJzFlGunXgrofcNbHXtx845I/1jn/tjLSmMOzDYCaK3w4= X-Received: by 2002:a05:6a21:1693:b0:1a5:6d24:90ac with SMTP id np19-20020a056a21169300b001a56d2490acmr302623pzb.56.1711583801585; Wed, 27 Mar 2024 16:56:41 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEOdM7LV9A019HoKWMGeZ6JW1EMjCFkpShrzyqNvbv1ENeTpMfK8ABjUUrqbui4Lt98cIqbfZIVr8hMFOtlD4M= X-Received: by 2002:a05:6a21:1693:b0:1a5:6d24:90ac with SMTP id np19-20020a056a21169300b001a56d2490acmr302605pzb.56.1711583801100; Wed, 27 Mar 2024 16:56:41 -0700 (PDT) MIME-Version: 1.0 References: <87jzo664gi.fsf@redhat.com> <20240119051213.2502965-1-amerey@redhat.com> <87plvg3pro.fsf@redhat.com> In-Reply-To: <87plvg3pro.fsf@redhat.com> From: Aaron Merey Date: Wed, 27 Mar 2024 19:56:30 -0400 Message-ID: Subject: Re: [PATCH 3/4 v5] gdb/debuginfod: Support on-demand debuginfo downloading To: Andrew Burgess Cc: gdb-patches@sourceware.org, Thiago Jung Bauermann X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hi Andrew, On Wed, Mar 27, 2024 at 6:58=E2=80=AFAM Andrew Burgess wrote: > > +gdb::array_view > > +index_cache::lookup_gdb_index_debuginfod (const char *index_path, > > + std::unique_ptr *resource) > > +{ > > + try > > + { > > + /* Try to map that file. */ > > + index_cache_resource_mmap *mmap_resource > > + =3D new index_cache_resource_mmap (index_path); > > + > > + /* Hand the resource to the caller. */ > > + resource->reset (mmap_resource); > > + > > + return gdb::array_view > > + ((const gdb_byte *) mmap_resource->mapping.get (), > > + mmap_resource->mapping.size ()); > > + } > > + catch (const gdb_exception_error &except) > > + { > > + warning (_("Unable to read %s: %s"), index_path, except.what ())= ; > > + } > > + > > + return {}; > > +} > > I wonder if the core of this function should be merged with > lookup_gdb_index? The core functionality should be identical, but > there's already a little divergence. > > I'd be tempted to just rename lookup_gdb_index_debuginfod to > lookup_gdb_index and have it be an overload, one that finds the index in > a named file, and the other that takes a build-id, creates a filename, > and then calls your new version. Ok I'll change this. > > diff --git a/gdb/dwarf2/public.h b/gdb/dwarf2/public.h > > index efb754b5fe8..4d3776465c3 100644 > > --- a/gdb/dwarf2/public.h > > +++ b/gdb/dwarf2/public.h > > @@ -44,4 +44,11 @@ extern bool dwarf2_initialize_objfile > > > > extern void dwarf2_build_frame_info (struct objfile *); > > > > +/* Query debuginfod for the .gdb_index associated with OBJFILE. If > > + successful, create an objfile to hold the .gdb_index information > > + and act as a placeholder until the full debuginfo needs to be > > + downloaded. */ > > + > > +extern bool dwarf2_has_separate_index (struct objfile *); > > Unless I'm missing something then this comment is not correct. This > function doesn't create an objfile to held the .gdb_index does it? My mistake, that comment should instead say something like "associate the .gdb_index information with OBJFILE until the full debuginfo is downloaded". In earlier revisions of the patch, this function did create a new objfile but the comment wasn't updated when the function changed. > > +void > > +dwarf2_gdb_index::expand_all_symtabs (struct objfile *objfile) > > +{ > > + try > > + { > > + dwarf2_base_index_functions::expand_all_symtabs (objfile); > > + } > > + catch (const gdb_exception &e) > > + { > > + if ((objfile->flags & OBJF_DOWNLOAD_DEFERRED) =3D=3D 0) > > + exception_print (gdb_stderr, e); > > + else > > + read_full_dwarf_from_debuginfod (objfile, this); > > This is the part that I don't really understand about this patch, why is > the download and read of the full dwarf done in the exception path? > > How do we know that the exception that was thrown indicates we should go > fetch the dwarf and not that some other error has occurred? > > I couldn't find any obvious path from which > read_full_dwarf_from_debuginfod might throw an exception, but if it does > then I guess we're not going to get the behaviour we expect. > > I guess my expectation, going into this patch, was that you would have > generalised the index checking code in some way so that when we check > the index and find a match, instead of reading the full DWARF, we'd > download the info, and then read the full DWARF. > > I'm not saying that what you have is wrong, or that it needs to change, > just that I found this approach surprising, and I don't really > understand the design -- but I'm open to being convinced. Performing the download and read in the exception path helps decouple dwarf2_base_index_functions and dwarf2_gdb_index functions from the lazy debuginfo logic. This design also facilitates an early return from these functions so that the next objfile in the progspace objfile list can be searched for a match. If the deferred download was successful, this next objfile holds the actual DWARF that gdb is looking fo= r. It is possible that some unrelated error during the initial index search triggers an unnecessary debuginfo download. FWIW I have not come across such a case. If this turned out to be an issue later on, we could for instance introduce a special exception type indicating whether or not gdb should attempt to download the deferred debuginfo. In any case, the chance of an unnecessary download with this patch is vastly lower than the current approach of immediately downloading all debuginfo. Aaron