Re: [PATCH v4 3/4] gdb: Have setter and getter callbacks for settings

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-10-05 4:27 p.m., Lancelot SIX wrote:
> On Tue, Oct 05, 2021 at 03:10:55PM -0400, Simon Marchi wrote:
>>> @@ -241,13 +295,23 @@ struct setting
>>>      gdb_assert (var_type_uses<T> (m_var_type));
>>>      gdb_assert (m_var != nullptr);
>>>
>>> -    return *static_cast<T const *> (m_var);
>>> +    if (m_var == nullptr)
>>> +      {
>>> +	gdb_assert (m_getter != nullptr);
>>> +	auto getter = reinterpret_cast<setting_getter_ftype<T>> (m_getter);
>>> +	return getter ();
>>> +      }
>>> +    else
>>> +      return *static_cast<T *> (m_var);
>>
>> While pulling these upstream changes to the ROCm GDB port, I found that
>> we forgot to remove the `gdb_assert (m_var != nullptr)` above.
>>
>> I can fix that when I send a patch that actually uses the
>> getters/setters (which would trigger the assert).
>>
>> Simon
> 
> Hi,
> 
> This is odd, I was sure I did fix this omission, but obviously not.
> Thanks for spotting this, and my apologies for the inconvenience.
> 
> Here is a (trivial) patch that fixes that.
> 
> Best,
> Lancelot.
> 
> ---
> From 4c5b728046770ef923323162ac48246b947267d2 Mon Sep 17 00:00:00 2001
> From: Lancelot SIX <lsix@lancelotsix.com>
> Date: Tue, 5 Oct 2021 19:55:19 +0000
> Subject: [PATCH] gdb: Remove deprecated assertion in setting::get
> 
> The commit 702991711a91bd47b209289562843a11e7009396 (gdb: Have setter
> and getter callbacks for settings) makes it possible for a setting not
> to be backed by a memory buffer but use callback functions instead to
> retrieve or set the setting's value.
> 
> An assertion was not properly updated to take into account that the
> m_var member (which points to a memory buffer, if used) might be nullptr
> if the setting uses callback functions.  If the setting is backed by a
> memory buffer, the m_var has to be non nullptr, which is already checked
> before the pointer is dereferenced.
> 
> This commit removes this assertion as it is not valid anymore.
> ---
>  gdb/command.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/gdb/command.h b/gdb/command.h
> index 7c226f193b8..0049ab6ff9e 100644
> --- a/gdb/command.h
> +++ b/gdb/command.h
> @@ -288,7 +288,6 @@ struct setting
>    const T &get () const
>    {
>      gdb_assert (var_type_uses<T> (m_var_type));
> -    gdb_assert (m_var != nullptr);
>  
>      if (m_var == nullptr)
>        {
> -- 
> 2.30.2
> 

Well, since you have the patch already, go ahead and push it.

Thanks,

Simon