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

Pedro Alves
2020-01-20T20:50:34Z

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