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 <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);
> > }
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
@@ -217,6 +217,34 @@ 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)
{
@@ -226,9 +254,7 @@ __curthread(void)
: "m" (*(char *)OFFSETOF_CURTHREAD));
return (td);
}
-#define curthread (__curthread())
-#define OFFSETOF_CURPCB 32
static __inline __pure2 struct pcb *
__curpcb(void)
{
@@ -237,8 +263,11 @@ __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) */
pgp2PEvYgRQWf.pgp
Description: PGP signature
