Re: -current build failure

On Sun, Jul 22, 2012 at 09:01:19PM +0300, Konstantin Belousov wrote:
> [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);
> > }
Did you ever looked what clang does generate for this function ? I got my
bad mood immediately improved after I saw that:

   0xffffffff804d0883 <+691>:   mov    %gs:0x0,%rax
   0xffffffff804d088c <+700>:   mov    %gs:0x8,%rax

> > 
> > 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
And there, clang generates
      0xffffffff804d088c <+700>:   mov    %gs:0x8,%rax
This is with the nice patch at the end of this reply. The code causes
panic at the pmap init time.

Longer description is that pc_curthread is offset 0 if %gs-based.
The dereferenced pointer point to the struct thread, which contains
td_proc pointer at offset 8. Instead, clang seems to dereference
td_proc from offset 8 based on %gs, or something similar.

pooma% clang -v
FreeBSD clang version 3.1 (branches/release_31 156863) 20120523
Target: x86_64-unknown-freebsd9.0

I am quite impressed.

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


diff --git a/sys/amd64/include/pcpu.h b/sys/amd64/include/pcpu.h
index 5d1fd4d..dc0d828 100644
--- a/sys/amd64/include/pcpu.h
+++ b/sys/amd64/include/pcpu.h
_at__at_ -217,6 +217,34 _at__at_ extern struct pcpu *pcpup;
 #define	PCPU_SET(member, val)	__PCPU_SET(pc_ ## member, val)
 
 #define	OFFSETOF_CURTHREAD	0
+#define	OFFSETOF_CURPCB		32
+
+#ifdef __clang__
+
+#define GS_BASED __attribute__((address_space(256)))
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wnull-dereference"
+
+static __inline struct thread *
+__curthread(void)
+{
+
+	return (*(struct thread *volatile GS_BASED *)OFFSETOF_CURTHREAD);
+}
+
+static __inline struct pcb *
+__curpcb(void)
+{
+
+	return (*(struct pcb *GS_BASED *)OFFSETOF_CURPCB);
+}
+
+#pragma clang diagnostic pop
+#undef GS_RELATIVE
+
+#else /* __clang__ */
+
 static __inline __pure2 struct thread *
 __curthread(void)
 {
_at__at_ -226,9 +254,7 _at__at_ __curthread(void)
 	    : "m" (*(char *)OFFSETOF_CURTHREAD));
 	return (td);
 }
-#define	curthread		(__curthread())
 
-#define	OFFSETOF_CURPCB		32
 static __inline __pure2 struct pcb *
 __curpcb(void)
 {
_at__at_ -237,8 +263,11 _at__at_ __curpcb(void)
 	__asm("movq %%gs:%1,%0" : "=r" (pcb) : "m" (*(char *)OFFSETOF_CURPCB));
 	return (pcb);
 }
-#define	curpcb		(__curpcb())
 
+#endif /* __clang__ */
+
+#define	curthread	(__curthread())
+#define	curpcb		(__curpcb())
 #define	IS_BSP()	(PCPU_GET(cpuid) == 0)
 
 #else /* !lint || defined(__GNUCLIKE_ASM) && defined(__GNUCLIKE___TYPEOF) */