Re: [PATCH 1/4] [gdb] Add logging_file::fd

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

On 10/21/26 6:54 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Logging_file is derived from ui_file, which defines a virtual fd member
> Tom> function:
> Tom> ...
> Tom>   /* If this object has an underlying file descriptor, then return it.
> Tom>      Otherwise, return -1.  */
> Tom>   virtual int fd () const
> Tom>   { return -1; }
> Tom> ...
> 
> Tom> Logging_file doesn't define its own fd, so it defaults to -1.
> 
> That test in bt-utils.c (and the other one in event-top.c) look weird to
> me.  It does this:
> 
>    if (gdb_stderr == nullptr || gdb_stderr->fd () == -1)
>      std::ignore = ::write (2, msg, strlen (msg));
>    else
>      gdb_stderr->write_async_safe (msg, strlen (msg));
> 
> But why is that -1 check there?
> 
> Is it because the default ui_file::write_async_safe aborts?
> 
> Since if so then maybe that's the real issue: the API is kind of
> incoherent here.
> 

Hi Tom,

thanks for the review.

AFAIR, the -1 check is indeed there to make sure not to run into the 
default ui_file::write_async_safe, specifically when gdb_stderr is 
temporarily set to a string_file.

> I wonder whether we could simply remove the fd() method entirely and
> instead make the default write_async_safe do something sensible; or make
> it pure virtual and require the subclasses to DTRT.

Removing the fd () method doesn't seem possible due to the use in 
backtrace_symbols_fd (used when using execinfo instead of libbacktrace).

I've submitted an alternative approach ( 
https://sourceware.org/pipermail/gdb-patches/2026-October/231353.html ):
- make the default write_async_safe print to fd ()
- make it return true if the write was handled
- use the return value to decide whether to call the stderr fd fallback.
- add a new method for_each_fd to handle the execinfo case as well

The code is somewhat cleaner, and there's no more direct testing of "fd 
() == -1" in the code, only in the ui_file classes.

I'm not very familiar with this code, so I used two LLMs for review to 
get here.  Unfortunately, they don't agree on what is correct behavior 
for the return value of logging_file::write_async_safe and 
logging_file::for_each_fd, so that probably requires some review attention.

Thanks,
- Tom