From owner-freebsd-current@FreeBSD.ORG Sun Jul 22 17:18:21 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 BBB58106566B; Sun, 22 Jul 2012 17:18:21 +0000 (UTC) (envelope-from theraven@FreeBSD.org) Received: from theravensnest.org (theraven.freebsd.your.org [216.14.102.27]) by mx1.freebsd.org (Postfix) with ESMTP id 92F048FC0A; Sun, 22 Jul 2012 17:18:21 +0000 (UTC) Received: from [192.168.0.2] (cpc2-cmbg15-2-0-cust445.5-4.cable.virginmedia.com [86.26.13.190]) (authenticated bits=0) by theravensnest.org (8.14.5/8.14.5) with ESMTP id q6MHIIQN019850 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES128-SHA bits=128 verify=NO); Sun, 22 Jul 2012 17:18:20 GMT (envelope-from theraven@FreeBSD.org) Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=us-ascii From: David Chisnall In-Reply-To: <20120721211628.GE2676@deviant.kiev.zoral.com.ua> Date: Sun, 22 Jul 2012 18:18:12 +0100 Content-Transfer-Encoding: quoted-printable Message-Id: <6006581B-6B1C-4DFB-8662-3EB35869CA5F@FreeBSD.org> References: <50097BF0.9010103@FreeBSD.org> <20120721211628.GE2676@deviant.kiev.zoral.com.ua> To: Konstantin Belousov X-Mailer: Apple Mail (2.1278) 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 17:18:21 -0000 On 21 Jul 2012, at 22:16, Konstantin Belousov wrote: > On Sat, Jul 21, 2012 at 04:00:45PM -0400, Kim Culhan wrote: >> On Fri, Jul 20, 2012 at 11:40 AM, Dimitry Andric = wrote: >>> On 2012-07-20 16:49, Kim Culhan wrote: >>>> Seeing this for r:238655 >>> ... >>>> In file included from = /usr/src/sys/modules/dtrace/dtrace/../../../sys/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 * The following will generate better code, not eliminate future = optimisation opportunities, and work correctly on both 32-bit and 64-bit = x86. =20 #define GS_RELATIVE __attribute__((address_space(256))) static __inline __pure2 struct thread * __curthread(void) { return (*(struct thread *volatile = GS_RELATIVE*)OFFSETOF_CURTHREAD); } The volatile that clang recommends is very important, because the = compiler (in this version) is not aware of changes to GS and so must = assume that it can change between calls. In this specific case it is = fine, and that's why 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 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 single one, but the inline asm one is passing inline asm = through the LLVM IR and so will be losing some other optimisation = opportunities. The presence of __pure2 does make a difference for clang = (gcc ignores it), but removing the volatile has the same effect). The = following version preserves the exact intention of the function for the = entire optimisation pipeline: #if __clang__ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wnull-dereference" inline struct thread * __curthread2(void) { return (*(struct thread *GS_RELATIVE*)OFFSETOF_CURTHREAD); } #pragma clang diagnostic pop #endif 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 = audited the code and determined that this is one of the exceptional = cases. David=