diff mbox series

gdb: add an assert to cmd_list_element constructor

Message ID 423dd71ea44c518a22cfaf09b8dd5e9932cbf922.1744549721.git.aburgess@redhat.com
State New
Headers
Series gdb: add an assert to cmd_list_element constructor |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply

Commit Message

Andrew Burgess April 13, 2025, 1:08 p.m. UTC 
  The cmd_list_element::doc variable must be non-nullptr, otherwise, in
`help_cmd` (cli/cli-decode.c), we will trigger an assert when we run
one of these lines:

      gdb_puts (c->doc, stream);

or,

      gdb_puts (alias->doc, stream);

as gdb_puts requires that the first argument (the doc string) be
non-nullptr.

Better, I think, to assert when the cmd_list_element is created,
rather than catching an assert later when 'help CMD' is used.

I only ran into this case when messing with the Python API command
creation code, I accidentally created a command with a nullptr doc
string, and only found out when I ran 'help CMD' and got an
assertion.

Built and tested on x86-64 GNU/Linux with an all-targets build; I
don't see any regressions, so (I hope) there are no commands that
currently violate this assertion.
---
 gdb/cli/cli-decode.h | 1 +
 1 file changed, 1 insertion(+)


base-commit: 33d5188ab101bf414c9950ba914a128d08166105

Comments

Simon Marchi April 13, 2025, 1:48 p.m. UTC | #1 
On 2025-04-13 09:08, Andrew Burgess wrote:
> The cmd_list_element::doc variable must be non-nullptr, otherwise, in
> `help_cmd` (cli/cli-decode.c), we will trigger an assert when we run
> one of these lines:
> 
>       gdb_puts (c->doc, stream);
> 
> or,
> 
>       gdb_puts (alias->doc, stream);
> 
> as gdb_puts requires that the first argument (the doc string) be
> non-nullptr.
> 
> Better, I think, to assert when the cmd_list_element is created,
> rather than catching an assert later when 'help CMD' is used.
> 
> I only ran into this case when messing with the Python API command
> creation code, I accidentally created a command with a nullptr doc
> string, and only found out when I ran 'help CMD' and got an
> assertion.
> 
> Built and tested on x86-64 GNU/Linux with an all-targets build; I
> don't see any regressions, so (I hope) there are no commands that
> currently violate this assertion.
> ---
>  gdb/cli/cli-decode.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
> index 217afbc8ca7..673cf6518d3 100644
> --- a/gdb/cli/cli-decode.h
> +++ b/gdb/cli/cli-decode.h
> @@ -62,6 +62,7 @@ struct cmd_list_element
>        type (not_set_cmd),
>        doc (doc_)
>    {
> +    gdb_assert (doc != nullptr);
>      memset (&function, 0, sizeof (function));
>    }
>  
> 
> base-commit: 33d5188ab101bf414c9950ba914a128d08166105

Would it make sense to add an assert that `name` isn't nullptr at the
same time?

LGTM in any case.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
Andrew Burgess April 14, 2025, 8:30 a.m. UTC | #2 
Simon Marchi <simark@simark.ca> writes:

> On 2025-04-13 09:08, Andrew Burgess wrote:
>> The cmd_list_element::doc variable must be non-nullptr, otherwise, in
>> `help_cmd` (cli/cli-decode.c), we will trigger an assert when we run
>> one of these lines:
>> 
>>       gdb_puts (c->doc, stream);
>> 
>> or,
>> 
>>       gdb_puts (alias->doc, stream);
>> 
>> as gdb_puts requires that the first argument (the doc string) be
>> non-nullptr.
>> 
>> Better, I think, to assert when the cmd_list_element is created,
>> rather than catching an assert later when 'help CMD' is used.
>> 
>> I only ran into this case when messing with the Python API command
>> creation code, I accidentally created a command with a nullptr doc
>> string, and only found out when I ran 'help CMD' and got an
>> assertion.
>> 
>> Built and tested on x86-64 GNU/Linux with an all-targets build; I
>> don't see any regressions, so (I hope) there are no commands that
>> currently violate this assertion.
>> ---
>>  gdb/cli/cli-decode.h | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
>> index 217afbc8ca7..673cf6518d3 100644
>> --- a/gdb/cli/cli-decode.h
>> +++ b/gdb/cli/cli-decode.h
>> @@ -62,6 +62,7 @@ struct cmd_list_element
>>        type (not_set_cmd),
>>        doc (doc_)
>>    {
>> +    gdb_assert (doc != nullptr);
>>      memset (&function, 0, sizeof (function));
>>    }
>>  
>> 
>> base-commit: 33d5188ab101bf414c9950ba914a128d08166105
>
> Would it make sense to add an assert that `name` isn't nullptr at the
> same time?

Good idea.  I've added this assert, and (after some testing) pushed the
patch.

Thanks,
Andrew

>
> LGTM in any case.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
diff mbox series

Patch

diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index 217afbc8ca7..673cf6518d3 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -62,6 +62,7 @@  struct cmd_list_element
       type (not_set_cmd),
       doc (doc_)
   {
+    gdb_assert (doc != nullptr);
     memset (&function, 0, sizeof (function));
   }