From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 7BF5A3858034 for ; Tue, 6 Apr 2021 09:57:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7BF5A3858034 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=vinschen@redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1617703049; h=from:from:reply-to: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=62sMJQvdnURFRveSTYllG4uIdpZXEkX3926YIkbTk7E=; b=Bb5QX6yz5ZRo3QndG/gTUZGTwamHfGRNWVUlZP6iZ6I2jSYpZKpJ5q+aMvG3O6gDcYmsrW 65SZNhzGlyXlFZpb1LSA1vVcEPTlCUglcFzW6M8yuUnAYfPbnXfR2bBqYTUiW4HZ1gf7q2 C29HIK3fB1F/1cfHXk7xDMxT6o62uRs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-169-5WuUUZ3fNf-PKEBp5dgeOg-1; Tue, 06 Apr 2021 05:57:26 -0400 X-MC-Unique: 5WuUUZ3fNf-PKEBp5dgeOg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 15712190D34B; Tue, 6 Apr 2021 09:57:25 +0000 (UTC) Received: from calimero.vinschen.de (ovpn-112-25.ams2.redhat.com [10.36.112.25]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AD720E710; Tue, 6 Apr 2021 09:57:23 +0000 (UTC) Received: by calimero.vinschen.de (Postfix, from userid 500) id 596CCA80773; Tue, 6 Apr 2021 11:57:22 +0200 (CEST) Date: Tue, 6 Apr 2021 11:57:22 +0200 From: Corinna Vinschen To: newlib@sourceware.org Subject: Re: [PATCH] libc: Replace i386/sys/fenv.h symlink with an #include shim Message-ID: Reply-To: newlib@sourceware.org Mail-Followup-To: newlib@sourceware.org, Joel Sherrill References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=vinschen@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Apr 2021 09:57:31 -0000 On Apr 5 09:37, David Macek via Newlib wrote: > Thank you. Below are my notes. I believe some are real issues, but > most are novice questions, so be kind. :) > > > 3835c015d09d Add build mechanism to share common header files between machines > > I assume I only need to check Makefile.am (and acinclude.m4) here. > It's good as far as I can tell, the order among other sources of > header files seems reasonable. > > 1. I'm unsure about the indentation, the file already had been > inconsistent, so I just note that the first hunk doesn't match its > immediate surroundings. Do you mean the indentation between the first if and the following for loops by any chance? There's a mix of 4 and 2 space indentations and I opted for a uniform upper level indentation of 4 spaces. Only the inner if has 2 spaces, as in the surrounding code. Given there's no other outer if in the surrounding code, there was no precedent at this point in the file. I don't care, actually. If anybody wants a 2 space indentation from the outer if to the for loops, ok with me. > 2. I would expect shared_machine_dir to be introduced to > configure.host in this patch as well, in the documentation and in the > initialization block. The first patch actually introduces shared_machine_dir. The second patch introduces its first usage for the first targets. > > e8e95bf6436a configure.host: define shared ix86 and x86_64 directory > > 3. The globs in Makefile.am fail gracefully when they don't match, right? Not sure what you're asking. If shared_machine_dir is empty, the shell code is eqivalent to if [ -n "" ]; then \ for i in $(srcdir)/libc/machine//machine/*.h; do [...] There's nothing *failing* as such. > > 718f3955449f fenv: add missing declarations to x86 fenv.h > > Can't judge. These functions are defined in x86 fenv.c, but they were not declared in the fenv.h header. This patch is adding them to the header, that's all. > > 7803e212adcd fenv: drop Cygwin-specific implementation in favor of newlib code > > 4. A noticeable difference between cygwin's and newlib's fenv.c is > use_sse being a run-time-initialized constant vs a (inline) function. > Are we keeping the latter intentionally, or it doesn't matter? I checked this when I created the patch and AFAICS it doesn't matter. use_sse as a global var requires initialization, while the inline function does not. It's more a question of speed vs. simplicity I guess, but these functions are typically not called in time-critical loops, so the speed factor is likely neglectable. > 5. Is it okay for autoload.cc and dcrt0.cc to still #include "fenv.h" > instead of ? It shouldn't matter, but it was never actually 100% correct to include "fenv.h" with fenv.h being a system header. I'll push a patch changing this in autoload.cc and dropping it entirely from dcrt0.cc. > > 3b79d7e1c31f Cygwin: don't call _feinitialise from _dll_crt0 > > 6. The date in the new fenv.c seems awfully in the past. Do you mean the dates in the comment? Yes, it's pretty long ago. But Cygwin's striving for backward compatibility. If we don't provide this symbol, and *iff* there's still an application running built in this timeframe, it will stop working. > 7. Why can we assume the FP environment is initialized? Is it because > fesetenv() is called in autoload.cc, or is it just a guarantee from > the OS? Good point! The FP environment is initialized by Windows, but it's initialized differently from what the x86 finit/fninit op does. The default precision after finit/fninit is 64 bit, while the Windows default precision is 53 bit. That's the only difference, but it still requires to initialize the FP env prior to calling the application's main function. However, rather than keeping the non-standard _feinitialise call, I change dcrt0.cc to call fesetenv(FE_DFL_ENV) instead, which is more standard. > > Please fetch this change and reset your branch to this new state. > > This should very much simplify reviewing the changes. > > 8. I noticed newlib/libm/fenv/fenv_stub.c still mentions symlinks. Not sure how to change the wording here. In retrospect the text is actually puzzeling me more than it helps. Joel, can you please take a look? I force-pushed the changes in terms of the includes and the call to fesetenv from dcrt0. 3b79d7e1c31f Cygwin: don't call _feinitialise from _dll_crt0 has been replaced with 797a9278bb29 Cygwin: don't export _feinitialise from newlib de6ffe470c44 Cygwin: fix fenv.h includes Please have another look. Thanks, Corinna