Date: Sun, 22 Jul 2012 21:01:19 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: David Chisnall <theraven@freebsd.org> Cc: Dimitry Andric <dim@freebsd.org>, freebsd-current@freebsd.org, Kim Culhan <w8hdkim@gmail.com> Subject: Re: -current build failure Message-ID: <20120722180119.GJ2676@deviant.kiev.zoral.com.ua> In-Reply-To: <6006581B-6B1C-4DFB-8662-3EB35869CA5F@FreeBSD.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <dim@freebsd.org> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120722180119.GJ2676>