From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x529.google.com (mail-pg1-x529.google.com [IPv6:2607:f8b0:4864:20::529]) by sourceware.org (Postfix) with ESMTPS id 6FE743858D32 for ; Tue, 26 Dec 2023 18:35:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6FE743858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6FE743858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::529 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703615732; cv=none; b=DQnjgR5fdYqbupOSR+PqiiZOfqWlKctuH3IFj83FmgINaYUcWMywTqY2r3UqHBhqFJY0pHJ/QUEWd4kSxGCucuCEr1RMJ2HYCVFxho3WZnmLnQVbT49TyZ0G8+FIzWxOHjeMPOPlMch7sDkdIjTEfVfnPDd0saMlUgzr8c4MOvI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703615732; c=relaxed/simple; bh=Bl1Qbs8vQ1/1RoDAn6akscx4YqeW+7aG4CaiRfzkexI=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=qurwj+tW0LtXzzDWtV/ivg4A8NC8S4QrArf5NDgSj35EHAA5lOwompgo0vdN/8y6xdzO4j4zHaL0K/OmSXc5GYN4+rb/s37IYEhFFU9Vh7fbUyKGAFymblcNmvWKHdOzifLbuA+rinAsmAO36ieyZlNZQlfZLH91GvP54mbARSk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pg1-x529.google.com with SMTP id 41be03b00d2f7-5c229dabbb6so1192346a12.0 for ; Tue, 26 Dec 2023 10:35:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1703615729; x=1704220529; darn=sourceware.org; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:from:to:cc:subject:date:message-id:reply-to; bh=BJ1fPlVraIV99b6A4vZchqs2UjBL7fMSwTEkNlYh1IA=; b=tR1bslmPW68TTe3gyo+jRHZF4g9xZhKkW6qDV8u6nPWlsmNXLr9EZlfOJD6aAQNMXQ uk0qMG3k+nPK9SL94EYzH0bwH08AgHQ2J0U1L1HopqZzaVPmCmSzTISprRvubWRbBhM1 s2EsJS1t9krzbPp67v+KWo3ef/1Owk3SibSEzl4g/QsGpsYTUBOsltPYIXI/UrruK3f2 ilp7CdyaB9oU9eOR+EXAY+1nKXZL82ETi1X++MZ2CKXnCVpNmlRQMYUIjXnUuBzLF6Nr zqYzpWjZ/E3F0+zMfZC26nhyCOGz1ifJwM8SUJAK5Q/yCRHS4dz1UU4oOlw8EIDPj156 UHWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703615729; x=1704220529; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=BJ1fPlVraIV99b6A4vZchqs2UjBL7fMSwTEkNlYh1IA=; b=BFxc4p2Kw79lyV/mXHi+FaBJTHLOEydLQS7LPboN8WOYz7Ekmu/nPykCZWQICGziq/ SjnWng1NLKPQpYugKvYm5xFKNiUIMmwrIt3aWs5L+dR427PWcatN+iTQdyDD/k6jZ6p8 pfAgS1yTdL/7FpBOsHLnens44dzFCNCYJKUeGGKyWsDlPk1S+I10wWU7RQESQ9v8Qv9v 5uY4tRJFJif3SyEYFCJaM/RJXW99XmUnPB+Fsd5jN3rkUl5GyqsvwVU0fovl3uj7dE7M 0HHs5ot+ZtScHlCQpmfqBOG98znvSuzeP06qZbTgygC47ycL5kDOUMl07e140uj6fDPA Gejg== X-Gm-Message-State: AOJu0YyfBkS48nT6SuOzoKzGT1cOwrn9MZ3HNPD3GFEp+DpIHfNeYduK 80YlfP5RRVlGXYre+tLhJt+XL5cTBWUW9Q== X-Google-Smtp-Source: AGHT+IEb1Km6vicSkkO0vzsqJz3lZrQW3NTbC3g0y0575bV8A3eM/NNO3sONJXWK2z1yMQfpMI4DiA== X-Received: by 2002:a17:902:ec81:b0:1d4:3fb3:c3f0 with SMTP id x1-20020a170902ec8100b001d43fb3c3f0mr1816689plg.9.1703615729333; Tue, 26 Dec 2023 10:35:29 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:b425:ec2d:a09a:544f]) by smtp.gmail.com with ESMTPSA id m5-20020a170902db8500b001d04c097d32sm10317471pld.270.2023.12.26.10.35.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Dec 2023 10:35:28 -0800 (PST) References: <20231028002008.1105723-1-amerey@redhat.com> <20231028002008.1105723-4-amerey@redhat.com> User-agent: mu4e 1.10.8; emacs 29.1 From: Thiago Jung Bauermann To: Aaron Merey Cc: aburgess@redhat.com, gdb-patches@sourceware.org Subject: Re: [PATCH 3/4 v4] gdb/debuginfod: Support on-demand debuginfo downloading In-reply-to: <20231028002008.1105723-4-amerey@redhat.com> Date: Tue, 26 Dec 2023 15:35:26 -0300 Message-ID: <87o7ecerrl.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Aaron Merey writes: > +void > +dwarf2_gdb_index::expand_matching_symbols > + (struct objfile *objfile, > + const lookup_name_info &lookup_name, > + domain_enum domain, > + int global, > + symbol_compare_ftype *ordered_compare) > +{ > + try > + { > + do_expand_matching_symbols (objfile, lookup_name, domain, > + global, ordered_compare); > + } > + catch (const gdb_exception &e) > + { > + if ((objfile->flags & OBJF_DOWNLOAD_DEFERRED) == 0) > + exception_print (gdb_stderr, e); > + else > + read_full_dwarf_from_debuginfod (objfile, this); > + return; "return" is redundant here. I suggest removing it. > + } > +} > +void > +read_full_dwarf_from_debuginfod (struct objfile *objfile, > + dwarf2_base_index_functions *fncs) > +{ > + gdb_assert (objfile->flags & OBJF_DOWNLOAD_DEFERRED); > + > + const struct bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ()); > + const char *filename; > + gdb_bfd_ref_ptr debug_bfd; > + gdb::unique_xmalloc_ptr symfile_path; > + scoped_fd fd; The "goto unset"s in this function make me wonder whether it is exception-safe. If SCOPE_EXIT is used at this point like so: SCOPE_EXIT { objfile->remove_deferred_status (); }; can it correctly replace the gotos? > + > + if (build_id == nullptr) > + goto unset; > + > + filename = bfd_get_filename (objfile->obfd.get ()); > + fd = debuginfod_debuginfo_query (build_id->data, build_id->size, > + filename, &symfile_path); > + if (fd.get () < 0) > + goto unset; > + > + /* Separate debuginfo successfully retrieved from server. */ > + debug_bfd = symfile_bfd_open (symfile_path.get ()); > + if (debug_bfd == nullptr > + || !build_id_verify (debug_bfd.get (), build_id->size, build_id->data)) > + { > + warning (_("File \"%s\" from debuginfod cannot be opened as bfd"), > + filename); > + goto unset; > + } > + > + /* Clear frame data so it can be recalculated using DWARF. */ > + dwarf2_clear_frame_data (objfile); > + > + /* This may also trigger a dwz download. */ > + symbol_file_add_separate (debug_bfd, symfile_path.get (), > + current_inferior ()->symfile_flags, objfile); > + > +unset: > + objfile->remove_deferred_status (); > +} > + > +/* See public.h. */ > + > +bool > +dwarf2_has_separate_index (struct objfile *objfile) > +{ > + if (objfile->flags & OBJF_DOWNLOAD_DEFERRED) > + return true; > + if (objfile->flags & OBJF_MAINLINE) > + return false; > + if (!IS_DIR_SEPARATOR (*objfile_filename (objfile))) > + return false; > + > + gdb::unique_xmalloc_ptr index_path; I hope this isn't too pedantic, but the index_path declaration can be moved right above the call to debuginfod_section_query. > + const bfd_build_id *build_id = build_id_bfd_get (objfile->obfd.get ()); > + > + if (build_id == nullptr) > + return false; > + > + scoped_fd fd = debuginfod_section_query (build_id->data, > + build_id->size, > + bfd_get_filename > + (objfile->obfd.get ()), > + ".gdb_index", > + &index_path); > diff --git a/gdb/objfiles.h b/gdb/objfiles.h > index c20b63ceadf..ea9bd2157dc 100644 > --- a/gdb/objfiles.h > +++ b/gdb/objfiles.h > @@ -612,6 +612,26 @@ struct objfile > /* See quick_symbol_functions. */ > void require_partial_symbols (bool verbose); > > + /* Indicate that the aquisition of this objfile's separate debug objfile > + is no longer deferred. Used when the debug objfile has been aquired *acquired > + or could not be found. */ > + void remove_deferred_status () > +void > +libsection1_test () > +{ > + pthread_t thr; > + > + printf ("In libsection1\n"); > + libsection2_test (); > + > + pthread_create (&thr, NULL, libsection2_thread_test, NULL); > + > + /* Give the new thread a chance to actually enter libsection2_thread_test. */ > + sleep (3); If the testcase is run in a slow simulator, it can take more than 3 seconds. Or in a heavily loaded machine. Can the test use pthread_cond_wait/pthread_cond_signal instead? > + printf ("Cancelling thread\n"); > + > + pthread_cancel (thr); > +} > +# Restart GDB and clear the debuginfod client cache. Then load BINFILE into > +# GDB and start running it. Match output with pattern RES and use TESTNAME > +# as the test name. > +proc_with_prefix clean_restart_with_prompt { binfile res testname } { The res argument isn't used by the function. > + global cache > + > + # Delete client cache so debuginfo downloads again. > + file delete -force $cache > + clean_restart > + > + gdb_test "set debuginfod enabled on" "" "clean_restart enable $testname" > + gdb_load $binfile > + > + if {![runto_main]} { > + return Isn't this return redundant? Suggest ending the procedure with just "[runto_main]". > + } > +} -- Thiago