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 98867388A029 for ; Wed, 7 Apr 2021 09:28:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 98867388A029 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=1617787705; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=NTY3oX7s/ceu5GhZ8hYnwFiUVkDy59N3Zpd0e87MH0I=; b=dYS/X6+JKzyzhpIXjM69B5GwThcEIPjpmKMMggaGh9/h9b8BlaQtURzJcqWbeonNYgj3QU t7wLfO0aNJZjIHuEZCpdbSyUWrSJVvJlGJe0pEDEYq+O4yyR+clOMbZRj75OM3xdqudtjz 3C0bOgpKHfLeXwN55THktB8RDYhA+TU= 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-77-YUYC-bu6OwiJFEV36lGY7Q-1; Wed, 07 Apr 2021 05:28:22 -0400 X-MC-Unique: YUYC-bu6OwiJFEV36lGY7Q-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 934FFC73A2 for ; Wed, 7 Apr 2021 09:28:21 +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 3B3E92B0BB for ; Wed, 7 Apr 2021 09:28:21 +0000 (UTC) Received: by calimero.vinschen.de (Postfix, from userid 500) id D154DA80773; Wed, 7 Apr 2021 11:28:19 +0200 (CEST) Date: Wed, 7 Apr 2021 11:28:19 +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 References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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=-13.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Wed, 07 Apr 2021 09:28:27 -0000 On Apr 7 09:28, David Macek wrote: > > > 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. > > First patch says what to do when shared_machine_dir is defined, second > patch defines shared_machine_dir for some architectures but it's not > declared anywhere. In other words, I'm missing this: > > diff --git a/newlib/configure.host b/newlib/configure.host > index ca0176e778..90416e3fa7 100644 > --- a/newlib/configure.host > +++ b/newlib/configure.host > @@ -33,6 +33,7 @@ > > # It sets the following shell variables: > # newlib_cflags Special CFLAGS to use when building > +# shared_machine_dir Subdirectory of libc/machine to use as base, optional > # machine_dir Subdirectory of libc/machine to configure > # sys_dir Subdirectory of libc/sys to configure > # have_sys_mach_dir Is there a machine subdirectory in sys subdirectory > @@ -54,6 +55,7 @@ > > newlib_cflags= > libm_machine_dir= > +shared_machine_dir= > machine_dir= > sys_dir= > posix_dir= Ah, right. I added that to the first patch. > > > > 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 > > I mean if shared_machine_dir is nonempty, three globs are executed: > > - $(srcdir)/libc/machine/$(shared_machine_dir)/machine/*.h > - $(srcdir)/libc/machine/$(shared_machine_dir)/sys/*.h > - $(srcdir)/libc/machine/$(shared_machine_dir)/include/*.h > > ... while only the .../sys/... one actually matches anything. I have > learned to be wary of such things. But having read the code again, I > guess the following -f test saves the day there, right? Right. The same mechanism is used in the already existing code and not all machine dirs have all these subdirs. > > > > 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. > > OK, sounds good. Are new declarations supposed to be documented somewhere? They aren't new, check other arches. They were just not declared for x86/x86_64, accidentally. > > 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. > > It's not dropped from dcrt0.cc in > de6ffe470c44bc6fcbd60986507a9ec8fdc77a4a, which I think is actually > correct. D'oh! I wrote that prior to checking the fenv initialization and forgot to fix my mail, sorry. > > > 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. > > Ah, okay, so calling _feinitialise(): > > - has never been supported in user code, ACK > - the intentional usage in initialization code injected into programs > has been removed for a long time now, Well, not injected as such, it was just originally part of the crt0.o startup code and later moved into the DLL startup code. > - the intentional usage in cygwin1.dll is now being replaced (as noted below). ACK > > > 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 propose this: > > diff --git a/newlib/libm/fenv/fenv_stub.c b/newlib/libm/fenv/fenv_stub.c > index a4eb652f3e..5a560bf7c5 100644 > --- a/newlib/libm/fenv/fenv_stub.c > +++ b/newlib/libm/fenv/fenv_stub.c > @@ -7,16 +7,16 @@ > /* > * This file is intentionally empty. > * > - * Newlib's build infrastructure needs a machine specific fiel to override > + * Newlib's build infrastructure needs a machine specific file to override > * the generic implementation in the library. When a target > * implementation of the fenv.h methods puts all methods in a single file > * (e.g. fenv.c) or some as inline methods in its , it will need > * to override the default implementation found in a file in this directory. > * > * For each file that the target's machine directory needs to override, > - * this file should be symbolically linked to that specific file name > - * in the target directory. For example, the target may use fe_dfl_env.c > - * from the default implementation but need to override all others. > + * there should be a corresponding stub file in the target directory. > + * To avoid copying this explanation far and wide, #including this fenv_stub.c > + * from the stub files in encouraged. > */ Thanks, I applied this and just changed the overall formatting a bit. Changes force-pushed to topic/shared_arch_headers. Corinna