From owner-freebsd-current@FreeBSD.ORG Mon Jul 23 19:19:01 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 614161065670; Mon, 23 Jul 2012 19:19:01 +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 A70CE8FC12; Mon, 23 Jul 2012 19:19:00 +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 q6NJJ8cX082494; Mon, 23 Jul 2012 22:19:08 +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 q6NJIuSo038475; Mon, 23 Jul 2012 22:18:56 +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 q6NJIu4O038474; Mon, 23 Jul 2012 22:18:56 +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: Mon, 23 Jul 2012 22:18:56 +0300 From: Konstantin Belousov To: David Chisnall Message-ID: <20120723191856.GR2676@deviant.kiev.zoral.com.ua> References: <50097BF0.9010103@FreeBSD.org> <20120721211628.GE2676@deviant.kiev.zoral.com.ua> <6006581B-6B1C-4DFB-8662-3EB35869CA5F@FreeBSD.org> <20120722180119.GJ2676@deviant.kiev.zoral.com.ua> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Qxd4aqMUuikGjl19" Content-Disposition: inline In-Reply-To: <20120722180119.GJ2676@deviant.kiev.zoral.com.ua> 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: Kim Culhan , freebsd-current@freebsd.org, Dimitry Andric 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: Mon, 23 Jul 2012 19:19:01 -0000 --Qxd4aqMUuikGjl19 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Jul 22, 2012 at 09:01:19PM +0300, Konstantin Belousov wrote: > [Why don't you bother to configure your mail client properly ? > Answering to email with 500+ long lines is not trivial] >=20 > 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 w= rote: > > >>> 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 * > >=20 > > The following will generate better code, not eliminate future optimisat= ion 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); > > } Did you ever looked what clang does generate for this function ? I got my bad mood immediately improved after I saw that: 0xffffffff804d0883 <+691>: mov %gs:0x0,%rax 0xffffffff804d088c <+700>: mov %gs:0x8,%rax > >=20 > > The volatile that clang recommends is very important, because the compi= ler (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 volati= le and both have the same behaviour. Adding the volatile just for clang wi= ll pessimise the code to silence a warning. This is not the correct thing = to do. >=20 > 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. >=20 > 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. >=20 > 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 th= at calls __curthread() twice results in two gs-relativel movs either withou= t __pure2 or with __pure2 and volatile. With clang, both forms produce a s= ingle 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 ex= act 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. >=20 > > __curthread2(void) > > { > >=20 > > return (*(struct thread *GS_RELATIVE*)OFFSETOF_CURTHREAD); > > } > > #pragma clang diagnostic pop > > #endif And there, clang generates 0xffffffff804d088c <+700>: mov %gs:0x8,%rax This is with the nice patch at the end of this reply. The code causes panic at the pmap init time. Longer description is that pc_curthread is offset 0 if %gs-based. The dereferenced pointer point to the struct thread, which contains td_proc pointer at offset 8. Instead, clang seems to dereference td_proc from offset 8 based on %gs, or something similar. pooma% clang -v FreeBSD clang version 3.1 (branches/release_31 156863) 20120523 Target: x86_64-unknown-freebsd9.0 I am quite impressed. > >=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 au= dited the code and determined that this is one of the exceptional cases. >=20 > 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. diff --git a/sys/amd64/include/pcpu.h b/sys/amd64/include/pcpu.h index 5d1fd4d..dc0d828 100644 --- a/sys/amd64/include/pcpu.h +++ b/sys/amd64/include/pcpu.h @@ -217,6 +217,34 @@ extern struct pcpu *pcpup; #define PCPU_SET(member, val) __PCPU_SET(pc_ ## member, val) =20 #define OFFSETOF_CURTHREAD 0 +#define OFFSETOF_CURPCB 32 + +#ifdef __clang__ + +#define GS_BASED __attribute__((address_space(256))) + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wnull-dereference" + +static __inline struct thread * +__curthread(void) +{ + + return (*(struct thread *volatile GS_BASED *)OFFSETOF_CURTHREAD); +} + +static __inline struct pcb * +__curpcb(void) +{ + + return (*(struct pcb *GS_BASED *)OFFSETOF_CURPCB); +} + +#pragma clang diagnostic pop +#undef GS_RELATIVE + +#else /* __clang__ */ + static __inline __pure2 struct thread * __curthread(void) { @@ -226,9 +254,7 @@ __curthread(void) : "m" (*(char *)OFFSETOF_CURTHREAD)); return (td); } -#define curthread (__curthread()) =20 -#define OFFSETOF_CURPCB 32 static __inline __pure2 struct pcb * __curpcb(void) { @@ -237,8 +263,11 @@ __curpcb(void) __asm("movq %%gs:%1,%0" : "=3Dr" (pcb) : "m" (*(char *)OFFSETOF_CURPCB)); return (pcb); } -#define curpcb (__curpcb()) =20 +#endif /* __clang__ */ + +#define curthread (__curthread()) +#define curpcb (__curpcb()) #define IS_BSP() (PCPU_GET(cpuid) =3D=3D 0) =20 #else /* !lint || defined(__GNUCLIKE_ASM) && defined(__GNUCLIKE___TYPEOF) = */ --Qxd4aqMUuikGjl19 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlANo58ACgkQC3+MBN1Mb4hr6ACgsHUzmQjA/VzkZSJwp9MPSQg6 4Z8AoNBzbtrOLSl+HIbazNudhqtA40Gz =NWSC -----END PGP SIGNATURE----- --Qxd4aqMUuikGjl19--