Re: -current build failure

[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:
> 
> 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 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.
> 
> 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 *
I wonder if there shall be static keyword as well.

> __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.

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.