Re: [PATCH 2/3] Consistently use BFD's time

[Eli Zaretskii <eliz at gnu dot org>]

> Cc: tromey@adacore.com, cbiesinger@google.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 20 Jan 2020 20:50:17 +0000
> 
> >> I'm not sure about that solution -- won't --avoid=stat mean that
> >> we disable any stat gnulib fix for all platforms, instead of just
> >> for Windows?
> > 
> > It would, but do we have any known problems with stat and fstat on
> > other platforms?
> 
> There's the list of known problems in the gnulib pages:
> 
>  https://www.gnu.org/software/gnulib/manual/html_node/fstat.html

This one is only about Solaris problems with st_mtime etc.  (MSVC
problems are of no concern to us.)

>  https://www.gnu.org/software/gnulib/manual/html_node/stat.html

Is the trailing slash issue important for GDB?

>  https://www.gnu.org/software/gnulib/manual/html_node/lstat.html

Likewise.

All in all, the gain sounds small to me, if at all.

> >> We only have one lstat call, but we also use fstat, for example, and
> >> I assume that these *stat modules in gnulib are all intertwined.
> >> Also, we may only have one lstat call nowadays, but who knows about
> >> the future.
> > 
> > Even having a gdb_lstat for that purpose will be simpler and more
> > future-proof than the whole Gnulib stat module, I think.
> 
> I think one could use that argument for any piece of gnulib in
> isolation.

I wouldn't say "any piece", there are some functions there with very
non-trivial guts.  But yes, the impact of Gnulib IME is mostly
important where it provides missing functions, not where it replaces
existing ones.

> But fighting against use of some particular modules ends up creating
> a larger burden over time IMO.  I'd rather avoid doing that if
> possible.

Sure, this goes without saying.  I was just considering this as one
alternative, since we don't seem to have a much better one.  Or do we?

>   extern int bfd_stat (bfd *, struct rpl_stat *);
> 
> This is incorrect, because that bfd function was not defined
> that way.  It is instead written as:
> 
>   extern int bfd_stat (bfd *, struct stat *);
> 
> Given the stat replacement, we pass it a rpl_stat pointer, when it
> is in reality expecting a "struct stat" one (or whatever that expands
> in the system headers).  So we get undefined behavior at run time.
> 
> By defining __need_system_sys_stat_h just before bfd.h is included,
> we're sure that bfd's bfd_stat is declared with the system's
> stat, just like when bfd itself was compiled.
> 
>   extern int bfd_stat (bfd *, struct stat *);
> 
> We undef  __need_system_sys_stat_h again just after including
> "bfd.h", so that the gdb uses the gnulib struct stat.  But we
> also create the sys_stat typedef so that we can refer to the
> system's stat type after __need_system_sys_stat_h is undefined
> and gnulib's stat replacement is visible.
> 
> So the *stat functions will be shadowed by the gnulib ones
> within gdb, yes.  But we also get a compile error if we
> call bfd_stat with a masked struct stat:

OK, I see.  This would work, of course, but IME solutions based on
specific order of inclusion of header files are fragile, and break
easily, because header files tend to include other header files and
break the order you carefully observed.  But if there's no better
alternative, perhaps this is the way to go.

> > Also, aren't some of the problems on platforms other than MinGW
> > resolved by the Gnulib sys/stat.h header, as opposed to the
> > replacement implementation of the functions themselves?
> 
> Some yes, but not all.  But it's the sys/stat.h header replacement
> that redefines struct stat, so I don't see the point you're making.

And you are sure that including first the system's stat.h and then the
Gnulib's version of it will never cause any compilation problems?

Also, if GDB uses values based on what bfd_stat retrieves, then these
values might be different from what the Gnulib replacement stat
retrieves in GDB's own code for the same file (due to size of the
fields), right?  Are we sure we never compare those expecting them to
match for the same file?

> So perhaps what we need is to enable largefile support by
> default on bfd for mingw, to force use of the 64-bit stat?
> Does the original issue go away if you configure
> with --enable-largefile?  Maybe we need a mingw-specific
> tweak in gdb's src/config/largefile.m4?
> 
> Looking my mingw-w64's stat.h, I see:
> 
>  #if defined(_FILE_OFFSET_BITS) && (_FILE_OFFSET_BITS == 64)
>  #ifdef _USE_32BIT_TIME_T
>  #define stat _stat32i64
>  #define fstat _fstat32i64
>  #else
>  #define stat _stat64
>  #define fstat _fstat64
>  #endif
>  #endif
> 
> So if BFD is compiled with _FILE_OFFSET_BITS == 64 and
> _USE_32BIT_TIME_T is not defined, the mismatch ends up going
> away?  Is there a reason we _wouldn't_ want to enable largefile
> support in bfd?

That's a non-starter for me, as I will explain in response to your
further message.