On 2024/01/05 16:30, Aditya Gupta wrote:
> Currently for most gdb_interface call with a non-null file pointer,
> GDB's output stream is replaced with the passed file pointer
>
> 'info threads', which is a gdb passthrough, fails to print all thread
> after support was added to get registers from crash_target:
>
> Id Target Id Frame
> 1 CPU 0 <unavailable> in ?? ()
> 2 CPU 1
>
> Also, it was noticed that 'info threads' will print all threads when
> 'info threads' is run 2nd and subsequent times.
>
> This incomplete output of 'info threads' was due to a subtle bug in
> gdb_interface. For any passthrough, it switches the gdb output and
> stderr streams to another FILE* passed in `struct gnu_request`.
> But it does not restore the original FILE* stream gdb was using.
>
> For each thread, which does not have it's registers yet, gdb goes to
> crash_target::fetch_registers to ask for registers. Which in turn might
> call 'datatype_info', which causes gdb's output stream to be set to
> '/dev/null'
>
> So all output after the call to datatype_info will go to /dev/null, and
> hence the data to be printed is lost.
>
> When 'info threads' is run a second time, it sets GDB's output streams
> to something which is NOT /dev/null, and since it already has registers
> for each thread, it does not go to 'crash_target::fetch_registers' and
> hence the output stream is consistent, and we get complete output
>
> So what happened is this:
> 1. GDB prints thread 1 (CPU 0) correctly since it had it's registers
> cached during initialisation in 'crash_target_init' (at that times
> registers where still unavailable, so it shows unavailable)
> 2. At CPU 1, to print the frame, it goes to 'crash_target::fetch_registers'
> to get the registers
> 3. crash_target then calls 'machdep->get_cpu_reg' which finally ends up
> at 'get_netdump_regs' which uses 'datatype_info' to get info of 'elf_prstatus.pr_reg'
> 4. Inside 'datatype_info', we set the file stream to /dev/null
> (pc->nullfp). Then it passes this request object to gdb_interface
> 5. gdb_interface replaces GDB's output stream to the passed stream, ie.
> /dev/null
> 6. Now, ALL FUTURE WRITES EVEN BY GDB, SUCH AS FOR THE 'info threads'
> COMMAND GOES TO THIS STREAM gnu_request->fp, WHICH is /dev/null,
> until someone else sets the non-null stream later
> 7. Similar behaviour occurs with all other threads
> 8. And hence we lose all subsequent output by GDB also
>
> When 'info threads' is run a second time:
> 1. 'gdb_pass_through' passes a default FILE* to 'gdb_interface', which
> is not /dev/null
> 2. Since the registers are cached for all threads now, it does not go
> back to 'crash_target' for registers, and hence no call is made to
> 'datatype_info', hence GDB's output stream is not changing
> 3. Since the gdb's stream, even though not what it was using originally,
> it's still valid, and not '/dev/null', hence we get the complete output
>
> Fix this by restoring the original output streams, after gdb_interface
> has handled the output
>
> Signed-off-by: Aditya Gupta <adityag@xxxxxxxxxxxxx>
> ---
> gdb-10.2.patch | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/gdb-10.2.patch b/gdb-10.2.patch
> index 8c1b43eb07b7..a4e45fa0e6fa 100644
> --- a/gdb-10.2.patch
> +++ b/gdb-10.2.patch
> @@ -1484,13 +1484,29 @@ exit 0
> + }
> +}
> +#endif
> +--- gdb-10.2/gdb/ui-file.c.orig
> ++++ gdb-10.2/gdb/ui-file.c
> +@@ -161,6 +161,12 @@ stdio_file::~stdio_file ()
> + fclose (m_file);
> + }
> +
> ++FILE*
> ++stdio_file::get_stream(void)
> ++{
> ++ return m_file;
> ++}
> ++
> + void
> + stdio_file::set_stream (FILE *file)
> + {
Please move this hunk to the end of gdb-10.2.patch.
> --- gdb-10.2/gdb/ui-file.h.orig
> +++ gdb-10.2/gdb/ui-file.h
> -@@ -195,10 +195,10 @@ class stdio_file : public ui_file
> +@@ -195,10 +195,11 @@ class stdio_file : public ui_file
>
> bool can_emit_style_escape () override;
>
> -private:
> ++ FILE *get_stream(void);
> /* Sets the internal stream to FILE, and saves the FILE's file
> descriptor in M_FD. */
> void set_stream (FILE *file);
> @@ -3267,3 +3283,31 @@ exit 0
>
> for (compunit_symtab *cust : objfile->compunits ())
> {
Please 'add' this patch to the end of gdb-10.2.patch, instead of
changing the existing hunk.
We adopt addition-only policy for gdb-10.2.patch [1] to not make the
patch and branch handling too complicated.
[1] https://github.com/crash-utility/crash/wiki#writing-patches
Thanks,
Kazu
> +--- gdb-10.2/gdb/symtab.c.orig
> ++++ gdb-10.2/gdb/symtab.c
> +@@ -6964,8 +6964,12 @@ void
> + gdb_command_funnel_1(struct gnu_request *req)
> + {
> + struct symbol *sym;
> ++ FILE *original_stdout_stream = nullptr;
> ++ FILE *original_stderr_stream = nullptr;
> +
> + if (req->command != GNU_VERSION && req->command != GNU_USER_PRINT_OPTION) {
> ++ original_stdout_stream = (dynamic_cast< stdio_file * >gdb_stdout)->get_stream();
> ++ original_stderr_stream = (dynamic_cast< stdio_file * >gdb_stderr)->get_stream();
> + (dynamic_cast<stdio_file *>gdb_stdout)->set_stream(req->fp);
> + (dynamic_cast<stdio_file *>gdb_stderr)->set_stream(req->fp);
> + }
> +@@ -7068,6 +7072,12 @@ gdb_command_funnel_1(struct gnu_request *req)
> + req->flags |= GNU_COMMAND_FAILED;
> + break;
> + }
> ++
> ++ /* Restore the streams gdb output was using */
> ++ if (original_stdout_stream)
> ++ (dynamic_cast<stdio_file *>gdb_stdout)->set_stream(original_stdout_stream);
> ++ if (original_stderr_stream)
> ++ (dynamic_cast<stdio_file *>gdb_stderr)->set_stream(original_stderr_stream);
> + }
> +
> + /*
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
