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