Date: Mon, 23 Jul 2012 22:18:56 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: David Chisnall <theraven@freebsd.org> Cc: Kim Culhan <w8hdkim@gmail.com>, freebsd-current@freebsd.org, Dimitry Andric <dim@freebsd.org> Subject: Re: -current build failure Message-ID: <20120723191856.GR2676@deviant.kiev.zoral.com.ua> In-Reply-To: <20120722180119.GJ2676@deviant.kiev.zoral.com.ua> References: <CAKZxVQV5xhFDN_WbTk-EMoQ18N8u1f4YhqKSJQFUzbX4NZxhUA@mail.gmail.com> <50097BF0.9010103@FreeBSD.org> <CAKZxVQXC6DuX5UTn3goNM9toxSVkP8-7bazTk%2Ba7yLEy7RsJYA@mail.gmail.com> <20120721211628.GE2676@deviant.kiev.zoral.com.ua> <6006581B-6B1C-4DFB-8662-3EB35869CA5F@FreeBSD.org> <20120722180119.GJ2676@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <dim@freebsd.org> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120723191856.GR2676>