Permalink
Browse files

Fix thread safety issue for flushing variable scopes

There was a race condition when flushing the variable scope, because
another thread might already see isolated set to true, before the heap
locals were properly initialized.

This result in this other thread seeing nil as the local value instead
of the proper value. This commit changes the flushing strategy to be
both safe for reads and writes of the locals.

The first step is to delay the allocation of the heap locals tuple. We
then atomically indicate that this scope uses heap locals. If any other
thread also wants to use the scope, it will spin until the heap locals
have been allocated. Spinning for both the read and write ensures that
we don't see any intermediate state during the move from stack to heap
locals. This means no writes are lost and no stale reads occur.

We then create the heap locals and only store then in the heap_locals_
ivar after initializing them, so any reads or writes will see the proper
locals and write to the correct slots.

Fixes #2124
  • Loading branch information...
dbussink committed Mar 25, 2013
1 parent 0c8dde2 commit c54998d22e3f6238a1c73ea715bb80686788a162
@@ -99,14 +99,15 @@ namespace rubinius {
void VariableScope::set_local(STATE, int pos, Object* val) {
if(isolated_) {
+ while(heap_locals_->nil_p()) atomic::pause();
heap_locals_->put(state, pos, val);
} else {
set_local(pos, val);
}
}
void VariableScope::set_local(int pos, Object* val) {
- assert(isolated_ == false);
+ assert(!isolated_);
Object** ary = locals_;
if(Fiber* fib = try_as<Fiber>(fiber_)) {
@@ -125,6 +126,7 @@ namespace rubinius {
Object* VariableScope::get_local(int pos) {
if(isolated_) {
+ while(heap_locals_->nil_p()) atomic::pause();
return heap_locals_->at(pos);
}
@@ -142,6 +144,18 @@ namespace rubinius {
return ary[pos];
}
+ void VariableScope::flush_to_heap(STATE) {
+ if(isolated_) return;
+ if(!atomic::compare_and_swap(&isolated_, 0, 1)) return;
+
+ Tuple* new_locals = Tuple::create(state, number_of_locals_);
+ for(int i = 0; i < number_of_locals_; i++) {
+ new_locals->put(state, i, locals_[i]);
+ }
+ atomic::memory_barrier();
+ heap_locals(state, new_locals);
+ }
+
void VariableScope::Info::mark(Object* obj, ObjectMark& mark) {
auto_mark(obj, mark);
@@ -37,9 +37,9 @@ namespace rubinius {
public:
Object* self_; // slot
- int number_of_locals_;
- bool isolated_;
Object** locals_;
+ int number_of_locals_;
+ int isolated_;
int block_as_method_;
public: /* Accessors */
@@ -59,7 +59,7 @@ namespace rubinius {
void fixup() { }
bool isolated() {
- return isolated_;
+ return isolated_ == 1;
}
bool block_as_method_p() {
@@ -82,6 +82,8 @@ namespace rubinius {
return number_of_locals_;
}
+ void flush_to_heap(STATE);
+
VariableScope* promote(STATE);
// Rubinius.primitive :variable_scope_of_sender
View
@@ -188,9 +188,9 @@ namespace VariableScope {
const static int heap_locals = 5;
const static int last_match = 6;
const static int self = 7;
- const static int number_of_locals = 8;
- const static int isolated = 9;
- const static int locals = 10;
+ const static int locals = 8;
+ const static int number_of_locals = 9;
+ const static int isolated = 10;
const static int block_as_method = 11;
}
namespace jit_RuntimeData {
View
@@ -327,10 +327,10 @@ StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_29);
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_16);
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_9);
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_9);
-StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
-StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 8));
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_21);
StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
+StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
+StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
if (StructTy_struct_rubinius__VariableScope->isOpaque()) {
StructTy_struct_rubinius__VariableScope->setBody(StructTy_struct_rubinius__VariableScope_fields, /*isPacked=*/false);
}
View
@@ -270,9 +270,9 @@ declare void @output22(%"struct.rubinius::Tuple"*)
%"struct.rubinius::Tuple"*, ; heap_locals
%"struct.rubinius::Object"*, ; last_match
%"struct.rubinius::Object"*, ; self
- i32, ; number_of_locals
- i8, ; isolated
%"struct.rubinius::Object"**, ; locals
+ i32, ; number_of_locals
+ i32, ; isolated
i32 ; block_as_method
}
View
@@ -327,10 +327,10 @@ StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_29);
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_16);
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_9);
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_9);
-StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
-StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 8));
StructTy_struct_rubinius__VariableScope_fields.push_back(PointerTy_21);
StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
+StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
+StructTy_struct_rubinius__VariableScope_fields.push_back(IntegerType::get(mod->getContext(), 32));
if (StructTy_struct_rubinius__VariableScope->isOpaque()) {
StructTy_struct_rubinius__VariableScope->setBody(StructTy_struct_rubinius__VariableScope_fields, /*isPacked=*/false);
}
View
@@ -270,9 +270,9 @@ declare void @output22(%"struct.rubinius::Tuple"*)
%"struct.rubinius::Tuple"*, ; heap_locals
%"struct.rubinius::Object"*, ; last_match
%"struct.rubinius::Object"*, ; self
- i32, ; number_of_locals
- i8, ; isolated
%"struct.rubinius::Object"**, ; locals
+ i32, ; number_of_locals
+ i32, ; isolated
i32 ; block_as_method
}
View
@@ -24,23 +24,22 @@ namespace rubinius {
scope->block(state, block_);
scope->module(state, module_);
scope->method(state, call_frame->compiled_code);
- scope->heap_locals(state, Tuple::create(state, mcode->number_of_locals));
+ scope->heap_locals(state, nil<Tuple>());
scope->last_match(state, last_match_);
scope->fiber(state, state->vm()->current_fiber.get());
scope->number_of_locals_ = mcode->number_of_locals;
-
- if(full) {
- scope->isolated_ = false;
- } else {
- scope->isolated_ = true;
- }
+ scope->isolated_ = 0;
scope->locals_ = locals_;
scope->dynamic_locals(state, nil<LookupTable>());
scope->set_block_as_method(call_frame->block_as_method_p());
+ if(!full) {
+ flush_to_heap(state);
+ }
+
on_heap_ = scope;
return scope;
@@ -94,12 +93,8 @@ namespace rubinius {
}
void StackVariables::flush_to_heap(STATE) {
- if(!on_heap_) return;
-
- on_heap_->isolated_ = true;
-
- for(int i = 0; i < on_heap_->number_of_locals_; i++) {
- on_heap_->set_local(state, i, locals_[i]);
+ if(on_heap_) {
+ on_heap_->flush_to_heap(state);
}
}
}

0 comments on commit c54998d

Please sign in to comment.