Re: "show remote foo-packet" regression (Re: [PATCH v2 1/3] gdb: Make global feature array a per-remote target array)

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Pedro Alves <pedro@palves.net> writes:
>
>> On 2022-04-21 11:25, Andrew Burgess wrote:
>>> Pedro Alves <pedro@palves.net> writes:
>>>>
>>>> This commit is present in the GDB 12 branch and not in GDB 11, so it's a regression there.
>>> 
>>> Sorry for the regression.  I took a look at the code and I honestly have
>>> no idea why I thought the code in the above commit would work :-/
>>> 
>>
>> No worries, happens all the time to me.  :-)
>>
>>> Below is a patch that should fix this issue.  I probably should write a
>>> test to go along with it, but thought I'd share this for feedback first.
>>> 
>>> Let me know what you think,
>>
>> LGTM.  Thanks for fixing it.
>
> Thanks, I'll just let a test run complete, and push this later today.

I pushed the following patch to master and gdb-12-branch.

Thanks,
Andrew

---

commit 5f21c7aae2040f3463d1ff56a4b0bcdbba34832d
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Apr 21 11:16:18 2022 +0100

    gdb: fix 'remote show FOO-packet' aliases
    
    The following behaviour was observed in GDB:
    
      (gdb) show remote X-packet
      Support for the `p' packet is auto-detected, currently unknown.
    
    Note the message mentions the 'p' packet.  This is a regression since
    this commit:
    
      commit 8579fd136a614985bd27f20539c7bb7c5a51287d
      Date:   Mon Nov 8 14:58:46 2021 +0000
    
          gdb/gdbsupport: make xstrprintf and xstrvprintf return a unique_ptr
    
    Before this commit the behaviour was:
    
      (gdb) show remote X-packet
      Support for the `X' packet is auto-detected, currently unknown.
    
    The problem was caused by a failed attempt to ensure that some
    allocated strings were deleted when GDB exits.  The code in the above
    commit attempted to make use of 'static' to solve this problem,
    however, the solution was just wrong.
    
    In this new commit I instead allocate a static vector into which all
    the allocated strings are stored, this ensures the strings are
    released when GDB exits (which makes output from tools like valgrind
    cleaner), but each string within the vector can be unique, which fixes
    the regression.

diff --git a/gdb/remote.c b/gdb/remote.c
index 75d6bf3919d..562cc586f2b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1968,15 +1968,17 @@ add_packet_config_cmd (struct packet_config *config, const char *name,
   /* set/show remote NAME-packet {auto,on,off} -- legacy.  */
   if (legacy)
     {
-      /* It's not clear who should take ownership of this string, so, for
-	 now, make it static, and give copies to each of the add_alias_cmd
-	 calls below.  */
-      static gdb::unique_xmalloc_ptr<char> legacy_name
+      /* It's not clear who should take ownership of the LEGACY_NAME string
+	 created below, so, for now, place the string into a static vector
+	 which ensures the strings is released when GDB exits.  */
+      static std::vector<gdb::unique_xmalloc_ptr<char>> legacy_names;
+      gdb::unique_xmalloc_ptr<char> legacy_name
 	= xstrprintf ("%s-packet", name);
       add_alias_cmd (legacy_name.get (), cmds.set, class_obscure, 0,
 		     &remote_set_cmdlist);
       add_alias_cmd (legacy_name.get (), cmds.show, class_obscure, 0,
 		     &remote_show_cmdlist);
+      legacy_names.emplace_back (std::move (legacy_name));
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/remote.exp b/gdb/testsuite/gdb.base/remote.exp
index 1f0869433f2..579dcd40e5c 100644
--- a/gdb/testsuite/gdb.base/remote.exp
+++ b/gdb/testsuite/gdb.base/remote.exp
@@ -195,4 +195,9 @@ gdb_test_no_output "set remote hardware-breakpoint-limit -1"
 gdb_test_no_output "set remote hardware-watchpoint-limit 2147483647"
 gdb_test_no_output "set remote hardware-breakpoint-limit 2147483647"
 
+# Check the X/P/p alias commands display the correct packet names.
+foreach pkt {X P p} {
+    gdb_test "show remote ${pkt}-packet" "Support for the `${pkt}' packet is.*"
+}
+
 gdb_exit