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 D054B3888C57 for ; Fri, 18 Mar 2022 22:44:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D054B3888C57 Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-339-JGLR4xrpMz65s5vu4CSHyg-1; Fri, 18 Mar 2022 18:44:35 -0400 X-MC-Unique: JGLR4xrpMz65s5vu4CSHyg-1 Received: by mail-wm1-f70.google.com with SMTP id o33-20020a05600c512100b0038a1d06e525so6500899wms.2 for ; Fri, 18 Mar 2022 15:44:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=9WR7fov1xFW6qfPk9VTDxd/vSIOBI9EFUjwB924x0xM=; b=8IHdlgeYlOsYmDATBAAeVmk0GH6+H/IEzPVI7bfUyR2v70dXrDfBQRaqY3zW0QtDer wdrDwOwhARCX75a1FJx3UbYBKnRd/lYrzPhS2SfbML6Fvz46ShdPlpD6vAHi+8r4Fcd4 035txpX++EnXN+FDf6DhgvAoI66b//4zTyl7Hl/kWaTRN7FbrxzENBzh88L14ZQ3X8JC Njnd9uYs5Ku8DzoTRx0ctQF6amHfmSyaqS9+4eFlbJNs2k8ez9+pPjKO0/0x8EBt14nq Jh9w9hp4LQBY9xqNDurtL6wKvWT2T9S0wv2RWgUjT7Vc0NPhtiYVZzAxckULhwug/5Yy SY+g== X-Gm-Message-State: AOAM532wRzWviwQFgds7MrK4aM92vW1Ss9sWFG99Uiglebs8WiqEzAsG y6FXp68y1Ta+YMBQEhH+4T6NuRjxquX274X+b844bjzp3XoIFmvx/Gjc+c6P1QVGZidXaNwZ3GS FdTDTAX/wjNIFPMw1pNpfTA== X-Received: by 2002:adf:cc88:0:b0:203:f088:c9f5 with SMTP id p8-20020adfcc88000000b00203f088c9f5mr6719662wrj.312.1647643473453; Fri, 18 Mar 2022 15:44:33 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyVzvzBj1KWHqvajg33yoH13ahNMWlkwQtg35vzxvhjdbQuLW5WAdyCkKL8Vy0lFDjAWiVvtA== X-Received: by 2002:adf:cc88:0:b0:203:f088:c9f5 with SMTP id p8-20020adfcc88000000b00203f088c9f5mr6719651wrj.312.1647643473193; Fri, 18 Mar 2022 15:44:33 -0700 (PDT) Received: from localhost (host109-155-5-90.range109-155.btcentralplus.com. [109.155.5.90]) by smtp.gmail.com with ESMTPSA id q125-20020a1c4383000000b0038c770e00b5sm5397445wma.29.2022.03.18.15.44.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 18 Mar 2022 15:44:32 -0700 (PDT) Date: Fri, 18 Mar 2022 22:44:31 +0000 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/python: remove gdb._mi_commands dict Message-ID: <20220318224431.GS1212730@redhat.com> References: <20220318195526.1263787-1-simon.marchi@efficios.com> MIME-Version: 1.0 In-Reply-To: <20220318195526.1263787-1-simon.marchi@efficios.com> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 22:35:17 up 19 days, 12:13, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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: Fri, 18 Mar 2022 22:44:38 -0000 * 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: >=20 > CXX python/py-micmd.o > /home/smarchi/src/binutils-gdb/gdb/python/py-micmd.c: In function =E2= =80=98int micmdpy_uninstall_command(micmdpy_object*)=E2=80=99: > /home/smarchi/src/binutils-gdb/gdb/python/py-micmd.c:430:20: error: = =E2=80=98PyDict_GetItemWithError=E2=80=99 was not declared in this scope; d= id you mean =E2=80=98PyDict_GetItemString=E2=80=99? > 430 | PyObject *curr =3D PyDict_GetItemWithError (mi_cmd_dict.get= (), > | ^~~~~~~~~~~~~~~~~~~~~~~ > | PyDict_GetItemString >=20 > 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. >=20 > 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. >=20 > - 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) =09=09=09(cmd->mi_command_name =3D=3D nullptr =09=09=09 ? "(null)" : cmd->mi_command_name)); =20 - /* 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 !=3D 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_p= y + object has been deleted, in which case the following assert will + hold. */ gdb_assert (cmd->mi_command =3D=3D nullptr); =20 /* Free the memory that holds the command name. */