Re: [PATCH] gdb/python: remove gdb._mi_commands dict

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2022-03-18 18:44, Andrew Burgess via Gdb-patches wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [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<std::string, gdbpy_ref<micmdpy_object>>).  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