On Wednesday 10 August 2005 12:55, Hannu Koivisto wrote:
> I was reading dynamic variable related VOPs for builds with thread
> support in src/compiler/x86/cell.lisp of SBCL from CVS as of
> 2005-07-29 (the file hasn't changed since so I assume the problems
> are still relevant) and saw things that I thought couldn't work.
> First I noticed that the use of *free-tls-index* variable (TLS
> index allocation) in the BIND VOP seemed thread-unsafe:
> ;; allocate a new tls-index
> (load-symbol-value tls-index *free-tls-index*)
> (inst add tls-index 4) ;XXX surely we can do this more
> (store-symbol-value tls-index *free-tls-index*) ;succintly
> More than one symbol may end up with the same TLS index. Then I
> noticed that the entire "need to allocate? - allocate - store
> allocated value - use allocated value" logic didn't seem to be
> thread-safe even if allocation itself was. Let's consider a case
> with only two threads. If they end up allocating a tls-index for
> the same symbol at the same time, only one of them stays in the
> symbol's tls-index slot and is thus the only one used for all
> future references, but the thread whose TLS index was overwritten
> still uses this walking dead index to store the given value.
> I.e. I think it would be possible for (let ((*foo* 1)) (boundp
> '*foo*)) to evaluate to NIL if the global value was not bound.
> I also took a look at the SET VOP and found it troubling that the
> code would set the global value if the value in TLS was not bound.
> And indeed, this test...
> (progn (defvar *f* 0) (progv '(*f*) '() (setq *f* 2)) *f*)
> ...evaluates to 2 but I believe 0 would be correct. Note that even
> though I spotted the problem in the SET VOP, the problematic "if
> the value in TLS in not bound, use the global value" logic affects
> other code as well.
Thank you for the bug reports.
The attached patch addresses the second problem. It introduces a new wideta=
no-tls-value-marker -, modifies the set/symbol-value/fast-symbol-value/boun=
vops, and hooks it into gc.
Please review this patch because I'm not familiar with assembly and maybe t=
new widetag needs to be hooked into more places. Only tested on x86.