From owner-freebsd-current@FreeBSD.ORG Sun Jul 22 18:01:25 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3CF3E106566C; Sun, 22 Jul 2012 18:01:25 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 8BD338FC08; Sun, 22 Jul 2012 18:01:24 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q6MI1WQA019355; Sun, 22 Jul 2012 21:01:32 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q6MI1K3o062611; Sun, 22 Jul 2012 21:01:20 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q6MI1J5T062610; Sun, 22 Jul 2012 21:01:19 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 22 Jul 2012 21:01:19 +0300 From: Konstantin Belousov To: David Chisnall Message-ID: <20120722180119.GJ2676@deviant.kiev.zoral.com.ua> References: <50097BF0.9010103@FreeBSD.org> <20120721211628.GE2676@deviant.kiev.zoral.com.ua> <6006581B-6B1C-4DFB-8662-3EB35869CA5F@FreeBSD.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="L7qmchFKydRS4b0B" Content-Disposition: inline In-Reply-To: <6006581B-6B1C-4DFB-8662-3EB35869CA5F@FreeBSD.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: Dimitry Andric , freebsd-current@freebsd.org, Kim Culhan Subject: Re: -current build failure X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Jul 2012 18:01:25 -0000 --L7qmchFKydRS4b0B Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable [Why don't you bother to configure your mail client properly ? Answering to email with 500+ long lines is not trivial] On Sun, Jul 22, 2012 at 06:18:12PM +0100, David Chisnall wrote: >=20 > On 21 Jul 2012, at 22:16, Konstantin Belousov wrote: >=20 > > On Sat, Jul 21, 2012 at 04:00:45PM -0400, Kim Culhan wrote: > >> On Fri, Jul 20, 2012 at 11:40 AM, Dimitry Andric wro= te: > >>> On 2012-07-20 16:49, Kim Culhan wrote: > >>>> Seeing this for r:238655 > >>> ... > >>>> In file included from /usr/src/sys/modules/dtrace/dtrace/../../../sy= s/pcpu.h:44: > >>>> ./machine/pcpu.h:226:13: error: indirection of non-volatile null > >>>> pointer will be deleted, not trap > >>>> [-Werror,-Wnull-dereference] > >>>> : "m" (*(char *)OFFSETOF_CURTHREAD)); > >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>>> ./machine/pcpu.h:226:13: note: consider using __builtin_trap() or > >>>> qualifying pointer with 'volatile' > >>>=20 > >>> That's indeed a valid warning from clang, since OFFSETOF_CURTHREAD is > >>> usually zero. It's probably due to recent work on dtrace. I'm not in > >>> the neighborhood of a FreeBSD box right now to verify, but can you > >>> please try to change the cast to "(volatile char *)"? That should fix > >>> the warning. > >>=20 > >> Yes it did, I know there are many considerations wrt to this warning. > >=20 > > This should be equivalent to what you tried. Can you test build and > > boot resulting kernel with this patch ? > >=20 > > diff --git a/sys/amd64/include/pcpu.h b/sys/amd64/include/pcpu.h > > index 5d1fd4d..7b3c934 100644 > > --- a/sys/amd64/include/pcpu.h > > +++ b/sys/amd64/include/pcpu.h > > @@ -217,16 +217,22 @@ extern struct pcpu *pcpup; > > #define PCPU_SET(member, val) __PCPU_SET(pc_ ## member, val) > >=20 > > #define OFFSETOF_CURTHREAD 0 > > +#ifdef __clang__ > > +#define VOLATILE volatile > > +#else > > +#define VOLATILE > > +#endif > > static __inline __pure2 struct thread * > > __curthread(void) > > { > > struct thread *td; > >=20 > > __asm("movq %%gs:%1,%0" : "=3Dr" (td) > > - : "m" (*(char *)OFFSETOF_CURTHREAD)); > > + : "m" (*(VOLATILE char *)OFFSETOF_CURTHREAD)); > > return (td); > > } > > #define curthread (__curthread()) > > +#undef VOLATILE > >=20 > > #define OFFSETOF_CURPCB 32 > > static __inline __pure2 struct pcb * >=20 > The following will generate better code, not eliminate future optimisatio= n opportunities, and work correctly on both 32-bit and 64-bit x86. =20 >=20 > #define GS_RELATIVE __attribute__((address_space(256))) > static __inline __pure2 struct thread * > __curthread(void) > { >=20 > return (*(struct thread *volatile GS_RELATIVE*)OFFSETOF_CURTHREAD); > } >=20 > The volatile that clang recommends is very important, because the compile= r (in this version) is not aware of changes to GS and so must assume that i= t can change between calls. In this specific case it is fine, and that's w= hy it's a warning and not an error. Both clang and gcc accept the volatile= and both have the same behaviour. Adding the volatile just for clang will= pessimise the code to silence a warning. This is not the correct thing to= do. I cannot understand what you are trying to say there. I indeed consider adding a volatile to shut down a pointless warning a waste. And on the other hand clang insist on issuing a warning which breaks the build. The function is _guaranteed_ to return the same value any time it is called in context of the current thread. In particular, 'gs changing' is completely irrelevant. First, segment index loaded into %gs itself does not change, even between threads. Second, we guarantee that the basing performed over the %gs is constant for current thread. Why should we say to clang that %gs base can change there, when it is not, is beyond me. Side note, i386 uses %fs and not %gs for pcpu basing. >=20 > I have tested this with clang and gcc. With gcc, a trivial function that= calls __curthread() twice results in two gs-relativel movs either without = __pure2 or with __pure2 and volatile. With clang, both forms produce a sin= gle one, but the inline asm one is passing inline asm through the LLVM IR a= nd so will be losing some other optimisation opportunities. The presence o= f __pure2 does make a difference for clang (gcc ignores it), but removing t= he volatile has the same effect). The following version preserves the exac= t intention of the function for the entire optimisation pipeline: >=20 > #if __clang__ > #pragma clang diagnostic push > #pragma clang diagnostic ignored "-Wnull-dereference" > inline struct thread * I wonder if there shall be static keyword as well. > __curthread2(void) > { >=20 > return (*(struct thread *GS_RELATIVE*)OFFSETOF_CURTHREAD); > } > #pragma clang diagnostic pop > #endif >=20 > The point of warnings is to tell you when you are doing something that is= usually wrong. The correct response is to disable them when you have audi= ted the code and determined that this is one of the exceptional cases. So it still cannot work without disabling the warning ? Not much different from what I noted initially. I am fine with whatever you end up under #ifdef clang, please commit a patch. --L7qmchFKydRS4b0B Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlAMP+8ACgkQC3+MBN1Mb4gQMQCfV/9Vdfusv1bqUkxJ0AUGUe22 4MEAoOnO9qAbrH/8IQqSOvI5PQqB8oM1 =hmVv -----END PGP SIGNATURE----- --L7qmchFKydRS4b0B--