From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 268FE3858D3C for ; Sat, 19 Mar 2022 00:29:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 268FE3858D3C Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 22J0Tnm6016055 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 18 Mar 2022 20:29:54 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 22J0Tnm6016055 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 4600A1F0D2; Fri, 18 Mar 2022 20:29:49 -0400 (EDT) Message-ID: Date: Fri, 18 Mar 2022 20:29:48 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [PATCH] gdb/python: remove gdb._mi_commands dict Content-Language: en-US To: Andrew Burgess , Simon Marchi Cc: gdb-patches@sourceware.org References: <20220318195526.1263787-1-simon.marchi@efficios.com> <20220318224431.GS1212730@redhat.com> From: Simon Marchi In-Reply-To: <20220318224431.GS1212730@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sat, 19 Mar 2022 00:29:49 +0000 X-Spam-Status: No, score=-3038.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Sat, 19 Mar 2022 00:30:00 -0000 On 2022-03-18 18:44, Andrew Burgess via Gdb-patches wrote: > * Simon Marchi via Gdb-patches [2022-03-18 15:55:26 -0400]: > >> The motivation for this patch is the fact that py-micmd.c doesn't build >> with Python 2, due to PyDict_GetItemWithError being a Python 3-only >> function: >> >> CXX python/py-micmd.o >> /home/smarchi/src/binutils-gdb/gdb/python/py-micmd.c: In function ‘int micmdpy_uninstall_command(micmdpy_object*)’: >> /home/smarchi/src/binutils-gdb/gdb/python/py-micmd.c:430:20: error: ‘PyDict_GetItemWithError’ was not declared in this scope; did you mean ‘PyDict_GetItemString’? >> 430 | PyObject *curr = PyDict_GetItemWithError (mi_cmd_dict.get (), >> | ^~~~~~~~~~~~~~~~~~~~~~~ >> | PyDict_GetItemString >> >> A first solution to fix this would be to try to replace >> PyDict_GetItemWithError equivalent Python 2 code. But I looked at why >> we are doing this in the first place: it is to maintain the >> `gdb._mi_commands` Python dictionary that we use as a `name -> >> gdb.MICommand object` map. Since the `gdb._mi_commands` dictionary is >> never actually used in Python, it seems like a lot of trouble to use a >> Python object for this. >> >> My first idea was to replace it with a C++ map >> (std::unordered_map>). While >> implementing this, I realized we don't really need this map at all. The >> mi_command_py objects registered in the main MI command table can own >> their backing micmdpy_object (that's a gdb.MICommand, but seen from the >> C++ code). To know whether an mi_command is an mi_command_py, we can >> use a dynamic cast. Since there's one less data structure to maintain, >> there are less chances of messing things up. >> >> - Change mi_command_py::m_pyobj to a gdbpy_ref, the mi_command_py is >> now what keeps the MICommand alive. >> - Set micmdpy_object::mi_command in the constructor of mi_command_py. >> If mi_command_py manages setting/clearing that field in >> swap_python_object, I think it makes sense that it also takes care of >> setting it initially. >> - Move a bunch of checks from micmdpy_install_command to >> swap_python_object, and make them gdb_asserts. >> - In micmdpy_install_command, start by doing an mi_cmd_lookup. This is >> needed to know whether there's a Python MI command already registered >> with that name. But we can already tell if there's a non-Python >> command registered with that name. Return an error if that happens, >> rather than waiting for insert_mi_cmd_entry to fail. Change the >> error message to "name is already in use" rather than "may already be >> in use", since it's more precise. > > This all looks good to me. I think the small patch below should also > be included though. > > Thanks, > Andrew > > --- > > diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c > index 18a199de6b9..9b2b7863827 100644 > --- a/gdb/python/py-micmd.c > +++ b/gdb/python/py-micmd.c > @@ -579,11 +579,10 @@ micmdpy_dealloc (PyObject *obj) > (cmd->mi_command_name == nullptr > ? "(null)" : cmd->mi_command_name)); > > - /* Remove the command from the MI command table if needed. This will > - cause the mi_command_py object to be deleted, which, in turn, will > - clear the cmd->mi_command member variable, hence the assert. */ > - if (cmd->mi_command != nullptr) > - remove_mi_cmd_entry (cmd->mi_command->name ()); > + /* As the mi_command_py object holds a reference to the micmdpy_object, > + the only way the dealloc function can be called is if he mi_command_py > + object has been deleted, in which case the following assert will > + hold. */ > gdb_assert (cmd->mi_command == nullptr); > > /* Free the memory that holds the command name. */ > Good catch. Ammended and pushed, thanks! Simon