From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from arjuna.pair.com (arjuna.pair.com [209.68.5.131]) by sourceware.org (Postfix) with ESMTPS id E1E433858C50 for ; Tue, 12 Jul 2022 16:44:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E1E433858C50 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=bitrange.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=bitrange.com Received: by arjuna.pair.com (Postfix, from userid 3006) id 921C88A591; Tue, 12 Jul 2022 12:44:40 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by arjuna.pair.com (Postfix) with ESMTP id 913948A547; Tue, 12 Jul 2022 12:44:40 -0400 (EDT) Date: Tue, 12 Jul 2022 12:44:40 -0400 (EDT) From: Hans-Peter Nilsson X-X-Sender: hp@arjuna.pair.com To: Dimitrije Milosevic cc: Xi Ruoyao , Djordje Todorovic , "gcc-patches@gcc.gnu.org" Subject: Re: Mips: Fix kernel_stat structure size In-Reply-To: Message-ID: References: <56af834718d603a4175a10df0a047d92b0b6d139.camel@xry111.site> User-Agent: Alpine 2.20.16 (BSF 172 2016-09-29) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jul 2022 16:44:42 -0000 On Tue, 12 Jul 2022, Dimitrije Milosevic wrote: > Hi Hans-Peter, > You're right, this is not ok for the O32 ABI. Your change however, broke the functionality > for the N32 ABI. That's just not true. There was no mips support for ASAN in gcc, hence nothing to break for N32. Maybe an anachronism? Or maybe you mean plain backporting to LLVM? Perhaps it needs adjustments for their _FILE_OFFSET_BITS=64 builds? On the other hand, that struct_kernel_stat_sz shouldn't be affected by that - unless that's a misnomer or they get the includes wrong - but then again, the include part was fixed by the patch. Unless mis-merged, as in gcc currently. > AFAIK, the changes like this should go through LLVM first > (yours didn't), As I recall, and judging from the mentioned commit, this would have given them no benefit; no visible error to fix, as they're always building the runtime with for 64-bit file size. But yes, it should have been submitted there eventually. > so I will send out a review, covering both 32 ABIs, meaning that both O32 and N32 ABIs > will be working properly. Very good. Thank you in advance. > As for this change, I'm not sure what should be done? > Should this be committed now, while the LLVM change is cherry-picked once it's committed. > Best regards, > Dimitrije Milosevic > > > From: Hans-Peter Nilsson > Sent: Saturday, July 9, 2022 4:44 PM > To: Xi Ruoyao > Cc: Dimitrije Milosevic ; Djordje Todorovic ; gcc-patches@gcc.gnu.org > Subject: Re: Mips: Fix kernel_stat structure size > ? > On Sat, 9 Jul 2022, Xi Ruoyao wrote: > > > On Fri, 2022-07-08 at 21:42 -0400, Hans-Peter Nilsson wrote: > > > On Fri, 1 Jul 2022, Dimitrije Milosevic wrote: > > > > > > > Fix kernel_stat structure size for non-Android 32-bit Mips. > > > > LLVM currently has this value for the kernel_stat structure size, > > > > as per compiler-rt/lib/sanitizer- > > > > common/sanitizer_platform_limits_posix.h. > > > > This also resolves one of the build issues for non-Android 32-bit > > > > Mips. > > > > > > I insist that PR105614 comment #7 is the way to go, i.e. fix > > > the merge error, avoiding the faulty include that it > > > reintroduced.? Was this tested on O32? > > > > I'm pretty sure it is *not* the way to go. > > > > Sanitizer does not really intercept system call.? It intercepts libc > > stat() or lstat() etc. calls.? So you need to keep struct_kernel_stat_sz > > same as the size of struct stat in libc, i. e. "the size of buffer which > > *libc* stat()-like functions writing into". > > > > The "kernel_" in the name is just misleading. > > You make a sound argument, and I just refer to my old commit > 9f943b2446f2d0a.? Either way, the proof is in the pussing: was > this tested for O32 or not? > > > And, if you still think it should be the way to go, let's submit the > > change to LLVM and get it reviewed properly. > > Sorry, I've no longer interest in mips besides I'd like to see > un-broken what I helped getting in a working state (support ASAN > in gcc for mips O32). > > brgds, H-P