Re: [PATCH] [gdb/tui] Factor out tui_noscroll_window et al

[Tom de Vries <tdevries@suse.de>]

On 2/21/24 18:32, Simon Marchi wrote:
> On 11/13/23 12:08, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> +/* A TUI window that doesn't scroll.  */
>> Tom> +
>> Tom> +struct tui_noscroll_window : public virtual tui_win_info {
>>
>> The "{" should be on the next line.
>>
>> Tom> +/* A TUI window that occupies a single line.  */
>> Tom> +
>> Tom> +struct tui_oneline_window : public virtual tui_win_info {
>>
>> Some of these only have a single use.  That seems like overkill to me,
>> unless we're introducing another use someday.
>>
>> Tom> +#define TUI_SRC_WIN \
>> Tom> +  (dynamic_cast<tui_source_window *> (tui_win_list[SRC_WIN]))
>> Tom> +#define TUI_DISASM_WIN \
>> Tom> +  (dynamic_cast<tui_disasm_window *> (tui_win_list[DISASSEM_WIN]))
>>
>> I think these should use gdb::checked_static_cast.
> 
> Actually, since the base is virtual, dynamic_cast is needed.  Try
> changing development to false in bfd/development.sh, you'll get:
> 
>        CXX    tui/tui-layout.o
>      In file included from /home/smarchi/src/binutils-gdb/gdb/tui/tui-data.h:31,
>                       from /home/smarchi/src/binutils-gdb/gdb/tui/tui-command.h:25,
>                       from /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:31:
>      /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h: In instantiation of ‘T gdb::checked_static_cast(V*) [with T = tui_cmd_window*; V = tui_win_info]’:
>      /home/smarchi/src/binutils-gdb/gdb/tui/tui-layout.c:79:3:   required from here
>      /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb-checked-static-cast.h:64:14: error: cannot convert from pointer to base class ‘tui_win_info’ to pointer to derived class ‘tui_cmd_window’ because the base is virtual
>         64 |   T result = static_cast<T> (v);
>            |              ^~~~~~~~~~~~~~~~~~
> 
> It might be a good idea to add a static_cast in the DEVELOPMENT branch
> of checked_static_cast to catch this kind of mistake.  I think it could
> be written this way to be succinct (but untested):
> 
> 
>        T result = static_cast<T> (v);
>      #ifdef DEVELOPMENT
>        gdb_assert (result == dynamic_cast<T> (v));
>      #endif
> 
>        return result;
> 
> IOW, this patch:
> 

Hi Simon,

LGTM.  Also I can confirm that the patch catches the problem show above 
(which was reported as PR31399).

I'll write a fix for that PR, and test with your patch applied as well.

Thanks,
- Tom


> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
> index b6ce8c8bb04..e935bb359f0 100644
> --- a/gdbsupport/gdb-checked-static-cast.h
> +++ b/gdbsupport/gdb-checked-static-cast.h
> @@ -54,14 +54,9 @@ checked_static_cast (V *v)
>                   || std::is_base_of<T_no_P, V>::value,
>                   "types must be related");
> 
> -#ifdef DEVELOPMENT
> -  if (v == nullptr)
> -    return nullptr;
> -
> -  T result = dynamic_cast<T> (v);
> -  gdb_assert (result != nullptr);
> -#else
>     T result = static_cast<T> (v);
> +#ifdef DEVELOPMENT
> +  gdb_assert (result == dynamic_cast<T> (v));
>   #endif
> 
>     return result;
> 
> Simon