Date: Sat, 13 May 1995 02:19:01 +1000 From: Bruce Evans <bde@zeta.org.au> To: bde@zeta.org.au, ylo@cs.hut.fi Cc: freebsd-bugs@freefall.cdrom.com, maddox@CS.Berkeley.EDU, maddox@redwood.CS.Berkeley.EDU, shadows-tech@cs.hut.fi Subject: Re: i386/395: CRITICAL PROBLEM: spl functions implemented incorrectly Message-ID: <199505121619.CAA01072@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>> "Accessing a volatile object, modifying an object, modifying a file, or
>> calling a function that does any of these operations are all _side effects_.
>> At ... _sequence points_, all side effects of previous evaluations shall
>> be complete and no side effects of subsequent evaluations shall have
>> taken place".
>You don't say anything about what can be assumed about values of non-volatile
>expressions.
They can be assumed to be non-volatile :-).
>I checked with gcc-2.6.3 (with patches for i386 bugs - I get same
>results with plain 2.6.3). The following code:
>extern int cpl;
>extern int value;
>static __inline int spl1()
>{
> int old = cpl;
> cpl |= 1;
> return old;
>}
>static __inline void splx(int x)
>{
> cpl = x;
>}
>void foo()
>{
> int x, y;
> value *= 2;
> x = spl1();
> value *= 3;
> splx(x);
>}
>When compiled with "gcc -O -S koe.c" this gives:
> .file "koe.c"
>gcc2_compiled.:
>___gnu_compiled_c:
>.text
> .align 2
>.globl _foo
> .type _foo,@function
>_foo:
> pushl %ebp
> movl %esp,%ebp
> movl _value,%eax
> leal 0(,%eax,2),%edx
> movl %edx,_value
> movl _cpl,%ecx
> orb $1,_cpl
> leal (%edx,%eax,4),%eax <-- note: "value" kept in register across
> movl %eax,_value call to spl1.
> movl %ecx,_cpl
> leave
> ret
>Lfe1:
> .size _foo,Lfe1-_foo
Note that cpl is stored to in the desired order.
>Thus, the spl code did not do what it was supposed to do: we cannot
>make assumptions about values computed before entering the critical
>section being still valid in the critical section.
spl isn't supposed to turn non-volatile variables into volatile ones. It
just happens to have that effect if you hide it sufficiently from the
compiler. For gcc-2.3.3 on the i386, any non-static function is hidden
sufficiently.
>There definitely is a serious problem here. I cannot be absolutely
>sure whether it is in the compiler or in the kernel as I do not have
>the standard. In my opinion the compiler is doing only reasonable and
The problem is actually that many variables are declared as
/*nonvolatile*/ when they are actually volatile. Almost any device
driver contains dozens of examples. Consider a buffer that is filled by
interrupt handlers, e.g, the clists in struct tty. The pointers for the
buffer and the buffer contents are all volatile but none are declared
volatile. A sufficiently good optimizer might cache some of the values
across function calls. Non-static spl()s just require a better
optimizer for there to be a problem. Note that the locking feature of
spl*() isn't important here. Any call to an external function together
with a call to a static inline spl*() would work just as well. Any
write through a pointer together with a static inline spl*() would work
just as well... On second thoughts, the optimizer doesn't need to be
much better for an external function to not help. Consider the case
where the critical variables and all pointers to them are static. Then
the external function can't legitimately modify the values.
I don't know of a good way to avoid this problem. Declaring all critical
variables as volatile would be slow. It would be nice if the variables
were volatile inside the critical region and volatile outside it, but
C doesn't provide this feature directly. You could probably get it by
declaring the variables as volatile and accessing them as
*(/* nonvolatile */ foo_t *)&foovar inside the critical region but I
don't want that.
I seem to remember some bug reports for gcc under Linux related to this
problem. I think it was expected that
__asm("" : : : "memory");
would flush all cached variables from registers. It should do this
because "memory" in the clobber list says that arbitrary memory might
be written to. I think it didn't work as expected. If/when it works,
it may be the best way to handle the problem.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199505121619.CAA01072>