[PATCH] [gdb/tui] Fix assert in tui_source_window_base::refresh_window

[Tom de Vries]

On 1/28/25 22:45, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> Say we use the executable of test-case gdb.tui/tui-missing-src.exp like this:
>> ...
>> $ gdb.sh -q -tui outputs/gdb.tui/tui-missing-src/tui-missing-src \
>>      -ex "b f2"\
>>      -ex run
>> ...
>> (from a directory not containing a file called main.c to make sure that the
>> missing source stays missing) and then issue finish:
>> ...
>> (gdb) finish
>> Run till exit from #0  f2 (x=4)
>>      at f2.c:5
>> 0x0000000000400570 in main ()
>>      at main.c:7
>> Value returned is $1 = 13
>> (gdb)
>> ...
>> and use control-<minus> to decrease the font size (IOW increase the $LINES and
>> $COLUMNS) on the terminal, we get:
>> ...
>> gdb/tui/tui-winsource.c:340: internal-error: refresh_window: \
>>    Assertion `pad_x + view_width <= pad_width || m_pad.get () == nullptr' failed.
>> A problem internal to GDB has been detected,
>> further debugging may prove unreliable.
>> ...
>>
>> The tui_source_window_base class has variable m_pad which keeps track of a
>> curses pad that is used to display the source code or disassembly efficiently.
>>
>> The assert in tui_source_window_base::refresh_window triggers while preparing
>> to display part of the pad.
>>
>> The problem is that the window is in a state in which the pad is not used,
>> because m_content.empty () == true.  Instead, it simply shows
>> "[ No Source Available ]".
>>
>> Fix this by bailing out of tui_source_window_base::refresh_window before
>> accessing the m_pad variable, if m_content.empty () == true.
>>
>> Tested on x86_64-linux.
>>
>> PR tui/32592
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32592
>> ---
>>   gdb/testsuite/gdb.tui/tui-missing-src.exp | 3 +++
>>   gdb/tui/tui-winsource.c                   | 3 +++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/gdb/testsuite/gdb.tui/tui-missing-src.exp b/gdb/testsuite/gdb.tui/tui-missing-src.exp
>> index 1929a1e3568..2310c1d71c8 100644
>> --- a/gdb/testsuite/gdb.tui/tui-missing-src.exp
>> +++ b/gdb/testsuite/gdb.tui/tui-missing-src.exp
>> @@ -101,3 +101,6 @@ Term::command "finish"
>>   Term::check_box_contents "check source box is empty after return" \
>>       0 0 80 15 "No Source Available"
>>   Term::check_contents "Back in main" "Value returned is .* 13"
>> +
>> +Term::resize 30 100
>> +Term::check_box "source box after resize" 0 0 100 19
>> diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
>> index a313e44bb33..a5d0c594545 100644
>> --- a/gdb/tui/tui-winsource.c
>> +++ b/gdb/tui/tui-winsource.c
>> @@ -314,6 +314,9 @@ tui_source_window_base::refresh_window ()
>>        the screen, potentially creating a flicker.  */
>>     wnoutrefresh (handle.get ());
>>   
>> +  if (m_content.empty ())
>> +    return;
>> +
> 
> If we make this change, then I'm pretty sure that the reset of
> ::refresh_window can be simplified a little.
> 
> Looking at the code you'll see there are checks for m_pad being nullptr.
> 
> I'm pretty sure the intended assumption of this code could be expressed
> as:
> 
>    gdb_assert (m_content.empty () == (m_pad == nullptr));
> 
> obviously this isn't actually true (hence the crash you're seeing), but
> if you are doing an early exit when `m_content.empty ()` then you can, I
> think, remove all the `m_pad == nullptr` handling from later in the
> function.
> 
> And, I think, add:
> 
>    gdb_assert (m_pad != nullptr);
> 
> immediately after your early return.
> 
> I'm happy to pick this up later this week if you'd prefer.
> 

Hi Andrew,

thanks for noticing this.

I've submitted a patch with your proposed changes ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/215040.html ).

Thanks,
- Tom

> Thanks,
> Andrew
> 
> 
>>     int pad_width = getmaxx (m_pad.get ());
>>     int left_margin = this->left_margin ();
>>     int view_width = this->view_width ();
>>
>> base-commit: 665888ea416e3b9b764ba2290af7dc69dbd6f25c
>> -- 
>> 2.43.0
>