From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id GK88MmluPGmFyDsAWB0awg (envelope-from ) for ; Fri, 12 Dec 2025 14:35:05 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1765568105; bh=amH71PStuC5L8TMhqRkMAeeAiBtQCQIHPFm6eUn5TU8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=hsCOaBcQn2BbxFZhaRBI+/M+CriVnvuX5f3/1w8Up+HXreIF+6mPFRVoTCBlqgpoT XQw2lI8YDQ3BDaFwACVvTeEgvl2c0gGI5T0mr8++FyXwHi1KxUX5IWFZXJou452e/x WtPE+QGJVLlfZSe4LbqU41e6dFlir0veZO/h87IM= Received: by simark.ca (Postfix, from userid 112) id A647C1E08D; Fri, 12 Dec 2025 14:35:05 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=MS2AYKjv; dkim-atps=neutral Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (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 164C91E08D for ; Fri, 12 Dec 2025 14:35:03 -0500 (EST) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id 6AEA74BA2E23 for ; Fri, 12 Dec 2025 19:35:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6AEA74BA2E23 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=MS2AYKjv Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id EDC024BA2E04 for ; Fri, 12 Dec 2025 19:34:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EDC024BA2E04 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EDC024BA2E04 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1765568076; cv=none; b=eCccOXE8qbVy2C3iIb11KBnrrF8vojzMvP0BpyWuOVJKIiITkNPAOAaTTQ1WdDTRGhicwNRnVQ5+7Gj8Ta7UJF6omfI12g9as9QQmDgS1ipFCr2oXRHiKjV+16qoJ5XL7hCDrtZ2p+BLjpsho4ddyycPQO/1aF8g4L06FI2RVm0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1765568076; c=relaxed/simple; bh=amH71PStuC5L8TMhqRkMAeeAiBtQCQIHPFm6eUn5TU8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=F6pqnQVUAXyJC4IjN8HmXioOCbx47hYF5Gb+wl8RBhomE+crTVj3PA5m5/dgTC1ZjYiTlnxdOVqwbBRGVJITpLcx2W/UKdJxacB+hemWnkxPEH2q1Cv5aCn9IqKTU5xoMIH1RQ0ZlwQsLmV85ql2r3dxq82gQQuNXAYJ+fNV7nE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EDC024BA2E04 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1765568075; bh=amH71PStuC5L8TMhqRkMAeeAiBtQCQIHPFm6eUn5TU8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=MS2AYKjvk8v/obHUXb4JnNOmhyoRkJVkC7zJ44DtxIf2lKqzgJ2wfAEYlgDAFAxpM MfRppdRO65f2xbLBUZk/O03UKPbUyDF+0VH8WeuXZ769uoEKPOzauG3tc+RFYf8SBJ qXqLyOO5g2I9hGJwCPAxlqLrInzzDCRdTEYU/HcA= Received: by simark.ca (Postfix) id 65A4D1E08D; Fri, 12 Dec 2025 14:34:35 -0500 (EST) Message-ID: <1c2082a9-02df-41ca-84bb-abdf6546ab35@simark.ca> Date: Fri, 12 Dec 2025 14:34:34 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 09/44] gdb, gdbserver, ze: in-memory libraries To: "Metzger, Markus T" Cc: "Aktemur, Tankut Baris" , "gdb-patches@sourceware.org" , Thiago Jung Bauermann References: <20250801-upstream-intelgt-mvp-v3-0-59ce0f87075b@intel.com> <20250801-upstream-intelgt-mvp-v3-9-59ce0f87075b@intel.com> <683efcc8-2a5d-4afe-b44d-8364e6868ecd@simark.ca> Content-Language: fr From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 On 12/12/25 6:20 AM, Metzger, Markus T wrote: > Hello Simon, > > Thanks for your review. > >>> diff --git a/gdb/features/library-list.dtd b/gdb/features/library-list.dtd >>> index >> 66945cbe97c13cfac5a4d00e9a752ccd8419252f..baa01485af950c58e9e242229 >> 365501c043e2fe1 100644 >>> --- a/gdb/features/library-list.dtd >>> +++ b/gdb/features/library-list.dtd >>> @@ -5,12 +5,16 @@ >>> notice and this notice are preserved. --> >>> >>> >>> - >>> - >>> + >>> + >> >> Hmm, looking at the history of this file and other .dtd files, we added >> attributes and elements in the past without bumping this version number. >> I guess it's not needed because we do our own checking of what the other >> side supports via the qSupported packet and adjust the behavior based on >> that. If not useful, I think we could keep 1.0 here for simplicity. > > This has been discussed with Thiago, as well in > https://sourceware.org/pipermail/gdb-patches/2025-July/219545.html > > Thiago thought that the version bump was necessary, but suggested bumping > it once for the entire series and not for each patch that makes changes to it. > > I'm fine, either way, but I wonder why we major.minor version if we then ignore it. As far as I know, the GDB client doesn't even check these version numbers, so I am not sure what bumping the version number would help with. It won't prevent an older GDB from trying to read a newer backwards-incompatible version. I see two possibilities: 1. Adding a new attribute or element is considered backwards-incompatible, in which case this version number should have been bumped a few times already. But they haven't so... not sure it's worth starting now. 2. Adding a new attribute or element is considered backwards-compatible (an older client would just ignore them), so it's not necessary to bump the version. Would it work to just make gdbserver return these in-memory-library elements, without even a qSupported flag? Would older GDBs just ignore them? >> new_solib.original_name = (std::string ("in-memory-") >> + core_addr_to_string_nz (info- >>> begin) >> + "-" >> + core_addr_to_string_nz (info- >>> end)); >> >> Just a note regarding ROCm. We have a particular syntax for in-memory >> objects, like this: >> >> memory://57663#offset=0x55555562b040&size=62488 >> >> It is documented here: >> >> https://llvm.org/docs/AMDGPUUsage.html#loaded-code-object-path-uniform- >> resource-identifier-uri >> >> Currently, solib-rocm.c generates solibs with that kind of name for >> in-memory objects. If we add proper support for solibs in memory, like >> this patch does, we'll probably want to change solib-rocm to use that. >> However, we'll want to make sure that "info shared" still shows the URIs >> in the format shown above, at least for ROCm. We can cross this bridge >> when we get there, but we'll need to find a way. > > The name that gets printed by 'info shared' is defined by the in-memory bfd. > > I don't know where this name is used. Looks like it is OK to not provide a name. Ok, so "info shared" shows solib->name. For in-memory solibs, you don't set solib->name? Does this mean that no name is shown in "info shared" for these libraries? As of today, even with this change, solib-rocm.c could set solib->name to whatever it wants. If we ever want to support remote debugging with ROCm, we would want to find a way to get our `memory://...` string as the name. Do you think that the remote side could provide a name inside an element? A ROCm would put that `memory://...` string. But in general, it would be a place to put a user-friendly name that will end up in "info shared". > >>> + if (so.end <= so.begin) >>> + error (_("Bad address range [%s; %s) for in-memory shared library."), >>> + core_addr_to_string_nz (so.begin), >>> + core_addr_to_string_nz (so.end)); >> >> Sounds like we should never end up with an solib like this, I don't >> think we need to handle this case here. > > We're subtracting so.begin from so.end. If you don't like the error, we > should assert this. The only place that currently adds such solibs ignores > those with a warning, so asserting it should be fine. Yes I think an assert makes sense here (or in an eventual solib constructor that takes these two addresses). Filtering for bad solib data would always be done earlier than that. >>> +/* Print a qXfer:libraries:read entry for DLL. */ >>> + >>> +static std::string >>> +print_qxfer_libraries_entry (dll_info &dll) >>> +{ >>> + switch (dll.location) >>> + { >>> + case dll_info::in_memory: >>> + if (get_client_state ().in_memory_library_supported) >>> + return string_printf >>> + (" " >>> + "\n", >>> + paddress (dll.begin), paddress (dll.end), >>> + paddress (dll.base_addr)); >>> + >>> + /* GDB does not support in-memory-library. Fall back to storing it in a >>> + temporary file and report that file to GDB. */ >> >> Curious, do you actually need this fallback? If not, could GDBserver >> just not report these in-memory libraries if GDB is too old? If we can >> avoid the complexity... > > Since support will be there from the beginning for our target, we don't > actually need the fall-back. A GDB that doesn't support this also wouldn't > be able to support the IntelGT architecture. > > I'm fine to drop it. That would be my preference, otherwise it's code that will never actually be used but will add maintenance cost. >>> @@ -2772,6 +2896,7 @@ handle_query (char *own_buf, int packet_len, int >> *new_packet_len_p) >>> /* We do not have any hook to indicate whether the non-SVR4 target >>> backend supports qXfer:libraries:read, so always report it. */ >>> strcat (own_buf, ";qXfer:libraries:read+"); >>> + strcat (own_buf, ";qXfer:libraries:read:in-memory-library+"); >> >> So, this makes gdbserver reply "I support in memory libraries". Is it >> really needed? It seems to me like it's only important for GDB to tell >> gdbserver, not the other way around. > > Right, GDB needs to opt into this new response. > > I thought it was common practice to announce features in gdbserver. > If we only do this for opt-ins, I can drop this. If GDB needs to know what gdbserver supports, it makes sense for gdbserver to announce it, but I think it's only the other way around in this case. But really, if we can just add the new element and it is gracefully ignored by older GDBs, without the need for a feature flag, that would be the simplest solution (and therefore my preference). Simon