Re: [PATCHv2 01/16] gdb: don't try to use readline before it's initialized

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

On 2022-04-06 08:04, Andrew Burgess via Gdb-patches wrote:
> While working on a different patch, I triggered an assertion from the
> initialize_current_architecture code, specifically from one of
> the *_gdbarch_init functions in a *-tdep.c file.  This exposes a
> couple of issues with GDB.
> 
> This is easy enough to reproduce by adding 'gdb_assert (false)' into a
> suitable function.  For example, I added a line into i386_gdbarch_init
> and can see the following issue.
> 
> I start GDB and immediately hit the assert, the output is as you'd
> expect, except for the very last line:
> 
>   $ ./gdb/gdb --data-directory ./gdb/data-directory/
>   ../../src.dev-1/gdb/i386-tdep.c:8455: internal-error: i386_gdbarch_init: Assertion `false' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
>   ----- Backtrace -----
>   ... snip ...
>   ---------------------
>   ../../src.dev-1/gdb/i386-tdep.c:8455: internal-error: i386_gdbarch_init: Assertion `false' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
>   Quit this debugging session? (y or n) ../../src.dev-1/gdb/ser-event.c:212:16: runtime error: member access within null pointer of type 'struct serial'
> 
> Something goes wrong when we try to query the user.  Note, I
> configured GDB with --enable-ubsan, I suspect that without this the
> above "error" would actually just be a crash.
> 
> The backtrace from ser-event.c:212 looks like this:
> 
>   (gdb) bt 10
>   #0  serial_event_clear (event=0x675c020) at ../../src/gdb/ser-event.c:212
>   #1  0x0000000000769456 in invoke_async_signal_handlers () at ../../src/gdb/async-event.c:211
>   #2  0x000000000295049b in gdb_do_one_event () at ../../src/gdbsupport/event-loop.cc:194
>   #3  0x0000000001f015f8 in gdb_readline_wrapper (
>       prompt=0x67135c0 "../../src/gdb/i386-tdep.c:8455: internal-error: i386_gdbarch_init: Assertion `false' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugg"...)
>       at ../../src/gdb/top.c:1141
>   #4  0x0000000002118b64 in defaulted_query(const char *, char, typedef __va_list_tag __va_list_tag *) (
>       ctlstr=0x2e4eb68 "%s\nQuit this debugging session? ", defchar=0 '\000', args=0x7fffffffa6e0)
>       at ../../src/gdb/utils.c:934
>   #5  0x0000000002118f72 in query (ctlstr=0x2e4eb68 "%s\nQuit this debugging session? ")
>       at ../../src/gdb/utils.c:1026
>   #6  0x00000000021170f6 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (problem=0x6107bc0 <internal_error_problem>, file=0x2b976c8 "../../src/gdb/i386-tdep.c",
>       line=8455, fmt=0x2b96d7f "%s: Assertion `%s' failed.", ap=0x7fffffffa8e8) at ../../src/gdb/utils.c:417
>   #7  0x00000000021175a0 in internal_verror (file=0x2b976c8 "../../src/gdb/i386-tdep.c", line=8455,
>       fmt=0x2b96d7f "%s: Assertion `%s' failed.", ap=0x7fffffffa8e8) at ../../src/gdb/utils.c:485
>   #8  0x00000000029503b3 in internal_error (file=0x2b976c8 "../../src/gdb/i386-tdep.c", line=8455,
>       fmt=0x2b96d7f "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:55
>   #9  0x000000000122d5b6 in i386_gdbarch_init (info=..., arches=0x0) at ../../src/gdb/i386-tdep.c:8455
>   (More stack frames follow...)
> 
> It turns out that the problem is that the async event handler
> mechanism has been invoked, but this has not yet been initialized.
> 
> If we look at gdb_init (in gdb/top.c) we can indeed see the call to
> gdb_init_signals is after the call to initialize_current_architecture.
> 
> If I reorder the calls, moving gdb_init_signals earlier, then the
> initial error is resolved, however, things are still broken.  I now
> see the same "Quit this debugging session? (y or n)" prompt, but when
> I provide an answer and press return GDB immediately crashes.
> 
> So what's going on now?  The next problem is that the call_readline
> field within the current_ui structure is not initialized, and this
> callback is invoked to process the reply I entered.
> 
> The problem is that call_readline is setup as a result of calling
> set_top_level_interpreter, which is called from captured_main_1.
> Unfortunately, set_top_level_interpreter is called after gdb_init is
> called.
> 
> I wondered how to solve this problem for a while, however, I don't
> know if there's an easy "just reorder some lines" solution here.
> Looking through captured_main_1 there seems to be a bunch of
> dependencies between printing various things, parsing config files,
> and setting up the interpreter.  I'm sure there is a solution hiding
> in there somewhere.... I'm just not sure I want to spend any longer
> looking for it.
> 
> So.
> 
> I propose a simpler solution, more of a hack/work-around.  In utils.c
> we already have a function filtered_printing_initialized, this is
> checked in a few places within internal_vproblem.  In some of these
> cases the call gates whether or not GDB will query the user.
> 
> My proposal is to add a new readline_initialized function, which
> checks if the current_ui has had readline initialized yet.  If this is
> not the case then we should not attempt to query the user.
> 
> After this change GDB prints the error message, the backtrace, and
> then aborts (including dumping core).  This actually seems pretty sane
> as, if GDB has not yet made it through the initialization then it
> doesn't make much sense to allow the user to say "no, I don't want to
> quit the debug session" (I think).

I think this is reasonable.  In theory, the internal error could be for
something you don't care about, and you could still want to continue.
But there's also a limit to the far-fetched theoritical scenarios we
must support.  So I think it is ok to say that any internal error during
initialization is fatal.  The patch LGTM.

Simon