Message ID | f4b3914a-7c00-1fd8-021c-aef0de2b4025@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 89954 invoked by alias); 20 Jan 2020 15:48:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 89886 invoked by uid 89); 20 Jan 2020 15:48:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=H*f:sk:83wo9s4, H*f:sk:f70b115, UD:FIELD, Gnulib X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (207.211.31.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 20 Jan 2020 15:48:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579535308; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=1r/j6A7UJC1YCgluq8gxKsnctPkfPV/QJKsspvOO9+A=; b=ESzwPB11kHNh4duJEys7TCanBkXhoLLmJpziKExVy/3ZBBSsFPI33FdTxRqccZ4H090/8L DT9isELCpzsxzl1y66poItjMepAVJKiFbytxfxPRtR/tw4nW6e/jn3v9N+gbLO5uzJNjRJ gX9lPkQtRDnCT8P7AGkWyuV4CflwSos= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-231-NKRSexYEMzORasntG2FZSA-1; Mon, 20 Jan 2020 10:48:25 -0500 Received: by mail-wr1-f70.google.com with SMTP id z14so14411004wrs.4 for <gdb-patches@sourceware.org>; Mon, 20 Jan 2020 07:48:25 -0800 (PST) Return-Path: <palves@redhat.com> Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id k8sm47734313wrl.3.2020.01.20.07.48.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Jan 2020 07:48:23 -0800 (PST) Subject: Re: [PATCH 2/3] Consistently use BFD's time To: Eli Zaretskii <eliz@gnu.org>, Tom Tromey <tromey@adacore.com> References: <20200114210956.25115-1-tromey@adacore.com> <20200114210956.25115-3-tromey@adacore.com> <83wo9s4sac.fsf@gnu.org> <f70b115f-bbae-c5c0-1d62-ae55eae84f62@redhat.com> <CAPTJ0XEGNC4Fa6FqV8XBvTQ9Syf26zE=EsCnHjJcqfNdAfc_=Q@mail.gmail.com> <83d0bi348d.fsf@gnu.org> <871rrygco6.fsf@tromey.com> <874kwteraa.fsf@tromey.com> <83v9p919hy.fsf@gnu.org> Cc: cbiesinger@google.com, gdb-patches@sourceware.org From: Pedro Alves <palves@redhat.com> Message-ID: <f4b3914a-7c00-1fd8-021c-aef0de2b4025@redhat.com> Date: Mon, 20 Jan 2020 15:48:22 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <83v9p919hy.fsf@gnu.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="------------C422D47019E9AB4AA1FBCF15" |
Commit Message
Pedro Alves
Jan. 20, 2020, 3:48 p.m. UTC
On 1/18/20 7:58 AM, Eli Zaretskii wrote: >> From: Tom Tromey <tromey@adacore.com> >> Cc: Eli Zaretskii <eliz@gnu.org>, Christian Biesinger <cbiesinger@google.com>, palves@redhat.com, gdb-patches@sourceware.org >> Date: Fri, 17 Jan 2020 13:55:57 -0700 >> >>>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes: >> >>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes: >> Eli> I wonder whether a better way is not to import the Gnulib 'stat' and >> Eli> 'fstat' modules at all. Are they required by other Gnulib modules, >> Eli> and if so, by which ones? >> >> Tom> I am wondering this as well. While I think we can track down the bad >> Tom> spots -- either calling the "wrong" stat or incorrectly comparing values >> Tom> from the different stats (the patches I sent probably accomplish the >> Tom> latter already) -- it seems fragile to rely on this. >> >> It came in originally in a patch I sent and one that Yao sent: >> >> https://sourceware.org/ml/gdb-patches/2013-11/msg00502.html > > This one removed gdb_stat.h, which had replacements for the S_IS* > macros, something that should be easy to bring back, and doesn't > really justify replacing the functions and the struct's themselves. > >> https://sourceware.org/ml/gdb-patches/2014-11/msg00654.html > > This seems to be about using 'lstat' freely. But I see only one call > to 'lstat' in our sources -- in symfile.c. So maybe having our own > replacement, or even calling 'lstat' conditioned by an #ifdef, would > be a good-enough solution. > >> I thought maybe removing these workarounds would be ok, but it seems >> like it can't be done: when I remove stat and lstat from >> update-gnulib.sh, they still appear. >> >> Maybe --avoid=stat will work. > > 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? 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. I played with a workaround along the lines of what I was suggesting earlier, and I couldn't make it work in the forms discussed previously. 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. See prototype patch below, particularly common-types.h, which the place where we include bfd.h for the first time. It builds with my mingw cross compiler, the remaining issue would be looking more in detail to the to_sys_stat conversion function. I've also attached a gnulib fix for an issue I ran into, which will need to go upstream. From 3666298dcbdb817dbd5603dd2073e5788c67cac1 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Mon, 20 Jan 2020 15:40:54 +0000 Subject: [PATCH 1/2] Handle different "struct stat" between GDB and BFD --- gdb/defs.h | 1 - gdb/gdb_bfd.c | 45 +++++++++++++++++++++++++++++++++++++++++---- gdb/gdb_bfd.h | 2 +- gdb/jit.c | 4 ++-- gdb/symfile.c | 2 +- gdbsupport/common-types.h | 13 +++++++++++++ 6 files changed, 58 insertions(+), 9 deletions(-) base-commit: 26916852e189323593102561f5e3e2137b892dec
Comments
On 1/20/20 3:48 PM, Pedro Alves wrote: > 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. > See prototype patch below, particularly common-types.h, which the > place where we include bfd.h for the first time. > > It builds with my mingw cross compiler, the remaining issue would be > looking more in detail to the to_sys_stat conversion function. > > I've also attached a gnulib fix for an issue I ran into, which will > need to go upstream. Forgot to say that I put this on the users/palves/stat branch if you'd like to try it & poke at it. Thanks, Pedro Alves
> 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? > 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 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? 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? 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? Thanks.
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
On 1/20/20 8:50 PM, Pedro Alves wrote: > 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? I'm looking at this some more, and am trying to understand what is really going on. I can't seem to reproduce your original issue, I think because I'm using (32-bit) mingw-w64, while the issue with 32-bit size_t happen on (32-bit) mingw.org instead. Correct? But re-reading your description of the problem on bug-gnulib, I think I get it. BFD already uses AC_SYS_LARGEFILE, wrapped in ACX_LARGEFILE (config/largefile.m4) due to a Solaris issue. Actually, the whole tree uses that -- ld, binutils, bfd, gdb, etc., even our toplevel gnulib/ directory's configure.ac calls it. But then, the gnulib/import/ So maybe the best to do here is to import gnulib with --avoid=largefile, and let the ACX_LARGEFILE in gnulib/'s top configure handle the enabling largefile support in sync with all other top level dirs. I tried that, and confirmed that _FILE_OFFSET_BITS=64 still ends up in gnulib's config.h. The build then fails inside gnulib for me on 32-bit mingw-w64, maybe there's a bug that needs to be fixed, but I'd think this _should_ work. See the users/palves/gnulib-largefile branch. Thanks, Pedro Alves
> 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.
> Cc: tromey@adacore.com, cbiesinger@google.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 21 Jan 2020 13:47:28 +0000 > > On 1/20/20 8:50 PM, Pedro Alves wrote: > > > 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? > > I'm looking at this some more, and am trying to understand > what is really going on. I can't seem to reproduce your original > issue, I think because I'm using (32-bit) mingw-w64, while the issue > with 32-bit size_t happen on (32-bit) mingw.org instead. Correct? It isn't only st_size, it's also the width of the st_?time fields. > So maybe the best to do here is to import gnulib with > --avoid=largefile, and let the ACX_LARGEFILE in gnulib/'s > top configure handle the enabling largefile support in sync > with all other top level dirs. I tried that, > and confirmed that _FILE_OFFSET_BITS=64 still ends up in > gnulib's config.h. The build then fails inside gnulib > for me on 32-bit mingw-w64, maybe there's a bug that needs > to be fixed, but I'd think this _should_ work. AFAIU, largefile is intentionally requested in all MinGW builds, the Gnulib developers explicitly said that was their intent. And largefile then causes the replacement of st_size etc. in struct stat, for consistency. MinGW64 doesn't need all that stuff at all, because their defaults are already set to use 64-bit st_size and 64-bit time_t fields. That's because MinGW64 tossed support for Windows version before Vista long ago, and therefore the incompatible changes Microsoft introduced into MSVCRT.DLL from Vista onwards are of no importance for MinGW64 (and the fragment from their stat.h you've shown is by now just useless ballast that is AFAIU never used in MinGW64). But mingw.org's MinGW still supports the older Windows versions, and the only sane way of doing that is to use 32-bit time_t, which basically means one needs to use the regular implementation of stat, not _stati64 or any other variety. Moreover, using _stati64 instead of stat, as MinGW64 does by default (and Gnulib forces the same on any other MinGW build), is a non-starter for me, because _stati64 didn't exist before Vista. So a GDB built that way will simply refuse to run on any older system, even if you build it on a Windows box that does have _stati64. Having said all that, let me explicitly say that I don't want to put a drag on GDB development, and therefore will not insist that solutions for this kind of problems must compile cleanly with my MinGW toolchain. That is why I never bothered to say anything here about the struct stat problem, and only posted that to the Gnulib list. I know how to hack the build to make it DTRT for mingw.org's MinGW; I did that with the pretest, as soon as I found out the reason for the breakage (in a nutshell, remote debugging with gdbserver was completely broken: GDB would crash as soon as you run the remote target). As long as the problem was limited to mingw.org's MinGW, I kept silence here, and only broke that silence because Tom uncovered what seems to be a similar problem, but one that affects MinGW64 as well. Bottom line: if you have a satisfactory solution for MinGW64, go for it regardless of what it will mean for the MinGW I use; I don't want to complicate this stuff any more than it already is, given that the Gnulib developers rejected what I consider the simplest and the most reliable solution (which would have seamlessly satisfied both varieties of MinGW). Now that I'm aware of the problems with the Gnulib stat replacements, I can work around that very easily. Thanks.
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> I'm not sure about that solution -- won't --avoid=stat mean that
Pedro> we disable any stat gnulib fix for all platforms, instead of just
Pedro> for Windows?
Yeah.
Actually, there is one more option for us, which is to patch gnulib
in-tree. We already have the machinery to do this.
Pedro> So I think we can take advantage of that to be able to make sure that
Pedro> when we include "bfd.h", its functions are declared using the system's
Pedro> stat, which is the same version that bfd is built against.
Pedro> See prototype patch below, particularly common-types.h, which the
Pedro> place where we include bfd.h for the first time.
Pedro> It builds with my mingw cross compiler, the remaining issue would be
Pedro> looking more in detail to the to_sys_stat conversion function.
This looks reasonably promising to me.
Tom
From 224fa934579ca3a1292c2e6a0377aaa9bfecc645 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Mon, 20 Jan 2020 15:40:55 +0000 Subject: [PATCH 2/2] Fix gnulib's lstat replacement in C++ namespace mode Fixes: unittests/string_view-selftests.c: In member function 'gnulib::_gl_lstat_wrapper::operator gnulib::_gl_lstat_wrapper::type() const': unittests/string_view-selftests.c:11432:22: error: expected primary-expression before ';' token return ::rpl_stat; ^ The problem is that the lstat replacement depends on the stat (function) declaration, which is only declared afterwards. The fix is simply to declare lstat after stat. --- gnulib/import/sys_stat.in.h | 66 ++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/gnulib/import/sys_stat.in.h b/gnulib/import/sys_stat.in.h index 9ddd1a8d004..537917b6a51 100644 --- a/gnulib/import/sys_stat.in.h +++ b/gnulib/import/sys_stat.in.h @@ -536,40 +536,6 @@ _GL_WARN_ON_USE (lchmod, "lchmod is unportable - " #endif -#if @GNULIB_LSTAT@ -# if ! @HAVE_LSTAT@ -/* mingw does not support symlinks, therefore it does not have lstat. But - without links, stat does just fine. */ -# if !(defined __cplusplus && defined GNULIB_NAMESPACE) -# define lstat stat -# endif -_GL_CXXALIAS_RPL_1 (lstat, stat, int, (const char *name, struct stat *buf)); -# elif @REPLACE_LSTAT@ -# if !(defined __cplusplus && defined GNULIB_NAMESPACE) -# undef lstat -# define lstat rpl_lstat -# endif -_GL_FUNCDECL_RPL (lstat, int, (const char *name, struct stat *buf) - _GL_ARG_NONNULL ((1, 2))); -_GL_CXXALIAS_RPL (lstat, int, (const char *name, struct stat *buf)); -# else -_GL_CXXALIAS_SYS (lstat, int, (const char *name, struct stat *buf)); -# endif -# if @HAVE_LSTAT@ -_GL_CXXALIASWARN (lstat); -# endif -#elif @GNULIB_OVERRIDES_STRUCT_STAT@ -# undef lstat -# define lstat lstat_used_without_requesting_gnulib_module_lstat -#elif defined GNULIB_POSIXCHECK -# undef lstat -# if HAVE_RAW_DECL_LSTAT -_GL_WARN_ON_USE (lstat, "lstat is unportable - " - "use gnulib module lstat for portability"); -# endif -#endif - - #if @REPLACE_MKDIR@ # if !(defined __cplusplus && defined GNULIB_NAMESPACE) # undef mkdir @@ -781,6 +747,38 @@ _GL_WARN_ON_USE (stat, "stat is unportable - " # endif #endif +#if @GNULIB_LSTAT@ +# if ! @HAVE_LSTAT@ +/* mingw does not support symlinks, therefore it does not have lstat. But + without links, stat does just fine. */ +# if !(defined __cplusplus && defined GNULIB_NAMESPACE) +# define lstat stat +# endif +_GL_CXXALIAS_RPL_1 (lstat, stat, int, (const char *name, struct stat *buf)); +# elif @REPLACE_LSTAT@ +# if !(defined __cplusplus && defined GNULIB_NAMESPACE) +# undef lstat +# define lstat rpl_lstat +# endif +_GL_FUNCDECL_RPL (lstat, int, (const char *name, struct stat *buf) + _GL_ARG_NONNULL ((1, 2))); +_GL_CXXALIAS_RPL (lstat, int, (const char *name, struct stat *buf)); +# else +_GL_CXXALIAS_SYS (lstat, int, (const char *name, struct stat *buf)); +# endif +# if @HAVE_LSTAT@ +_GL_CXXALIASWARN (lstat); +# endif +#elif @GNULIB_OVERRIDES_STRUCT_STAT@ +# undef lstat +# define lstat lstat_used_without_requesting_gnulib_module_lstat +#elif defined GNULIB_POSIXCHECK +# undef lstat +# if HAVE_RAW_DECL_LSTAT +_GL_WARN_ON_USE (lstat, "lstat is unportable - " + "use gnulib module lstat for portability"); +# endif +#endif #if @GNULIB_UTIMENSAT@ /* Use the rpl_ prefix also on Solaris <= 9, because on Solaris 9 our utimensat -- 2.14.5