Another round of fixes.

test_vs_proto2.googlemessage1 passes again,
with no memory leaks!

core/upb.c

@@ -13,31 +13,31 @@
 #include "upb_string.h"
 
 #define alignof(t) offsetof(struct { char c; t x; }, x)
-#define TYPE_INFO(wire_type, ctype, allows_delimited) \
+#define TYPE_INFO(wire_type, ctype, allows_delimited, inmemory_type) \
     {alignof(ctype), sizeof(ctype), wire_type, \
      (1 << wire_type) | (allows_delimited << UPB_WIRE_TYPE_DELIMITED), \
-     #ctype},
+     UPB_TYPE(inmemory_type), #ctype},
 
 const upb_type_info upb_types[] = {
-  {0, 0, 0, 0, ""}, // There is no type 0.
-  TYPE_INFO(UPB_WIRE_TYPE_64BIT,       double,    1)    // DOUBLE
-  TYPE_INFO(UPB_WIRE_TYPE_32BIT,       float,     1)    // FLOAT
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int64_t,   1)    // INT64
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      uint64_t,  1)    // UINT64
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int32_t,   1)    // INT32
-  TYPE_INFO(UPB_WIRE_TYPE_64BIT,       uint64_t,  1)    // FIXED64
-  TYPE_INFO(UPB_WIRE_TYPE_32BIT,       uint32_t,  1)    // FIXED32
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      bool,      1)    // BOOL
-  TYPE_INFO(UPB_WIRE_TYPE_DELIMITED,   void*,     1)    // STRING
-  TYPE_INFO(UPB_WIRE_TYPE_START_GROUP, void*,     0)    // GROUP
-  TYPE_INFO(UPB_WIRE_TYPE_DELIMITED,   void*,     1)    // MESSAGE
-  TYPE_INFO(UPB_WIRE_TYPE_DELIMITED,   void*,     1)    // BYTES
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      uint32_t,  1)    // UINT32
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      uint32_t,  1)    // ENUM
-  TYPE_INFO(UPB_WIRE_TYPE_32BIT,       int32_t,   1)    // SFIXED32
-  TYPE_INFO(UPB_WIRE_TYPE_64BIT,       int64_t,   1)    // SFIXED64
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int32_t,   1)    // SINT32
-  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int64_t,   1)    // SINT64
+  {0, 0, 0, 0, 0, ""}, // There is no type 0.
+  TYPE_INFO(UPB_WIRE_TYPE_64BIT,       double,    1, DOUBLE)    // DOUBLE
+  TYPE_INFO(UPB_WIRE_TYPE_32BIT,       float,     1, FLOAT)     // FLOAT
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int64_t,   1, INT64)     // INT64
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      uint64_t,  1, UINT64)    // UINT64
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int32_t,   1, INT32)     // INT32
+  TYPE_INFO(UPB_WIRE_TYPE_64BIT,       uint64_t,  1, UINT64)    // FIXED64
+  TYPE_INFO(UPB_WIRE_TYPE_32BIT,       uint32_t,  1, UINT32)    // FIXED32
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      bool,      1, BOOL)      // BOOL
+  TYPE_INFO(UPB_WIRE_TYPE_DELIMITED,   void*,     1, STRING)    // STRING
+  TYPE_INFO(UPB_WIRE_TYPE_START_GROUP, void*,     0, MESSAGE)   // GROUP
+  TYPE_INFO(UPB_WIRE_TYPE_DELIMITED,   void*,     1, MESSAGE)   // MESSAGE
+  TYPE_INFO(UPB_WIRE_TYPE_DELIMITED,   void*,     1, STRING)    // BYTES
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      uint32_t,  1, UINT32)    // UINT32
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      uint32_t,  1, ENUM)      // ENUM
+  TYPE_INFO(UPB_WIRE_TYPE_32BIT,       int32_t,   1, INT32)     // SFIXED32
+  TYPE_INFO(UPB_WIRE_TYPE_64BIT,       int64_t,   1, INT64)     // SFIXED64
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int32_t,   1, INT32)     // SINT32
+  TYPE_INFO(UPB_WIRE_TYPE_VARINT,      int64_t,   1, INT64)     // SINT64
 };
 
 void upb_seterr(upb_status *status, enum upb_status_code code,

core/upb.h

@@ -97,6 +97,7 @@ typedef struct {
   uint8_t size;
   upb_wire_type_t native_wire_type;
   uint8_t allowed_wire_types;  // For packable fields, also allows delimited.
+  uint8_t inmemory_type;    // For example, INT32, SINT32, and SFIXED32 -> INT32
   char *ctype;
 } upb_type_info;
 
@@ -183,6 +184,7 @@ typedef struct {
 UPB_VALUE_ACCESSORS(double, _double, double, UPB_TYPE(DOUBLE));
 UPB_VALUE_ACCESSORS(float, _float, float, UPB_TYPE(FLOAT));
 UPB_VALUE_ACCESSORS(int32, int32, int32_t, UPB_TYPE(INT32));
+UPB_VALUE_ACCESSORS(enumval, int32, int32_t, UPB_TYPE(ENUM));
 UPB_VALUE_ACCESSORS(int64, int64, int64_t, UPB_TYPE(INT64));
 UPB_VALUE_ACCESSORS(uint32, uint32, uint32_t, UPB_TYPE(UINT32));
 UPB_VALUE_ACCESSORS(uint64, uint64, uint64_t, UPB_TYPE(UINT64));

core/upb_def.c

@@ -630,10 +630,10 @@ static upb_flow_t upb_fielddef_value(void *_b, upb_fielddef *f, upb_value val) {
   upb_defbuilder *b = _b;
   switch(f->number) {
     case GOOGLE_PROTOBUF_FIELDDESCRIPTORPROTO_TYPE_FIELDNUM:
-      b->f->type = upb_value_getint32(val);
+      b->f->type = upb_value_getenumval(val);
       break;
     case GOOGLE_PROTOBUF_FIELDDESCRIPTORPROTO_LABEL_FIELDNUM:
-      b->f->label = upb_value_getint32(val);
+      b->f->label = upb_value_getenumval(val);
       break;
     case GOOGLE_PROTOBUF_FIELDDESCRIPTORPROTO_NUMBER_FIELDNUM:
       b->f->number = upb_value_getint32(val);
@@ -1019,7 +1019,6 @@ bool upb_symtab_add_defs(upb_symtab *s, upb_def **defs, int num_defs,
   upb_symtab_ent *tmptab_e;
   for(tmptab_e = upb_strtable_begin(&tmptab); tmptab_e;
       tmptab_e = upb_strtable_next(&tmptab, &tmptab_e->e)) {
-    printf("Inserting def for " UPB_STRFMT "\n", UPB_STRARG(tmptab_e->def->fqname));
     upb_symtab_ent *symtab_e =
         upb_strtable_lookup(&s->symtab, tmptab_e->def->fqname);
     if(symtab_e) {
@@ -1210,7 +1209,7 @@ static uint32_t upb_baredecoder_readf32(upb_baredecoder *d)
 
 static void upb_baredecoder_sethandlers(upb_src *src, upb_handlers *handlers) {
   upb_baredecoder *d = (upb_baredecoder*)src;
-  upb_dispatcher_reset(&d->dispatcher, handlers);
+  upb_dispatcher_reset(&d->dispatcher, handlers, false);
 }
 
 static void upb_baredecoder_run(upb_src *src, upb_status *status) {

core/upb_msg.c

@@ -168,10 +168,10 @@ static void upb_msg_appendval(upb_msg *msg, upb_fielddef *f, upb_value val) {
     // We do:
     //  - upb_string_recycle(), upb_string_substr() instead of
     //  - upb_string_unref(), upb_string_getref()
-    // because we have a convenient place of caching these string objects for
-    // reuse, where as the upb_src who is sending us these strings may not.
-    // This saves the upb_src from allocating new upb_strings all the time to
-    // give us.
+    // because we can conveniently caching these upb_string objects in the
+    // upb_msg, whereas the upb_src who is sending us these strings may not
+    // have a good way of caching them.  This saves the upb_src from allocating
+    // new upb_strings all the time to give us.
     //
     // If you were using this to copy one upb_msg to another this would
     // allocate string objects whereas a upb_string_getref could have avoided
@@ -183,6 +183,7 @@ static void upb_msg_appendval(upb_msg *msg, upb_fielddef *f, upb_value val) {
   } else {
     upb_value_write(p, val, f->type);
   }
+  upb_msg_sethas(msg, f);
 }
 
 upb_msg *upb_msg_appendmsg(upb_msg *msg, upb_fielddef *f, upb_msgdef *msgdef) {

core/upb_msg.h

@@ -60,7 +60,7 @@ INLINE upb_value upb_value_read(upb_valueptr ptr, upb_fieldtype_t ft) {
   case UPB_TYPE(t): val.val.member_name = *ptr.member_name; break;
 #else
 #define CASE(t, member_name) \
-  case UPB_TYPE(t): val.val.member_name = *ptr.member_name; val.type = ft; break;
+  case UPB_TYPE(t): val.val.member_name = *ptr.member_name; val.type = upb_types[ft].inmemory_type; break;
 #endif
 
   switch(ft) {
@@ -82,7 +82,13 @@ INLINE upb_value upb_value_read(upb_valueptr ptr, upb_fieldtype_t ft) {
     CASE(BYTES,    str)
     CASE(MESSAGE,  msg)
     CASE(GROUP,    msg)
-    default: assert(false);
+    case UPB_VALUETYPE_ARRAY:
+      val.val.arr = *ptr.arr;
+#ifndef NDEBUG
+      val.type = UPB_VALUETYPE_ARRAY;
+#endif
+      break;
+    default: printf("type: %d\n", ft); assert(false);
   }
   return val;
 
@@ -91,7 +97,11 @@ INLINE upb_value upb_value_read(upb_valueptr ptr, upb_fieldtype_t ft) {
 
 INLINE void upb_value_write(upb_valueptr ptr, upb_value val,
                             upb_fieldtype_t ft) {
-  assert(val.type == ft);
+  if (ft == UPB_VALUETYPE_ARRAY) {
+    assert(val.type == UPB_VALUETYPE_ARRAY);
+  } else {
+    assert(val.type == upb_types[ft].inmemory_type);
+  }
 #define CASE(t, member_name) \
   case UPB_TYPE(t): *ptr.member_name = val.val.member_name; break;
 
@@ -114,6 +124,9 @@ INLINE void upb_value_write(upb_valueptr ptr, upb_value val,
     CASE(BYTES,    str)
     CASE(MESSAGE,  msg)
     CASE(GROUP,    msg)
+    case UPB_VALUETYPE_ARRAY:
+      *ptr.arr = val.val.arr;
+      break;
     default: assert(false);
   }
 
@@ -142,7 +155,7 @@ INLINE upb_valueptr _upb_array_getptr(upb_array *a, upb_fielddef *f,
 upb_array *upb_array_new(void);
 
 INLINE void upb_array_unref(upb_array *a, upb_fielddef *f) {
-  if (upb_atomic_unref(&a->refcount)) _upb_array_free(a, f);
+  if (a && upb_atomic_unref(&a->refcount)) _upb_array_free(a, f);
 }
 
 void upb_array_recycle(upb_array **arr, upb_fielddef *f);

core/upb_stream.h

@@ -198,7 +198,8 @@ INLINE void upb_src_run(upb_src *src, upb_status *status);
 struct _upb_dispatcher;
 typedef struct _upb_dispatcher upb_dispatcher;
 INLINE void upb_dispatcher_init(upb_dispatcher *d);
-INLINE void upb_dispatcher_reset(upb_dispatcher *d, upb_handlers *h);
+INLINE void upb_dispatcher_reset(upb_dispatcher *d, upb_handlers *h,
+                                 bool supports_skip);
 INLINE upb_flow_t upb_dispatch_startmsg(upb_dispatcher *d);
 INLINE upb_flow_t upb_dispatch_endmsg(upb_dispatcher *d);
 INLINE upb_flow_t upb_dispatch_startsubmsg(upb_dispatcher *d,

core/upb_stream_vtbl.h

@@ -218,16 +218,19 @@ typedef struct {
 
 struct _upb_dispatcher {
   upb_dispatcher_frame stack[UPB_MAX_NESTING], *top, *limit;
+  bool supports_skip;
 };
 
 INLINE void upb_dispatcher_init(upb_dispatcher *d) {
   d->limit = d->stack + sizeof(d->stack);
 }
 
-INLINE void upb_dispatcher_reset(upb_dispatcher *d, upb_handlers *h) {
+INLINE void upb_dispatcher_reset(upb_dispatcher *d, upb_handlers *h,
+                                 bool supports_skip) {
   d->top = d->stack;
   d->top->depth = 1;  // Never want to trigger end-of-delegation.
   d->top->handlers = *h;
+  d->supports_skip = supports_skip;
 }
 
 INLINE upb_flow_t upb_dispatch_startmsg(upb_dispatcher *d) {
@@ -256,7 +259,7 @@ INLINE upb_flow_t upb_dispatch_startsubmsg(upb_dispatcher *d,
     d->top->depth = 0;
     ret = d->top->handlers.set->startmsg(d->top->handlers.closure);
   }
-  if (ret == UPB_CONTINUE) ++d->top->depth;
+  if (ret == UPB_CONTINUE || !d->supports_skip) ++d->top->depth;
   upb_handlers_uninit(&handlers);
   return ret;
 }

core/upb_string.c

@@ -63,7 +63,7 @@ void _upb_string_free(upb_string *str) {
 
 void upb_string_recycle(upb_string **_str) {
   upb_string *str = *_str;
-  if(str && upb_atomic_read(&str->refcount) == 1) {
+  if(str && upb_atomic_only(&str->refcount)) {
     str->ptr = NULL;
     upb_string_release(str);
   } else {

core/upb_string.h

@@ -57,6 +57,9 @@ extern "C" {
 // All members of this struct are private, and may only be read/written through
 // the associated functions.
 struct _upb_string {
+  // The string's refcount.
+  upb_atomic_refcount_t refcount;
+
   // The pointer to our currently active data.  This may be memory we own
   // or a pointer into memory we don't own.
   const char *ptr;
@@ -76,9 +79,6 @@ struct _upb_string {
   uint32_t size;
 #endif
 
-  // The string's refcount.
-  upb_atomic_refcount_t refcount;
-
   // Used if this is a slice of another string, NULL otherwise.  We own a ref
   // on src.
   struct _upb_string *src;
@@ -86,9 +86,9 @@ struct _upb_string {
 
 // Internal-only initializer for upb_string instances.
 #ifdef UPB_HAVE_MSIZE
-#define _UPB_STRING_INIT(str, len, refcount) {(char*)str, NULL, len, {refcount}, NULL}
+#define _UPB_STRING_INIT(str, len, refcount) {{refcount}, (char*)str, NULL, len, NULL}
 #else
-#define _UPB_STRING_INIT(str, len, refcount) {(char*)str, NULL, len, 0, {refcount}, NULL}
+#define _UPB_STRING_INIT(str, len, refcount) {{refcount}, (char*)str, NULL, len, 0, NULL}
 #endif
 
 // Special pseudo-refcounts for static/stack-allocated strings, respectively.

stream/upb_decoder.c

@@ -181,10 +181,10 @@ INLINE bool upb_decode_fixed(upb_decoder *d, upb_wire_type_t wt,
   size_t bytes = table[wt];
   if (s->len >= bytes) {
     // Common (fast) case.
-    memcpy(&val, s->ptr, bytes);
+    memcpy(val, s->ptr, bytes);
     upb_dstate_advance(s, bytes);
   } else {
-    if (!upb_getbuf(d, &val, bytes, s)) return false;
+    if (!upb_getbuf(d, val, bytes, s)) return false;
   }
   return true;
 }
@@ -270,7 +270,6 @@ void upb_decoder_run(upb_src *src, upb_status *status) {
     if (!upb_decode_tag(d, &state, &tag)) {
       if (status->code == UPB_EOF && d->top == d->stack) {
         // Normal end-of-file.
-        printf("Normal end-of-file!\n");
         upb_clearerr(status);
         CHECK_FLOW(upb_dispatch_endmsg(&d->dispatcher));
         upb_string_unref(str);
@@ -345,6 +344,9 @@ void upb_decoder_run(upb_src *src, upb_status *status) {
         upb_value_setint64(&val, upb_zzdec_64(upb_value_getint64(val)));
         break;
       default:
+#ifndef NDEBUG
+        val.type = upb_types[f->type].inmemory_type;
+#endif
         break;  // Other types need no further processing at this point.
     }
     CHECK_FLOW(upb_dispatch_value(&d->dispatcher, f, val));
@@ -359,7 +361,7 @@ err:
 
 void upb_decoder_sethandlers(upb_src *src, upb_handlers *handlers) {
   upb_decoder *d = (upb_decoder*)src;
-  upb_dispatcher_reset(&d->dispatcher, handlers);
+  upb_dispatcher_reset(&d->dispatcher, handlers, false);
   d->top = d->stack;
   d->buf_stream_offset = 0;
   d->top->msgdef = d->toplevel_msgdef;

tests/test_stream.c

@@ -87,7 +87,7 @@ static void test_dispatcher() {
   upb_set_handler_closure(&h, &data, NULL);
   upb_dispatcher d;
   upb_dispatcher_init(&d);
-  upb_dispatcher_reset(&d, &h);
+  upb_dispatcher_reset(&d, &h, false);
 
   upb_dispatch_startmsg(&d);
   upb_value val;

tests/test_vs_proto2.cc

@@ -79,6 +79,7 @@ void compare_arrays(const google::protobuf::Reflection *r,
   }
 }
 
+#include <inttypes.h>
 void compare_values(const google::protobuf::Reflection *r,
                     const google::protobuf::Message& proto2_msg,
                     const google::protobuf::FieldDescriptor *proto2_f,
@@ -176,9 +177,15 @@ void parse_and_compare(MESSAGE_CIDENT *proto2_msg,
   // Parse to both proto2 and upb.
   ASSERT(proto2_msg->ParseFromArray(upb_string_getrobuf(str), upb_string_len(str)));
   upb_status status = UPB_STATUS_INIT;
+  upb_msg_clear(upb_msg, upb_md);
   upb_strtomsg(str, upb_msg, upb_md, &status);
-  ASSERT(upb_ok(&status));
+  if (!upb_ok(&status)) {
+    fprintf(stderr, "Error parsing test protobuf: ");
+    upb_printerr(&status);
+    exit(1);
+  }
   compare(*proto2_msg, upb_msg, upb_md);
+  upb_status_uninit(&status);
 }
 
 int main(int argc, char *argv[])
@@ -252,8 +259,11 @@ int main(int argc, char *argv[])
 
   upb_msg_unref(upb_msg, msgdef);
   upb_def_unref(UPB_UPCAST(msgdef));
+  upb_def_unref(fds_msgdef);
   upb_string_unref(str);
   upb_symtab_unref(symtab);
+  upb_status_uninit(&status);
+  google::protobuf::ShutdownProtobufLibrary();
 
   return 0;
 }