Re: -current build failure

From: David Chisnall <theraven_at_FreeBSD.org>
Date: Sun, 22 Jul 2012 18:18:12 +0100
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_at_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'
>>> 
>>> 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.
>> 
>> Yes it did, I know there are many considerations wrt to this warning.
> 
> This should be equivalent to what you tried. Can you test build and
> boot resulting kernel with this patch ?
> 
> 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
> _at__at_ -217,16 +217,22 _at__at_ extern struct pcpu *pcpup;
> #define	PCPU_SET(member, val)	__PCPU_SET(pc_ ## member, val)
> 
> #define	OFFSETOF_CURTHREAD	0
> +#ifdef __clang__
> +#define	VOLATILE	volatile
> +#else
> +#define	VOLATILE
> +#endif
> static __inline __pure2 struct thread *
> __curthread(void)
> {
> 	struct thread *td;
> 
> 	__asm("movq %%gs:%1,%0" : "=r" (td)
> -	    : "m" (*(char *)OFFSETOF_CURTHREAD));
> +	    : "m" (*(VOLATILE char *)OFFSETOF_CURTHREAD));
> 	return (td);
> }
> #define	curthread		(__curthread())
> +#undef VOLATILE
> 
> #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.  

#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
Received on Sun Jul 22 2012 - 15:18:21 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:29 UTC