[PATCH] gdb: obtain a lock before reading a separate debug info file

[Andrew Burgess]

Rui Ueyama <rui314@gmail.com> writes:

> Thank you for reviewing!
>
> On Thu, Jul 18, 2024 at 2:57 AM Tom Tromey <tom@tromey.com> wrote:
>>
>> >>>>> "Rui" == Rui Ueyama <rui314@gmail.com> writes:
>>
>> Thanks for the patch.
>>
>> Rui> The mold linker has recently gained the `--separate-debug-file` flag to
>> Rui> create a separate debug info file in the background. We don't want to read
>> Rui> a debug file until it's complete. Since mold locks a debug file while
>> Rui> writing to it, we can avoid this issue by obtaining a shared file lock.

When you say "in the background", this is only while the link is being
performed, right?  It's not running the debug generation at some point
after the link has completed?

I ask because I'm trying to understand when the problem occurs.  A user
starts debugging while the link is still being performed and ends up
reading an incomplete debug info file.  But doesn't the same problem
exist for the executable itself?

>>
>> Sounds reasonable.
>>
>> Rui> +#if HAVE_FLOCK
>> Rui> +  /* The mold linker has a feature to create a separate debug info file in
>> Rui> +     the background. The linker obtains an exclusive file lock until the
>> Rui> +     file is ready. We'll obtain a shared lock to avoid reading a
>> Rui> +     half-written file. This is effectively a no-op for other linkers who
>> Rui> +     don't lock a file. */
>> Rui> +  flock(fileno((FILE *)abfd->iostream), LOCK_SH);
>>
>> Unfortunately I don't think this will work properly, because BFD has a
>> virtual I/O layer, and gdb uses it.  So, for example, a BFD coming from
>> a remote target won't have an ordinary FILE* as an iostream.  (I'm not
>> sure offhand if this field will be NULL in this case or if it will be a
>> pointer to some other kind of object.)
>>
>> I also suspect it isn't ok for code outside of BFD to access these bits
>> in this way.
>>
>> I'm not totally sure of the best way to approach this.  Adding a new
>> iovec entry was my first thought, but adding this to the remote protocol
>> seems like a fair amount of work.  Though perhaps that scenario doesn't
>> matter and some more local-only approach would be fine (i.e., have the
>> various gdb implementations of this method just be no-ops).
>>
>> A change to the iovec would have to be discussed on the binutils list
>> though, as that's where the BFD maintainers are.
>
> Looks like gdb_bfd_open internally handles the raw file descriptor.
> So, how about adding a new boolean parameter, which is false by
> default, to gdb_bfd_open to let it lock a file with flock() if it's
> true? For our purpose, that change should be sufficient. In
> particular, I don't think we need to handle remote protocols because
> the mold linker can only create separate debug info on a local
> filesystem.

mold might be running on the remote system alongside gdbserver in which
case a remote GDB can still try to fetch the debug file from the remote.

Thanks,
Andrew