Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>  
> Andrew> +  reopen_exec_file ();
>
> This addition could probably use a comment explaining why it is
> needed.

I added a comment.

>
> Andrew> -      new_modtime = new_statbuf.st_mtime;
> Andrew> +      long new_modtime = new_statbuf.st_mtime;
>
> Might as well change this to time_t now.

I fixed this.

>
> I don't really understand how reread_symbols interacts with target:
> files.  It seems like this is just broken.

Yup.

>
> At AdaCore we carry a patch that changes this code to use bfd_stat.
> According to internal notes, this was pushed:
>
> https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00386.html
>
> ... but I don't see it in gdb.  Maybe we never resolved the 'stat' issue
> in gnulib?

I guess not.  I already posted a series to look at this:

  https://inbox.sourceware.org/gdb-patches/cover.1695824275.git.aburgess@redhat.com/

But after reviewing your series I've updated this:

  https://inbox.sourceware.org/gdb-patches/cover.1695909469.git.aburgess@redhat.com/

I didn't fully understand all the discussion w.r.t. gnulib stat vs
system stat, but I'm hoping the change above, which is less that your
originally proposed full change, might be mergable.

Though even after that series reopen_exec_file is still broken for
target: files, I guess I'll see how the above goes before fixing that
too.

> Anyway, the reason I bring this up is that reopen_exec_file calls
> bfd_cache_close_all -- but then the loop in reread_symbols, in the
> bfd_stat case, will reopen the files (BFD uses fstat).  This seems
> unfortunate.
>
> I don't think gdb has a very clear idea of when bfd_cache_close_all
> ought to be called.  It would be good to clear this up.  I'm not very
> clear on it myself.  Maybe it is to avoid ETXTBUSY errors -- in which
> case it seems like it should be called just before starting the
> inferior.  But, ISTR some error on Windows as well, though I don't know
> exactly what... so maybe the files need to be guaranteed-closed in other
> situations as well.

I agree this stuff doesn't seem well defined at all, but I don't think I
made anything particularly worse in this respect, so I'm leaving that as
a problem for the future.

I did take a quick look at when bfd_cache_close_all is currently called,
and we do have a call in symbol_file_add_with_addrs, which is used when
a solib is loaded -- which means in many cases, when an inferior starts
we will end up calling bfd_cache_close_all anyway.  Note: I'm not
arguing that this is an actual well designed solution, just that in many
cases we're not going to hit any problems because of this.

Thanks,
Andrew