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

[Pedro Alves <palves@redhat.com>]

On 1/20/20 5:28 PM, Eli Zaretskii wrote:
>> Cc: cbiesinger@google.com, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Mon, 20 Jan 2020 15:48:22 +0000
>>
>>> I guess this means some other Gnulib module pulls in 'stat', in which
>>> case --avoid=stat is the way to try to avoid it, yes.  (My guess is
>>> that the 'largefile' module causes 'stat' to be pulled in.)
>>
>> 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
 https://www.gnu.org/software/gnulib/manual/html_node/stat.html
 https://www.gnu.org/software/gnulib/manual/html_node/lstat.html

> 
>> 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.  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.

> 
>> I did come up with a workaround though, it's just different.
>>
>> I noticed that gnulib's sys/stat.h replacement starts with a way to
>> bypass it:
>>
>>  #if defined __need_system_sys_stat_h
>>  /* Special invocation convention.  */
>>
>>  #include_next <sys/stat.h>
>>
>>  #else
>>  /* Normal invocation convention.  */
>>
>>  #ifndef _GL_SYS_STAT_H
>>
>> So I think we can take advantage of that to be able to make sure that
>> when we include "bfd.h", its functions are declared using the system's
>> stat, which is the same version that bfd is built against.
> 
> But stat/fstat the functions will still shadow the system ones, would
> they not?  

Let's look at the patch:

 --- c/gdbsupport/common-types.h
 +++ w/gdbsupport/common-types.h
 @@ -32,8 +32,21 @@ typedef unsigned long long ULONGEST;
  
  #else /* GDBSERVER */
  
 +/* Gnulib may replace struct stat with its own version.  Bfd does not
 +   use gnulib, so when we call into bfd, we must use the system struct
 +   stat.  */
 +#define __need_system_sys_stat_h 1
 +
 +#include <sys/stat.h>
 +
  #include "bfd.h"
  
 +typedef struct stat sys_stat;
 +
 +#undef __need_system_sys_stat_h
 +
 +#include <sys/stat.h>

Currently, without that patch, because of the gnulib struct stat
replacement, when we include bfd.h, we end up with the following
function declared, if you expand the preprocessor macros:

  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:

 binutils-gdb/src/gdb/gdb_bfd.c: In constructor 'gdb_bfd_data::gdb_bfd_data(bfd*)':
 binutils-gdb/src/gdb/gdb_bfd.c:72:29: error: cannot convert 'rpl_stat*' to '_stat64*' for argument '2' to 'int bfd_stat(bfd*, _stat64*)'
      if (bfd_stat (abfd, &buf) == 0)
                              ^

> And if they would, doesn't it mean subtle bugs where, e.g.,
> the Gnulib replacement implementations rely on wide-enough st_size,
> for example, or st_mtime?

I'm not sure what you mean here.  When the replacement implementations
are called, they're passed a replaced struct stat pointer too.  It's
only while the bfd.h header is being compiled that the gnulib replacements
aren't visible.

> 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.

Now, the solution I came up with is reusable for any other library we
may end up depending on that is built with a different struct stat
and requires passing a struct stat in one of its entry pointers.

However, with all that said, bfd is always built along with gdb, so
we have a higher degree of control.  Maybe a better solution for
this is really to make sure that bfd is compiled with largefile
support, as suggested in the bug-gnulib discussion.

So far, I was under the impression that you're reaching the
GNULIB_defined_struct_stat code, where gnulib defines its own
struct stat.  But reading your description of the bug in gnulib
again, I see you're actually getting stat defined to _stati64.

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?

Thanks,
Pedro Alves