From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2e.google.com (mail-oa1-x2e.google.com [IPv6:2001:4860:4864:20::2e]) by sourceware.org (Postfix) with ESMTPS id BF812385842A for ; Tue, 1 Nov 2022 12:41:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BF812385842A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x2e.google.com with SMTP id 586e51a60fabf-12c8312131fso16643346fac.4 for ; Tue, 01 Nov 2022 05:41:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:to:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=ZRxMeBLjNvpK7ZXHA5P0c033ve7YnA+HrXNBC5YLjdY=; b=QVWgig+Pz6qqhPSojU+9eNeDlRJfUztDwEjNlc48uUQVkUC2+VUToP6jRkd4R6CCqn hfmjUVncdZmyITENc/1G/dg1d/PjW8QqXLAaCis/CLllBL08yGhGySC3++WpvvLInY/6 8ag6X2Folp0RiyBCMbSleOO+BJVTgXRg0lXYwHImTyYdD5QphxyR4zNHE3tkiL772IbX KOfMnZfJWVn4/sIOuRQA8cTJvRul1kT1gaFpinrBnYIDbJxTLBInDIcBwBoka865MW+7 BO5HGGmzcQ7gMr9kwNExjzY2vTPXHNJR+sotGmtwhhxLg059ubgXgki0SXJX2wnD/6lz KOIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:to:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ZRxMeBLjNvpK7ZXHA5P0c033ve7YnA+HrXNBC5YLjdY=; b=OneF0FeKZmsFWQSF+Gqhdrc2IIgc3Nvz1aj7CME0UlLtikLJmjchwCN2BHoDbyfoMj JAS8qQOvNtoCdnsGdOG1i0J1U43AQEjeETetXgTsNZnHHLsPeV/GddiYs2PvbsbcR8GV RHsuOTlo7nj9CAEjN/PZTZldEg0N7JgBSmE3ZvF3NvQ0caIGVNwi+UFW1XzXPPjmbEfH wjL6PDAGGw3XgpKwzCyVra1o11KR1WL1dXKsO3fTRAR5aUe69GpoFcg2m+InhOK1autr MHRpE0RIhajs7E14XlSKSBtIQY4Zu71jJdMATaVlL4T3NP1nSbjqaZGRjsYHbi2ckaJt yUcw== X-Gm-Message-State: ACrzQf06X3wS2yoFKtRrvuOITEhw+ncwvGb/CXVPvjpHwe6LdkrNjnYS YvvW5gSw+3iCvFJfPenCmvJChw== X-Google-Smtp-Source: AMsMyM7zekTK/nynFzw6a62VBRzGPgPdSblK1wvl24goIpLD3Bdhc77ph6o5ymLViaZJkPDavTT/eQ== X-Received: by 2002:a05:6870:ac26:b0:13c:6ef9:a070 with SMTP id kw38-20020a056870ac2600b0013c6ef9a070mr11005693oab.23.1667306479851; Tue, 01 Nov 2022 05:41:19 -0700 (PDT) Received: from [192.168.15.31] ([191.17.238.148]) by smtp.gmail.com with ESMTPSA id h22-20020a056808015600b00342eade43d4sm3351297oie.13.2022.11.01.05.41.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Nov 2022 05:41:19 -0700 (PDT) Message-ID: <6fe4affc-cce4-ba36-7e7d-2cbb7908a3a6@linaro.org> Date: Tue, 1 Nov 2022 09:40:01 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730) To: YunQiang Su , libc-alpha@sourceware.org References: <20221028205440.1091298-1-aurelien@aurel32.net> <85bbb5fa-735e-254e-4467-e2726fd0ecc3@linaro.org> Content-Language: en-US From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 01/11/22 08:57, Aurelien Jarno wrote: > On 2022-11-01 11:55, Aurelien Jarno wrote: >> On 2022-10-31 23:20, Aurelien Jarno wrote: >>> On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote: >>>> >>>> >>>> On 31/10/22 13:03, Adhemerval Zanella Netto wrote: >>>>> >>>>> >>>>> On 28/10/22 17:54, Aurelien Jarno wrote: >>>>>> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit >>>>>> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore >>>>>> stop calling it from __fstatat for systems with 64-bit time_t. >>>>>> >>>>>> Resolves: BZ #29730 >>>>> >>>>> Although this change is not incorrect, I think the fix I just sent [1] is >>>>> slight better because it just alias non-LFS stat to LFS stat for mips >>>>> (which is also a small runtime and code optimization), while avoiding >>>>> touching generic code (so we need to check if trigger another issue on >>>>> different architecture). >>>>> >>>>> [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html >>>> >>>> So I was wrong that we can assume stat and stat64 are equal on mips. Maybe >>>> instead of adding the fix on stat we can make it more generic and move it >>>> to is_time_t_range (so it can potentially fix usage not only on stat code) >>>> as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ? >>> >>> Yes, we can move it to is_time_t_range, but then it gets trickier to get >>> the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it >>> is not defined for OpenRISC, so it means it will cast to time_t. However >>> this has been explicitly changed that way in commit 6e8a0aac2f883 to >>> unbreak OpenRISC. >>> >>> We probably want a condition that is "kernel ABI uses 32-bit time_t". Do >>> we have something like that already defined? >> >> I guess this can be checked with: >> (__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) >> >> And then we can rename the function to is_ktime_t_range to better >> reflect its behaviour. > > Looking at it more in details, I am not sure it's a good idea, as this > function is also used for non-syscall cases, like in mktime or timegm. > While that works because there is a __TIMESIZE != 64 guard, this still > seems fragile. The "(__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))" is a Linuxism and I agree it should not be used in generic code. > > In that regard, I think the approach from YunQiang to explicitly call > either one or the other function might be better. > > An hybrid solution would be to define both in_time_t_range and > in_ktime_t_range, the later being the one called in_int32_t_range in > YunQiang's patch. > > What do you think? > I am more inclined to either go with your initial patch that handles it on fstatat or go with a mips specific fstatat.c to make clear about MIPS ABI idiosyncrasies (which is something I tried to avoid, but for this specific case seems to make sense): * sysdeps/unix/sysv/linux/mips/mips64/n64/fstatat.c: /* Different than other ABIS, mips64 has different layouts for non-LFS and LFS struct stat. */ int __fstatat (int fd, const char *file, struct stat *buf, int flag) { struct __stat64_t64 st64; int r = __fstatat64_time64 (fd, file, &st64, flag); if (r == 0) { /* Clear internal pad and reserved fields. */ memset (buf, 0, sizeof (*buf)); buf->st_dev = st64.st_dev; buf->st_ino = st64.st_ino; buf->st_mode = st64.st_mode; buf->st_nlink = st64.st_nlink; buf->st_uid = st64.st_uid; buf->st_gid = st64.st_gid; buf->st_rdev = st64.st_rdev; buf->st_size = st64.st_size; buf->st_blksize = st64.st_blksize; buf->st_blocks = st64.st_blocks; buf->st_atim = st64.st_atim; buf->st_mtim = st64.st_mtim; buf->st_ctim = st64.st_ctim; } return r; } We can then treat the YunQiang patch as a code refactor instead.