From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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 ; Mon, 20 Jan 2020 07:48:25 -0800 (PST) Return-Path: 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 , Tom Tromey References: <20200114210956.25115-1-tromey@adacore.com> <20200114210956.25115-3-tromey@adacore.com> <83wo9s4sac.fsf@gnu.org> <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 Message-ID: Date: Mon, 20 Jan 2020 15:52:00 -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" X-SW-Source: 2020-01/txt/msg00609.txt.bz2 This is a multi-part message in MIME format. --------------C422D47019E9AB4AA1FBCF15 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-length: 8321 On 1/18/20 7:58 AM, Eli Zaretskii wrote: >> From: Tom Tromey >> Cc: Eli Zaretskii , Christian Biesinger , palves@redhat.com, gdb-patches@sourceware.org >> Date: Fri, 17 Jan 2020 13:55:57 -0700 >> >>>>>>> "Tom" == Tom Tromey writes: >> >>>>>>> "Eli" == Eli Zaretskii 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 #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 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(-) diff --git a/gdb/defs.h b/gdb/defs.h index 1ad52feb1f8..f38b9dc6ff5 100644 --- a/gdb/defs.h +++ b/gdb/defs.h @@ -34,7 +34,6 @@ #undef PACKAGE_TARNAME #include -#include "bfd.h" #include #include diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c index 5a6dee2d51a..82b6a6bbaa2 100644 --- a/gdb/gdb_bfd.c +++ b/gdb/gdb_bfd.c @@ -66,7 +66,7 @@ struct gdb_bfd_data needs_relocations (0), crc_computed (0) { - struct stat buf; + sys_stat buf; if (bfd_stat (abfd, &buf) == 0) { @@ -355,24 +355,61 @@ gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream) return 0; } +/* Convert between a struct stat (potentially a gnulib replacement) + and a sys_stat (the system's struct stat). */ + +static sys_stat +to_sys_stat (struct stat *st) +{ + sys_stat sst {}; + +#define COPY(FIELD) \ + sst.FIELD = st->FIELD + + COPY (st_dev); + COPY (st_ino); + COPY (st_mode); + COPY (st_nlink); + COPY (st_uid); + COPY (st_gid); + COPY (st_rdev); + + /* Check for overflow? */ + COPY (st_size); + + // Should probably check _GL_WINDOWS_STAT_TIMESPEC and refer to + // st_atim, etc. instead. + COPY (st_atime); + COPY (st_mtime); + COPY (st_ctime); + +#undef COPY + + return sst; +} + /* Wrapper for target_fileio_fstat suitable for passing as the STAT_FUNC argument to gdb_bfd_openr_iovec. */ static int gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream, - struct stat *sb) + sys_stat *sb) { int fd = *(int *) stream; int target_errno; int result; - result = target_fileio_fstat (fd, sb, &target_errno); + struct stat st; + + result = target_fileio_fstat (fd, &st, &target_errno); if (result == -1) { errno = fileio_errno_to_host (target_errno); bfd_set_error (bfd_error_system_call); } + *sb = to_sys_stat (&st); + return result; } @@ -818,7 +855,7 @@ gdb_bfd_openr_iovec (const char *filename, const char *target, void *stream), int (*stat_func) (struct bfd *abfd, void *stream, - struct stat *sb)) + sys_stat *sb)) { bfd *result = bfd_openr_iovec (filename, target, open_func, open_closure, diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h index 9b1e292bf18..f0ad4814a80 100644 --- a/gdb/gdb_bfd.h +++ b/gdb/gdb_bfd.h @@ -154,7 +154,7 @@ gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target, void *stream), int (*stat_func) (struct bfd *abfd, void *stream, - struct stat *sb)); + sys_stat *sb)); /* A wrapper for bfd_openr_next_archived_file that initializes the gdb-specific reference count. */ diff --git a/gdb/jit.c b/gdb/jit.c index eeaab70bfe0..976f8555250 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -124,11 +124,11 @@ mem_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf, /* For statting the file, we only support the st_size attribute. */ static int -mem_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb) +mem_bfd_iovec_stat (struct bfd *abfd, void *stream, sys_stat *sb) { struct target_buffer *buffer = (struct target_buffer*) stream; - memset (sb, 0, sizeof (struct stat)); + memset (sb, 0, sizeof (sys_stat)); sb->st_size = buffer->size; return 0; } diff --git a/gdb/symfile.c b/gdb/symfile.c index f7bada75f35..65d342a53dd 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1260,7 +1260,7 @@ separate_debug_file_exists (const std::string &name, unsigned long crc, { unsigned long file_crc; int file_crc_p; - struct stat parent_stat, abfd_stat; + sys_stat parent_stat, abfd_stat; int verified_as_different; /* Find a separate debug info file as if symbols would be present in diff --git a/gdbsupport/common-types.h b/gdbsupport/common-types.h index 61099b4e25d..8c136212c80 100644 --- a/gdbsupport/common-types.h +++ b/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 + #include "bfd.h" +typedef struct stat sys_stat; + +#undef __need_system_sys_stat_h + +#include + /* * A byte from the program being debugged. */ typedef bfd_byte gdb_byte; base-commit: 26916852e189323593102561f5e3e2137b892dec -- 2.14.5 --------------C422D47019E9AB4AA1FBCF15 Content-Type: text/x-patch; name="0002-Fix-gnulib-s-lstat-replacement-in-C-namespace-mode.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0002-Fix-gnulib-s-lstat-replacement-in-C-namespace-mode.patc"; filename*1="h" Content-length: 3623 >From 224fa934579ca3a1292c2e6a0377aaa9bfecc645 Mon Sep 17 00:00:00 2001 From: Pedro Alves 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 --------------C422D47019E9AB4AA1FBCF15--