aboutsummaryrefslogtreecommitdiffhomepage
path: root/ruby
diff options
context:
space:
mode:
authorGravatar Chris Fallin <cfallin@google.com>2015-02-02 13:03:08 -0800
committerGravatar Chris Fallin <cfallin@google.com>2015-02-02 13:03:08 -0800
commiteb33f9d3d6f1f78ee70f4bea4326adaf035e1e67 (patch)
tree3d3bd24d98f81fcdea8a2c819e1fb49faca08042 /ruby
parent07b8b0f28d1ad88c7c1c64e8707dab8537313c26 (diff)
Updated based on code-review comments.
Diffstat (limited to 'ruby')
-rw-r--r--ruby/ext/google/protobuf_c/encode_decode.c9
-rw-r--r--ruby/ext/google/protobuf_c/protobuf.h14
-rw-r--r--ruby/ext/google/protobuf_c/storage.c44
3 files changed, 51 insertions, 16 deletions
diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c
index b9ecd456..0630f567 100644
--- a/ruby/ext/google/protobuf_c/encode_decode.c
+++ b/ruby/ext/google/protobuf_c/encode_decode.c
@@ -359,9 +359,12 @@ static void *oneofsubmsg_handler(void *closure,
DEREF(msg, oneofdata->ofs, VALUE) =
rb_class_new_instance(0, NULL, subklass);
}
- // Set the oneof case *after* allocating the new class instance -- see comment
- // in layout_set() as to why. There are subtle interactions with possible GC
- // points and oneof field type transitions.
+ // Set the oneof case *after* allocating the new class instance -- otherwise,
+ // if the Ruby GC is invoked as part of a call into the VM, it might invoke
+ // our mark routines, and our mark routines might see the case value
+ // indicating a VALUE is present and expect a valid VALUE. See comment in
+ // layout_set() for more detail: basically, the change to the value and the
+ // case must be atomic w.r.t. the Ruby VM.
DEREF(msg, oneofdata->case_ofs, uint32_t) =
oneofdata->oneof_case_num;
diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h
index 5b94ae26..d8a327aa 100644
--- a/ruby/ext/google/protobuf_c/protobuf.h
+++ b/ruby/ext/google/protobuf_c/protobuf.h
@@ -292,6 +292,15 @@ void native_slot_set(upb_fieldtype_t type,
VALUE type_class,
void* memory,
VALUE value);
+// Atomically (with respect to Ruby VM calls) either update the value and set a
+// oneof case, or do neither. If |case_memory| is null, then no case value is
+// set.
+void native_slot_set_value_and_case(upb_fieldtype_t type,
+ VALUE type_class,
+ void* memory,
+ VALUE value,
+ uint32_t* case_memory,
+ uint32_t case_number);
VALUE native_slot_get(upb_fieldtype_t type,
VALUE type_class,
const void* memory);
@@ -313,6 +322,11 @@ VALUE field_type_class(const upb_fielddef* field);
#define MAP_KEY_FIELD 1
#define MAP_VALUE_FIELD 2
+// Oneof case slot value to indicate that no oneof case is set. The value `0` is
+// safe because field numbers are used as case identifiers, and no field can
+// have a number of 0.
+#define ONEOF_CASE_NONE 0
+
// These operate on a map field (i.e., a repeated field of submessages whose
// submessage type is a map-entry msgdef).
bool is_map_field(const upb_fielddef* field);
diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c
index 662101f8..5b1549d2 100644
--- a/ruby/ext/google/protobuf_c/storage.c
+++ b/ruby/ext/google/protobuf_c/storage.c
@@ -109,6 +109,17 @@ void native_slot_validate_string_encoding(upb_fieldtype_t type, VALUE value) {
void native_slot_set(upb_fieldtype_t type, VALUE type_class,
void* memory, VALUE value) {
+ native_slot_set_value_and_case(type, type_class, memory, value, NULL, 0);
+}
+
+void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
+ void* memory, VALUE value,
+ uint32_t* case_memory,
+ uint32_t case_number) {
+ // Note that in order to atomically change the value in memory and the case
+ // value (w.r.t. Ruby VM calls), we must set the value at |memory| only after
+ // all Ruby VM calls are complete. The case is then set at the bottom of this
+ // function.
switch (type) {
case UPB_TYPE_FLOAT:
if (!is_ruby_num(value)) {
@@ -198,6 +209,10 @@ void native_slot_set(upb_fieldtype_t type, VALUE type_class,
default:
break;
}
+
+ if (case_memory != NULL) {
+ *case_memory = case_number;
+ }
}
VALUE native_slot_get(upb_fieldtype_t type,
@@ -408,6 +423,13 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
// We assign all value slots first, then pack the 'case' fields at the end,
// since in the common case (modern 64-bit platform) these are 8 bytes and 4
// bytes respectively and we want to avoid alignment overhead.
+ //
+ // Note that we reserve 4 bytes (a uint32) per 'case' slot because the value
+ // space for oneof cases is conceptually as wide as field tag numbers. In
+ // practice, it's unlikely that a oneof would have more than e.g. 256 or 64K
+ // members (8 or 16 bits respectively), so conceivably we could assign
+ // consecutive case numbers and then pick a smaller oneof case slot size, but
+ // the complexity to implement this indirection is probably not worthwhile.
upb_msg_oneof_iter oit;
for (upb_msg_oneof_begin(&oit, msgdef);
!upb_msg_oneof_done(&oit);
@@ -576,7 +598,7 @@ void layout_set(MessageLayout* layout,
if (upb_fielddef_containingoneof(field)) {
if (val == Qnil) {
// Assigning nil to a oneof field clears the oneof completely.
- *oneof_case = 0;
+ *oneof_case = ONEOF_CASE_NONE;
memset(memory, 0, NATIVE_SLOT_MAX_SIZE);
} else {
// The transition between field types for a single oneof (union) slot is
@@ -587,18 +609,14 @@ void layout_set(MessageLayout* layout,
// what it thinks is a primitive field but is actually a VALUE for the new
// field type).
//
- // native_slot_set() has two parts: (i) conversion of some sort, and (ii)
- // setting the in-memory content to the new value. It guarantees that all
- // calls to the Ruby VM are completed before the memory slot is altered.
- //
// In order for the transition to be safe, the oneof case slot must be in
- // sync with the value slot whenever the Ruby VM has been called. Because
- // we are guaranteed that no Ruby VM calls occur after native_slot_set()
- // alters the memory slot and before it returns, we set the oneof case
- // immediately after native_slot_set() returns.
- native_slot_set(upb_fielddef_type(field), field_type_class(field),
- memory, val);
- *oneof_case = upb_fielddef_number(field);
+ // sync with the value slot whenever the Ruby VM has been called. Thus, we
+ // use native_slot_set_value_and_case(), which ensures that both the value
+ // and case number are altered atomically (w.r.t. the Ruby VM).
+ native_slot_set_value_and_case(
+ upb_fielddef_type(field), field_type_class(field),
+ memory, val,
+ oneof_case, upb_fielddef_number(field));
}
} else if (is_map_field(field)) {
check_map_field_type(val, field);
@@ -624,7 +642,7 @@ void layout_init(MessageLayout* layout,
if (upb_fielddef_containingoneof(field)) {
memset(memory, 0, NATIVE_SLOT_MAX_SIZE);
- *oneof_case = 0;
+ *oneof_case = ONEOF_CASE_NONE;
} else if (is_map_field(field)) {
VALUE map = Qnil;