From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id DtCUA6Iod2hzHjYAWB0awg (envelope-from ) for ; Wed, 16 Jul 2025 00:20:50 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=sTaal/cJ; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id EF0991E11C; Wed, 16 Jul 2025 00:20:49 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.8 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_SBL_CSS,RCVD_IN_VALIDITY_CERTIFIED, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 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 362821E0C2 for ; Wed, 16 Jul 2025 00:20:43 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id AE8AB3858433 for ; Wed, 16 Jul 2025 04:20:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org AE8AB3858433 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=sTaal/cJ Received: from mail-vk1-xa36.google.com (mail-vk1-xa36.google.com [IPv6:2607:f8b0:4864:20::a36]) by sourceware.org (Postfix) with ESMTPS id 10B853858D32 for ; Wed, 16 Jul 2025 04:20:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 10B853858D32 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 10B853858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::a36 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1752639606; cv=none; b=tzdawTpNsRo06mixYPIl/yfOkajee4uIB4uWZ4y3G+kQ1qHnVdns87RWQcdV9ClUUN3cDOLTMwagzERTxAkbfzJIqhN5Vz7hkxC8dzTQfm3+W+XaYjYBU3GawNWCJz1KGq/3eEO8MWu1RvtEs77SG/qCGnsNn8y4SPwylCdgIaQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1752639606; c=relaxed/simple; bh=XVcKjwjN/ZCQMcIBBMHanzNExXGKFPRdEJlv1q8u/Tg=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=scynfz6Y+ZBhc1wDgZh4hqnh0QmdyfCF981h4W82z7YUYLyqm7zWl5L9M5o/zuzWNbTKWq0oA2FnmT2uhIMW/fAuarPItxWyXtg+gIQ9dbCBrVyNQZRF/FwTjkjaFrMyQoJZ+1jSHXJUFCnQexNewr40S8ObDdIpY0XOc25V9JY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 10B853858D32 Received: by mail-vk1-xa36.google.com with SMTP id 71dfb90a1353d-5315ebfda19so2931981e0c.1 for ; Tue, 15 Jul 2025 21:20:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1752639605; x=1753244405; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=OL7ZNTUTG8g1oLA5B10r09hB/QAQDEjFRmOEVaAWQas=; b=sTaal/cJyGlVQQSy7VKWuLiImkCZnRu425JI5KU7PZXMrP8ICP2Q1hRKMQ3ph6vCFk 7pTXfZLg9n8n4vVbyjShvO+sF8jo6vZEw0Uf1JW7he/TaQT5PGLue7jX2gFr5+IF6wPm KFMAzmQhQBGJmY/vkS+bHQ5ofz1i6LxsuYMgiyC7x+97MW8NQcVUzEQgNgMhwpTWan6d lVSQ6MQzInA/0GoXoyT6t1SMwnfpwigiyoCTV2/Bl34ZjRBfEPygHiQvyXgN6KMJcOr/ XCa6qjslXsjNeT/ibSfGnoZHDInU+OWoiXaYM6AxqAn0a0QDmzBB4OEN7w9EoFLROOAt nspA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1752639605; x=1753244405; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=OL7ZNTUTG8g1oLA5B10r09hB/QAQDEjFRmOEVaAWQas=; b=GhhNc8BS5P1nCWzVFqMREIKRpFyondPlL5CZJj7DxU94Fy8eQilY8KtGBdYvZZjJNq 4+q2Y2g6OaA/EjxdYUIj0t7UcIgAozNob80W3hcJ9hXcyD8CYwZvmtse2TR6KwjYcizC npWoWNAkOt1IVBqPZ/TG+ZJUJSw8NSZco6enOTU1y85KZsiBXTy31pUDiI6XaIbWRUM9 RnZUASgVk4fEekChqa9+NWw11+/HQ7ZAFmLV2yBcagENUBuT4tkihUTtp0+YhN0+hvju NafczRwbwPn0DhUEn7bDPuWtE0Wi4L7lkdpybGvIqOUIr9jfx5RjMNqI0D6c2uUs0Lbo elnA== X-Gm-Message-State: AOJu0YzJGww0KvwWylXguF36w2juPKVI7fmaR3reiaowC3fFkPj3/c9i VQMktycA3Mt4Zi0lpBL834kH94AzQ0QaGbs3zkxWpR9ScfSw07ilZrVDCc1OQZQHKNg= X-Gm-Gg: ASbGncv1D65mT+LwuxW4dkP1xCSY3zXkHzhZHTnC5HgGAF+Ud3LzkTUYl5XY9FWvuRN UAaCRmRwqXtfglzdRT9MTbHnf5bvcrWNmAZ7ELW5mrs23vCoByRXRmlr14AO4GGdnyWTLzVYmXj CIzm6gYlDscsa+snY8eJDl6OcqcMOJYkedXXy/0yVXe9vYny3Ytt1i3LBV1MTqfciHn5d5KRrbt teuhz/mYedDwjDtGxvR7q09vDF/1b6qh3wkehrfpokYAYDaWaVHn1YnX8bkGobmTFspbjFQPb0B geWiJ88m8LnIu8qqSHbQprWwLl4JmC0Bh6Tai8HKjt6mcm9/4LhTuJhUvY9RLTtOdIZHQyo//zD FlRPs+n/gI8Wa3APEHtk/eI0ipBxlo2L3 X-Google-Smtp-Source: AGHT+IEqjnNH9ckCxGsvQwbg5xzf0Ndfxl5LADe1AaSPMdSdQmNiraQ+kJfM8JlrkAVmqdUt6jy1dA== X-Received: by 2002:a05:6122:3105:b0:52c:4eb0:118d with SMTP id 71dfb90a1353d-5373fbd94ccmr499703e0c.4.1752639605110; Tue, 15 Jul 2025 21:20:05 -0700 (PDT) Received: from localhost ([2804:14d:7e39:88d6:11ec:ba5d:316b:779f]) by smtp.gmail.com with ESMTPSA id 71dfb90a1353d-535e7273d18sm3097544e0c.21.2025.07.15.21.20.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Jul 2025 21:20:03 -0700 (PDT) From: Thiago Jung Bauermann To: Tankut Baris Aktemur Cc: gdb-patches@sourceware.org, Markus Metzger Subject: Re: [PATCH v2 11/47] gdb, gdbserver, rsp, ze: acknowledge libraries In-Reply-To: <20241213-upstream-intelgt-mvp-v2-11-5c4caeb7b33d@intel.com> (Tankut Baris Aktemur's message of "Fri, 13 Dec 2024 16:59:28 +0100") References: <20241213-upstream-intelgt-mvp-v2-0-5c4caeb7b33d@intel.com> <20241213-upstream-intelgt-mvp-v2-11-5c4caeb7b33d@intel.com> User-Agent: mu4e 1.12.11; emacs 30.1 Date: Wed, 16 Jul 2025 01:20:01 -0300 Message-ID: <87y0sodaxa.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 Hello Baris, Tankut Baris Aktemur writes: > @@ -48084,16 +48141,18 @@ The format of a library list is described by th= is DTD: > @smallexample > > > - > + > > - > + + ack (yes | no) 'no'> > > - - end CDATA #REQUIRED> > + + end CDATA #REQUIRED > + ack (yes | no) 'no'> > > - > + > > - > + > @end smallexample I would move the last column (the one with "#FIXED", "#REQUIRED" and 'no') a couple more spaces to the right. The change above leaves them right next to "(segment*, section*)>" and it feels a bit crowded IMHO. >=20=20 > In addition, segments and section descriptors cannot be mixed within a > diff --git a/gdb/features/library-list.dtd b/gdb/features/library-list.dtd > index f55071c8e906f091752e8ca78ec29bcd76028433..473b8eaa719b1d310f9eb4fe8= f531df85008845d 100644 > --- a/gdb/features/library-list.dtd > +++ b/gdb/features/library-list.dtd > @@ -6,14 +6,16 @@ >=20=20 > > > - > + Same comment here as in patch 10: IMO it's better to just use CDATA #REQUIRED and rely on library_list_start_list to check if the version is acceptable. Also, same comment about bumping the version only once in the series, to 1.1. >=20=20 > > - > + + ack (yes | no) 'no'> >=20=20 > > - - end CDATA #REQUIRED> > + + end CDATA #REQUIRED > + ack (yes | no) 'no'> >=20=20 > > Same comment here about moving the last column a couple of spaces to the right. > @@ -15679,6 +15700,60 @@ remote_target::vcont_r_supported () > && get_remote_state ()->supports_vCont.r); > } >=20=20 > +void > +remote_target::ack_library (const char *name) Missing doc comment for this method. > +{ > + struct remote_state *rs =3D get_remote_state (); No need for the struct keyword. > + char *p =3D rs->buf.data (); > + char *endp =3D p + get_remote_packet_size (); > + > + xsnprintf (p, endp - p, "vAck:library:%s", name); > + > + putpkt (rs->buf); > + getpkt (&rs->buf, 0); > + > + packet_result result > + =3D m_features.packet_ok (rs->buf, PACKET_vAck_library); This fits 80 columns as one line. > + switch (result.status ()) > + { > + case PACKET_OK: > + break; > + case PACKET_UNKNOWN: > + error (_("No support for acknowledging libraries.")); > + case PACKET_ERROR: > + error (_("Acknowledging library '%s' failed: '%s'"), name, > + rs->buf.data ()); > + } > +} > + > +void > +remote_target::ack_in_memory_library (CORE_ADDR begin, CORE_ADDR end) Missing doc comment for this method. > +{ > + struct remote_state *rs =3D get_remote_state (); No need for the struct keyword. > + char *p =3D rs->buf.data (); > + char *endp =3D p + get_remote_packet_size (); > + > + xsnprintf (p, endp - p, "vAck:in-memory-library:%s,%s", > + core_addr_to_string_nz (begin), core_addr_to_string_nz (end)); > + > + putpkt (rs->buf); > + getpkt (&rs->buf, 0); > + > + packet_result result > + =3D m_features.packet_ok (rs->buf, PACKET_vAck_in_memory_library); > + switch (result.status ()) > + { > + case PACKET_OK: > + break; > + case PACKET_UNKNOWN: > + error (_("No support for acknowledging in-memory libraries.")); > + case PACKET_ERROR: > + error (_("Failed to acknowledge in-memory library %s-%s: %s"), > + core_addr_to_string_nz (begin), core_addr_to_string_nz (end), > + rs->buf.data ()); > + } > +} > + > /* The "set/show range-stepping" set hook. */ >=20=20 > static void > @@ -16442,6 +16517,12 @@ Show the maximum size of the address (in bits) i= n a memory packet."), NULL, >=20=20 > add_packet_config_cmd (PACKET_vCtrlC, "vCtrlC", "ctrl-c", 0); >=20=20 > + add_packet_config_cmd (PACKET_vAck_library, > + "vAck:library", "ack-library", 0); This fits 80 columns as one line. > + > + add_packet_config_cmd (PACKET_vAck_in_memory_library, > + "vAck:in-memory-library", "ack-in-memory-library", 0); > + > add_packet_config_cmd (PACKET_QThreadEvents, "QThreadEvents", "thread-= events", > 0); > @@ -473,6 +503,32 @@ solib_target_in_dynsym_resolve_code (CORE_ADDR pc) > return in_plt_section (pc); > } >=20=20 > +static void > +solib_target_ack_library (solib &so) > +{ > + lm_info_target *lm > + =3D gdb::checked_static_cast (so.lm_info.get ()); > + > + if (!lm->need_ack) > + return; > + > + /* Try only once, whether we succeed or not. */ > + lm->need_ack =3D false; > + switch (lm->location) > + { > + case lm_on_disk: > + target_ack_library (so.so_original_name.c_str ()); > + return; > + > + case lm_in_memory: > + target_ack_in_memory_library (lm->begin, lm->end); > + return; > + } > + > + warning (_("bad solib location '%d' for %s."), lm->location, > + so.so_original_name.c_str ()); Isn't this an internal error? If so, it should call gdb_assert_not_reached. > +} > + > const solib_ops solib_target_so_ops =3D > { > solib_target_relocate_section_addresses, > @@ -489,4 +545,5 @@ const solib_ops solib_target_so_ops =3D > nullptr, > default_find_solib_addr, > gdb_bfd_open_from_target_memory, > + solib_target_ack_library, > }; > diff --git a/gdb/solib.c b/gdb/solib.c > index cb302f9215257ddff18c94f5be39e189b02d84e6..92fc5137a3d469f55043235a6= 5dcadcc1f25fe96 100644 > --- a/gdb/solib.c > +++ b/gdb/solib.c > @@ -967,6 +967,7 @@ solib_add (const char *pattern, int from_tty, int rea= dsyms) > if (from_tty) > add_flags |=3D SYMFILE_VERBOSE; >=20=20 > + std::list added_solibs; > for (solib &gdb : current_program_space->solibs ()) > if (!pattern || re_exec (gdb.so_name.c_str ())) > { > @@ -989,14 +990,23 @@ solib_add (const char *pattern, int from_tty, int r= eadsyms) > styled_string (file_name_style.style (), > gdb.so_name.c_str ())); > } > - else if (solib_read_symbols (gdb, add_flags)) > - loaded_any_symbols =3D true; > + else > + added_solibs.emplace_back (&gdb); This should use the push_back method since we don't want to create a new object in the list. > } > } >=20=20 > + for (solib *gdb : added_solibs) > + if (solib_read_symbols (*gdb, add_flags)) > + loaded_any_symbols =3D true; > + > if (loaded_any_symbols) > breakpoint_re_set (); >=20=20 > + /* Acknowledge loading of new solibs. This must be called after > + breakpoints have been set in this newly loaded solib. */ > + for (solib *gdb : added_solibs) > + solib_ack_library (*gdb); > + > if (from_tty && pattern && !any_matches) > gdb_printf ("No loaded shared libraries match the pattern `%s'.\n", > pattern); > @@ -1700,6 +1710,16 @@ default_find_solib_addr (solib &so) > return {}; > } >=20=20 > +/* See solist.h. */ > + > +void solib_ack_library (solib &so) This function is only called from solib_add, shouldn't it be static? Or even just inlined into the caller? > +{ > + const solib_ops *ops =3D gdbarch_so_ops (current_inferior ()->arch ()); > + > + if (ops->ack_library !=3D nullptr) > + (*ops->ack_library) (so); > +} > + > void _initialize_solib (); >=20=20 > void > @@ -60,6 +72,78 @@ unloaded_dll (const char *name, CORE_ADDR base_addr) > unloaded_dll (current_process (), name, base_addr); > } >=20=20 > +static void > +ack_dll (process_info *process, dll_info &dll) > +{ > + gdb_assert (dll.need_ack); > + > + switch (dll.location) > + { > + case dll_info::on_disk: > + /* Check if this is a temporary file for an in-memory library. */ > + if (dll.begin =3D=3D UNSPECIFIED_CORE_ADDR) > + { > + target_ack_library (process, dll.name.c_str ()); > + dll.need_ack =3D false; > + return; > + } > + > + [[fallthrough]]; > + case dll_info::in_memory: > + target_ack_in_memory_library (process, dll.begin, dll.end); > + dll.need_ack =3D false; > + return; > + } I'm confused by this switch statement. So a vAck:library packet will be used only if it's for an in-memory library and vAck:in-memory-library isn't supported? I would think that a vAck:library would also be used for an on-disk library. > + internal_error (_("bad library location: %d."), dll.location); > +} > + > +void > +ack_dll (process_info *proc, const char *name) > +{ > + std::list &dlls =3D proc->all_dlls; > + std::list::iterator it > + =3D std::find_if (dlls.begin (), dlls.end (), > + [name] (const dll_info &dll) > + { > + return (dll.name =3D=3D std::string (name)); > + }); > + > + if (it !=3D dlls.end ()) > + ack_dll (proc, *it); > +} > + > +void > +ack_dll (const char *name) > +{ > + ack_dll (current_process (), name); > +} > + > +void > +ack_dll (process_info *proc, CORE_ADDR begin, CORE_ADDR end) > +{ > + std::list &dlls =3D proc->all_dlls; > + std::list::iterator it > + =3D std::find_if (dlls.begin (), dlls.end (), > + [begin, end] (const dll_info &dll) > + { > + /* For root devices with multiple sub-devices, modules with > + identical start/end addresses may be received for different > + sub-devices. Therefore we check for the 'NEED_ACK' flag in > + the search, too. */ And is it guaranteed that only one of them will have need_ack set to true? Can you explain that in the comment? > + return ((dll.begin =3D=3D begin) && (dll.end =3D=3D end) && dll.need_= ack); > + }); > + > + if (it !=3D dlls.end ()) > + ack_dll (proc, *it); > +} > @@ -3371,6 +3398,66 @@ err: > return; > } >=20=20 > +/* Parse vAck packets. */ > + > +static void > +handle_v_ack (char *own_buf) > +{ > + client_state &cs =3D get_client_state (); > + char *p; > + > + /* Move past vAck: to the first type string. */ > + p =3D &own_buf[5]; > + do > + { > + if (cs.vack_library_supported > + && (strncmp (p, "library:", strlen ("library:")) =3D=3D 0)) > + { > + p +=3D strlen ("library:"); > + > + /* We expect a single argument: the filename. */ > + const char *name =3D p; > + p =3D strchr (p, ';'); > + if (p !=3D nullptr) > + *p++ =3D '\0'; > + > + ack_dll (name); The documentation for vAck says: Acknowledge a =E2=80=98;=E2=80=99-separated list of remote stub responses= , each with a =E2=80=98,=E2=80=99- separated list of arguments defined by its type. But the above will consider everything between the ':' and ';' to be the library name, even if there's a ','. I think you should check whether there's a ',' in name, and if so either reject the packet or ignore the additional arguments =E2=80=94 not sure which is better, I'm leaning towards rejecting the packet. > + } > + else if (cs.vack_in_memory_library_supported > + && (strncmp (p, "in-memory-library:", > + strlen ("in-memory-library:")) =3D=3D 0)) > + { > + p +=3D strlen ("in-memory-library:"); > + > + /* We expect two arguments: begin and end address. */ > + CORE_ADDR begin, end; > + > + begin =3D (CORE_ADDR) strtoull (p, &p, 16); > + if (*p !=3D ',') > + break; If this break is taken and begin is the last argument in the packet, then won't *p =3D=3D 0 and cause write_ok () to be called below? An error should be returned instead. > + end =3D (CORE_ADDR) strtoull (p+1, &p, 16); > + if (*p =3D=3D ';') > + p +=3D 1; > + else if (*p !=3D 0) > + break; > + > + ack_dll (begin, end); > + } > + else > + break; > + } > + while (p !=3D nullptr && *p !=3D 0); > + > + if (p =3D=3D nullptr || *p =3D=3D 0) > + write_ok (own_buf); > + else > + { > + std::string junk { p }; > + sprintf (own_buf, "E.junk in vAck: '%s'.", junk.c_str ()); > + } > +} --=20 Thiago