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 <d...@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
> @@ -217,16 +217,22 @@ 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_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
