From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 B0E03385C305 for ; Sun, 19 Jun 2022 04:02:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B0E03385C305 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-613-KTgrC44VOwm90Ofq3wDO-g-1; Sun, 19 Jun 2022 00:02:16 -0400 X-MC-Unique: KTgrC44VOwm90Ofq3wDO-g-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4A11B185A7B2; Sun, 19 Jun 2022 04:02:16 +0000 (UTC) Received: from f35-zws-1 (unknown [10.2.16.89]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D78CCC28112; Sun, 19 Jun 2022 04:02:14 +0000 (UTC) Date: Sat, 18 Jun 2022 21:02:13 -0700 From: Kevin Buettner To: Markus Metzger via Gdb-patches Cc: Markus Metzger , Lu@sourceware.org, Hongjiu Subject: Re: [PATCH v5 03/15] gdb, gdbserver: support dlmopen() Message-ID: <20220618210213.73f6b911@f35-zws-1> In-Reply-To: <20220602132514.957983-4-markus.t.metzger@intel.com> References: <20220602132514.957983-1-markus.t.metzger@intel.com> <20220602132514.957983-4-markus.t.metzger@intel.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 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_LOW, SPF_HELO_NONE, SPF_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 19 Jun 2022 04:02:22 -0000 Hi Markus, This patch looks pretty good. Just a couple of comments/questions... On Thu, 2 Jun 2022 15:25:02 +0200 Markus Metzger via Gdb-patches wrote: > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c > index 6222f79a7a0..1f2abcf16ef 100644 [...] > @@ -1258,49 +1338,110 @@ svr4_current_sos_direct (struct svr4_info *info) > if (library_list.main_lm) > info->main_lm_addr = library_list.main_lm; > > - return library_list.head ? library_list.head : svr4_default_sos (info); > + /* Remove an empty special zero namespace so we know that when there > + is one, it is actually used, and we have a flat list without > + namespace information. */ > + if ((library_list.solib_lists.find (0) > + != library_list.solib_lists.end ()) > + && (library_list.solib_lists[0] == nullptr)) > + library_list.solib_lists.erase (0); > + > + std::swap (info->solib_lists, library_list.solib_lists); > + return; > } [...] > @@ -1697,17 +1872,34 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm) > if (!svr4_current_sos_via_xfer_libraries (&library_list, annex)) > return 0; > > - tail->next = library_list.head; > + /* We expect gdbserver to provide updates for the namespace that > + contains LM, which whould be this namespace... */ > + so_list *sos = nullptr; > + if (library_list.solib_lists.find (debug_base) > + != library_list.solib_lists.end ()) > + std::swap (sos, library_list.solib_lists[debug_base]); > + if (sos == nullptr) > + { > + /* ...or for the special zero namespace for earlier versions... */ > + if (library_list.solib_lists.find (0) > + != library_list.solib_lists.end ()) > + std::swap (sos, library_list.solib_lists[0]); [...] I was puzzled for a while by the uses of std:swap() in the above snippets of code. I checked the rest of the GDB sources and found that (at least in GDB) this is not a common idiom. I'd appreciate it if you could add a few lines of explanation for at least one of them. > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index 6aef39e5e02..98715fdc87e 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -2451,6 +2451,102 @@ proc skip_shlib_tests {} { > return 1 > } > > +# Return 1 if we should skip dlmopen tests, 0 if we should not. > + > +proc skip_dlmopen_tests {} { > + global srcdir subdir gdb_prompt inferior_exited_re > + > + # We need shared library support. > + if { [skip_shlib_tests] } { > + return 1 > + } > + > + set me "skip_dlmopen_tests" > + set lib { > + int foo (void) { > + return 42; > + } > + } > + set src { > + #define _GNU_SOURCE > + #include > + #include > + #include > + #include > + > + int main (void) { > + struct r_debug *r_debug; > + ElfW(Dyn) *dyn; > + void *handle; > + > + /* The version is kept at 1 until we create a new namespace. */ > + handle = dlmopen (LM_ID_NEWLM, DSO_NAME, RTLD_LAZY | RTLD_LOCAL); > + if (!handle) { > + printf ("dlmopen failed: %s.\n", dlerror ()); > + return 1; > + } > + > + r_debug = 0; > + /* Taken from /usr/include/link.h. */ > + for (dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) > + if (dyn->d_tag == DT_DEBUG) > + r_debug = (struct r_debug *) dyn->d_un.d_ptr; > + > + if (!r_debug) { > + printf ("r_debug not found.\n"); > + return 1; > + } > + if (r_debug->r_version < 2) { > + printf ("dlmopen debug not supported.\n"); > + return 1; > + } > + printf ("dlmopen debug supported.\n"); > + return 0; > + } > + } > + > + set libsrc [standard_temp_file "libfoo.c"] > + set libout [standard_temp_file "libfoo.so"] > + gdb_produce_source $libsrc $lib > + > + if { [gdb_compile_shlib $libsrc $libout {debug}] != "" } { > + verbose -log "failed to build library" > + return 1 > + } > + if { ![gdb_simple_compile $me $src executable \ > + [list shlib_load debug \ > + additional_flags=-DDSO_NAME=\"$libout\"]] } { > + verbose -log "failed to build executable" > + return 1 > + } > + > + gdb_exit > + gdb_start > + gdb_reinitialize_dir $srcdir/$subdir > + gdb_load $obj > + > + if { [gdb_run_cmd] != 0 } { > + verbose -log "failed to start skip test" > + return 1 > + } > + gdb_expect { > + -re "$inferior_exited_re normally.*${gdb_prompt} $" { > + set skip_dlmopen_tests 0 > + } > + -re "$inferior_exited_re with code.*${gdb_prompt} $" { > + set skip_dlmopen_tests 1 > + } > + default { > + warning "\n$me: default case taken" > + set skip_dlmopen_tests 1 > + } > + } > + gdb_exit > + > + verbose "$me: returning $skip_dlmopen_tests" 2 > + return $skip_dlmopen_tests > +} > + > # Return 1 if we should skip tui related tests. I'm wondering if this can be a gdb_caching_proc? At the moment, there's probably not much point since I think it's only used by the new test. But if we end up with more tests which use it, it might be worthwhile. Kevin