From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52853 invoked by alias); 20 Jan 2020 20:50:34 -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 52845 invoked by uid 89); 20 Jan 2020 20:50:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=reusable, reality, redefines, fighting 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 20:50:23 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579553422; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ary8Qtabi8GsYNJ3J+xU4rN7JN5v53hNV6ooZ3xzclM=; b=CwiwccYW6/Sl5rEsde6FKysZVuec+vbhHFjp1tae6BNoPuUl9rydNoYhO1lwxk24ixWSHg g0wNidhdr7B+QnD9I8XjTIenJqWltejmoBO2FTsyxPj4xofrwRb1EpcVAJB+s9oLf0V83+ 6jD0mRzaPkxqKMVPCzo7nRZYUWgPfR0= 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-163-RBDJS_FFOuuHj2LrvWKy_w-1; Mon, 20 Jan 2020 15:50:21 -0500 Received: by mail-wr1-f70.google.com with SMTP id k18so314448wrw.9 for ; Mon, 20 Jan 2020 12:50:20 -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 b17sm50353015wrx.15.2020.01.20.12.50.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Jan 2020 12:50:18 -0800 (PST) Subject: Re: [PATCH 2/3] Consistently use BFD's time To: Eli Zaretskii 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> <8336caxciw.fsf@gnu.org> Cc: tromey@adacore.com, cbiesinger@google.com, gdb-patches@sourceware.org From: Pedro Alves Message-ID: <40fa6532-4440-d47b-63db-9485828c791f@redhat.com> Date: Mon, 20 Jan 2020 20:58: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: <8336caxciw.fsf@gnu.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-01/txt/msg00613.txt.bz2 On 1/20/20 5:28 PM, Eli Zaretskii wrote: >> Cc: cbiesinger@google.com, gdb-patches@sourceware.org >> From: Pedro Alves >> 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 >> >> #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 + #include "bfd.h" +typedef struct stat sys_stat; + +#undef __need_system_sys_stat_h + +#include 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