Debugging test_def, it's close to working again!

Makefile

@@ -74,9 +74,9 @@ OTHERSRC=src/upb_encoder.c src/upb_text.c
 # Override the optimization level for upb_def.o, because it is not in the
 # critical path but gets very large when -O3 is used.
 core/upb_def.o: core/upb_def.c
-	$(CC) $(CFLAGS) $(CPPFLAGS) -Os -c -o $@ $<
+	$(CC) $(CFLAGS) $(CPPFLAGS) -O0 -c -o $@ $<
 core/upb_def.lo: core/upb_def.c
-	$(CC) $(CFLAGS) $(CPPFLAGS) -Os -c -o $@ $< -fPIC
+	$(CC) $(CFLAGS) $(CPPFLAGS) -O0 -c -o $@ $< -fPIC
 
 lang_ext/lua/upb.so: lang_ext/lua/upb.lo
 	$(CC) $(CFLAGS) $(CPPFLAGS) -shared -o $@ $< core/libupb_pic.a
@@ -112,13 +112,13 @@ tests/test.proto.pb: tests/test.proto
 
 TESTS=tests/test_string \
     tests/test_table \
-    tests/test_stream \
-#    tests/test_def \
+    tests/test_def \
+#    tests/test_stream \
 #    tests/test_decoder \
 #    tests/t.test_vs_proto2.googlemessage1 \
 #    tests/t.test_vs_proto2.googlemessage2 \
 #    tests/test.proto.pb
-tests: $(TESTS)
+tests: $(LIBUPB) $(TESTS)
 
 OTHER_TESTS=tests/tests \
 $(TESTS): $(LIBUPB)

core/upb_def.c

@@ -319,6 +319,18 @@ void upb_defbuilder_setscopename(upb_defbuilder *b, upb_string *str) {
 }
 
 // Handlers for google.protobuf.FileDescriptorProto.
+static upb_flow_t upb_defbuilder_FileDescriptorProto_startmsg(void *_b) {
+  upb_defbuilder *b = _b;
+  upb_defbuilder_startcontainer(b);
+  return UPB_CONTINUE;
+}
+
+static upb_flow_t upb_defbuilder_FileDescriptorProto_endmsg(void *_b) {
+  upb_defbuilder *b = _b;
+  upb_defbuilder_endcontainer(b);
+  return UPB_CONTINUE;
+}
+
 static upb_flow_t upb_defbuilder_FileDescriptorProto_value(void *_b,
                                                            upb_fielddef *f,
                                                            upb_value val) {
@@ -353,8 +365,8 @@ static upb_flow_t upb_defbuilder_FileDescriptorProto_startsubmsg(
 static void upb_defbuilder_register_FileDescriptorProto(upb_defbuilder *b,
                                                         upb_handlers *h) {
   static upb_handlerset handlers = {
-    NULL,  // startmsg
-    NULL,  // endmsg
+    &upb_defbuilder_FileDescriptorProto_startmsg,
+    &upb_defbuilder_FileDescriptorProto_endmsg,
     &upb_defbuilder_FileDescriptorProto_value,
     &upb_defbuilder_FileDescriptorProto_startsubmsg,
   };
@@ -457,9 +469,11 @@ static upb_flow_t upb_enumdef_EnumValueDescriptorProto_value(void *_b,
     case GOOGLE_PROTOBUF_ENUMVALUEDESCRIPTORPROTO_NAME_FIELDNUM:
       upb_string_unref(b->name);
       upb_string_getref(upb_value_getstr(val));
+      b->saw_name = true;
       break;
     case GOOGLE_PROTOBUF_ENUMVALUEDESCRIPTORPROTO_NUMBER_FIELDNUM:
       b->number = upb_value_getint32(val);
+      b->saw_number = true;
       break;
     default:
       break;
@@ -507,8 +521,8 @@ static upb_flow_t upb_enumdef_EnumDescriptorProto_startmsg(void *_b) {
 }
 
 static upb_flow_t upb_enumdef_EnumDescriptorProto_endmsg(void *_b) {
-  upb_defbuilder *b = _b;
-  assert(upb_defbuilder_last(b)->fqname != NULL);
+  (void)_b;
+  assert(upb_defbuilder_last((upb_defbuilder*)_b)->fqname != NULL);
   return UPB_CONTINUE;
 }
 
@@ -627,10 +641,8 @@ static upb_flow_t upb_fielddef_value(void *_b, upb_fielddef *f, upb_value val) {
       b->f->name = upb_string_getref(upb_value_getstr(val));
       break;
     case GOOGLE_PROTOBUF_FIELDDESCRIPTORPROTO_TYPE_NAME_FIELDNUM: {
-      upb_string *str = upb_string_new();
-      if (!upb_value_getfullstr(val, str, NULL)) return UPB_BREAK;
       if(b->f->def) upb_def_unref(b->f->def);
-      b->f->def = UPB_UPCAST(upb_unresolveddef_new(str));
+      b->f->def = UPB_UPCAST(upb_unresolveddef_new(upb_value_getstr(val)));
       b->f->owned = true;
       break;
     }
@@ -720,6 +732,7 @@ static upb_flow_t upb_msgdef_endmsg(void *_b) {
     m->size = offset + type_info->size;
     max_align = UPB_MAX(max_align, type_info->align);
   }
+  free(sorted_fields);
 
   if (max_align > 0) m->size = upb_align_up(m->size, max_align);
 
@@ -1131,6 +1144,7 @@ void upb_symtab_addfds(upb_symtab *s, upb_src *src, upb_status *status)
 // * keeping a pointer to the upb_fielddef* and reading it later (the same
 //   upb_fielddef is reused over and over).
 // * detecting errors in the input (we trust that our input is known-good).
+// * skipping the rest of the submessage (UPB_SKIPSUBMSG).
 //
 // It also does not support any of the follow protobuf features:
 // * packed fields.
@@ -1189,18 +1203,27 @@ static uint32_t upb_baredecoder_readf32(upb_baredecoder *d)
   return val;
 }
 
-bool upb_baredecoder_run(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);
+}
+
+static void upb_baredecoder_run(upb_src *src, upb_status *status) {
+  upb_baredecoder *d = (upb_baredecoder*)src;
+  assert(!upb_handlers_isempty(&d->dispatcher.top->handlers));
   upb_string *str = NULL;
   upb_strlen_t stack[UPB_MAX_NESTING];
   upb_strlen_t *top = &stack[0];
   *top = upb_string_len(d->input);
   d->offset = 0;
 
-  upb_dispatch_startmsg(&d->dispatcher);
+#define CHECK(x) if (x != UPB_CONTINUE && x != BEGIN_SUBMSG) goto err;
+
+  CHECK(upb_dispatch_startmsg(&d->dispatcher));
   while(d->offset < upb_string_len(d->input)) {
     // Detect end-of-submessage.
     while(d->offset >= *top) {
-      upb_dispatch_endsubmsg(&d->dispatcher);
+      CHECK(upb_dispatch_endsubmsg(&d->dispatcher));
       d->offset = *(top--);
     }
 
@@ -1216,9 +1239,11 @@ bool upb_baredecoder_run(upb_baredecoder *d) {
       upb_string_substr(str, d->input, d->offset, delim_len);
       upb_value v;
       upb_value_setstr(&v, str);
-      if(upb_dispatch_value(&d->dispatcher, &f, v) == BEGIN_SUBMSG) {
+      upb_flow_t ret = upb_dispatch_value(&d->dispatcher, &f, v);
+      CHECK(ret);
+      if(ret == BEGIN_SUBMSG) {
         // Should deliver as a submessage instead.
-        upb_dispatch_startsubmsg(&d->dispatcher, &f);
+        CHECK(upb_dispatch_startsubmsg(&d->dispatcher, &f));
         *(++top) = d->offset + delim_len;
       } else {
         d->offset += delim_len;
@@ -1228,11 +1253,9 @@ bool upb_baredecoder_run(upb_baredecoder *d) {
       switch(wt) {
         case UPB_WIRE_TYPE_VARINT:
           upb_value_setraw(&v, upb_baredecoder_readv64(d));
-          upb_dispatch_value(&d->dispatcher, &f, v);
           break;
         case UPB_WIRE_TYPE_64BIT:
           upb_value_setraw(&v, upb_baredecoder_readf64(d));
-          upb_dispatch_value(&d->dispatcher, &f, v);
           break;
         case UPB_WIRE_TYPE_32BIT:
           upb_value_setraw(&v, upb_baredecoder_readf32(d));
@@ -1241,28 +1264,33 @@ bool upb_baredecoder_run(upb_baredecoder *d) {
           assert(false);
           abort();
       }
-      upb_dispatch_value(&d->dispatcher, &f, v);
+      CHECK(upb_dispatch_value(&d->dispatcher, &f, v));
     }
   }
-  upb_dispatch_endmsg(&d->dispatcher);
-  return true;
+  CHECK(upb_dispatch_endmsg(&d->dispatcher));
+  printf("SUCCESS!!\n");
+  upb_string_unref(str);
+  return;
+
+err:
+  upb_copyerr(status, d->dispatcher.top->handlers.status);
+  upb_printerr(d->dispatcher.top->handlers.status);
+  upb_printerr(status);
+  upb_string_unref(str);
+  printf("ERROR!!\n");
 }
 
 static upb_baredecoder *upb_baredecoder_new(upb_string *str)
 {
-  //static upb_src_vtable vtbl = {
-  //  (upb_src_getdef_fptr)&upb_baredecoder_getdef,
-  //  (upb_src_getval_fptr)&upb_baredecoder_getval,
-  //  (upb_src_getstr_fptr)&upb_baredecoder_getstr,
-  //  (upb_src_skipval_fptr)&upb_baredecoder_skipval,
-  //  (upb_src_startmsg_fptr)&upb_baredecoder_startmsg,
-  //  (upb_src_endmsg_fptr)&upb_baredecoder_endmsg,
-  //};
+  static upb_src_vtbl vtbl = {
+    &upb_baredecoder_sethandlers,
+    &upb_baredecoder_run,
+  };
   upb_baredecoder *d = malloc(sizeof(*d));
+  upb_src_init(&d->src, &vtbl);
   d->input = upb_string_getref(str);
   d->offset = 0;
   upb_dispatcher_init(&d->dispatcher);
-  //upb_src_init(&d->src, &vtbl);
   return d;
 }
 

core/upb_stream.h

@@ -136,8 +136,8 @@ 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_dispatch_startmsg(upb_dispatcher *d);
-INLINE void upb_dispatch_endmsg(upb_dispatcher *d);
+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, struct _upb_fielddef *f);
 INLINE upb_flow_t upb_dispatch_endsubmsg(upb_dispatcher *d);
 INLINE upb_flow_t upb_dispatch_value(upb_dispatcher *d, struct _upb_fielddef *f,
@@ -151,8 +151,21 @@ INLINE upb_flow_t upb_dispatch_unknownval(upb_dispatcher *d,
 struct _upb_src;
 typedef struct _upb_src upb_src;
 
-void upb_src_sethandlers(upb_src *src, upb_handlers *handlers);
-void upb_src_run(upb_src *src, upb_status *status);
+// upb_src_sethandlers() must be called once and only once before upb_src_run()
+// is called.  This sets up the callbacks that will handle the parse.  A
+// upb_src that is fully initialized except for the call to
+// upb_src_sethandlers() is called "prepared" -- this is useful for library
+// functions that want to consume the output of a generic upb_src.
+// Calling sethandlers() multiple times is an error and will trigger an abort().
+INLINE void upb_src_sethandlers(upb_src *src, upb_handlers *handlers);
+
+// Runs the src, calling the callbacks that were registered with
+// upb_src_sethandlers(), and returning the status of the operation in
+// "status." The status might indicate UPB_TRYAGAIN (indicating EAGAIN on a
+// non-blocking socket) or a resumable error; in both cases upb_src_run can be
+// called again later.  TRYAGAIN could come from either the src (input buffers
+// are empty) or the handlers (output buffers are full).
+INLINE void upb_src_run(upb_src *src, upb_status *status);
 
 
 /* upb_bytesrc ****************************************************************/

core/upb_stream_vtbl.h

@@ -13,6 +13,7 @@
 
 #include <assert.h>
 #include "upb_stream.h"
+#include "upb_string.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -21,10 +22,8 @@ extern "C" {
 // Typedefs for function pointers to all of the virtual functions.
 
 // upb_src
-struct _upb_src {
-};
-typedef struct {
-} upb_src_vtbl;
+typedef void (*upb_src_sethandlers_fptr)(upb_src *src, upb_handlers *handlers);
+typedef void (*upb_src_run_fptr)(upb_src *src, upb_status *status);
 
 // upb_bytesrc.
 typedef upb_strlen_t (*upb_bytesrc_read_fptr)(
@@ -42,42 +41,65 @@ typedef upb_strlen_t (*upb_bytesink_putstr_fptr)(
 typedef struct {
   upb_bytesrc_read_fptr   read;
   upb_bytesrc_getstr_fptr getstr;
-} upb_bytesrc_vtable;
+} upb_bytesrc_vtbl;
 
 typedef struct {
   upb_bytesink_write_fptr  write;
   upb_bytesink_putstr_fptr putstr;
-} upb_bytesink_vtable;
+} upb_bytesink_vtbl;
+
+typedef struct {
+  upb_src_sethandlers_fptr sethandlers;
+  upb_src_run_fptr         run;
+} upb_src_vtbl;
+
 
 // "Base Class" definitions; components that implement these interfaces should
 // contain one of these structures.
 
 struct _upb_bytesrc {
-  upb_bytesrc_vtable *vtbl;
+  upb_bytesrc_vtbl *vtbl;
   upb_status status;
   bool eof;
 };
 
 struct _upb_bytesink {
-  upb_bytesink_vtable *vtbl;
+  upb_bytesink_vtbl *vtbl;
   upb_status status;
   bool eof;
 };
 
-INLINE void upb_bytesrc_init(upb_bytesrc *s, upb_bytesrc_vtable *vtbl) {
+struct _upb_src {
+  upb_src_vtbl *vtbl;
+};
+
+INLINE void upb_bytesrc_init(upb_bytesrc *s, upb_bytesrc_vtbl *vtbl) {
   s->vtbl = vtbl;
   s->eof = false;
   upb_status_init(&s->status);
 }
 
-INLINE void upb_bytesink_init(upb_bytesink *s, upb_bytesink_vtable *vtbl) {
+INLINE void upb_bytesink_init(upb_bytesink *s, upb_bytesink_vtbl *vtbl) {
   s->vtbl = vtbl;
   s->eof = false;
   upb_status_init(&s->status);
 }
 
+INLINE void upb_src_init(upb_src *s, upb_src_vtbl *vtbl) {
+  s->vtbl = vtbl;
+}
+
 // Implementation of virtual function dispatch.
 
+// upb_src
+INLINE void upb_src_sethandlers(upb_src *src, upb_handlers *handlers) {
+  src->vtbl->sethandlers(src, handlers);
+}
+
+INLINE void upb_src_run(upb_src *src, upb_status *status) {
+  src->vtbl->run(src, status);
+}
+
 // upb_bytesrc
 INLINE upb_strlen_t upb_bytesrc_read(upb_bytesrc *src, void *buf,
                                      upb_strlen_t count) {
@@ -152,7 +174,41 @@ INLINE bool upb_handlers_isempty(upb_handlers *h) {
   return !h->set && !h->closure;
 }
 
+INLINE upb_flow_t upb_nop(void *closure) {
+  (void)closure;
+  return UPB_CONTINUE;
+}
+
+INLINE upb_flow_t upb_value_nop(void *closure, struct _upb_fielddef *f, upb_value val) {
+  (void)closure;
+  (void)f;
+  (void)val;
+  return UPB_CONTINUE;
+}
+
+INLINE upb_flow_t upb_startsubmsg_nop(void *closure, struct _upb_fielddef *f,
+                                      upb_handlers *delegate_to) {
+  (void)closure;
+  (void)f;
+  (void)delegate_to;
+  return UPB_CONTINUE;
+}
+
+INLINE upb_flow_t upb_unknownval_nop(void *closure, upb_field_number_t fieldnum,
+                                     upb_value val) {
+  (void)closure;
+  (void)fieldnum;
+  (void)val;
+  return UPB_CONTINUE;
+}
+
 INLINE void upb_register_handlerset(upb_handlers *h, upb_handlerset *set) {
+  if (!set->startmsg) set->startmsg = &upb_nop;
+  if (!set->endmsg) set->endmsg = &upb_nop;
+  if (!set->value) set->value = &upb_value_nop;
+  if (!set->startsubmsg) set->startsubmsg = &upb_startsubmsg_nop;
+  if (!set->endsubmsg) set->endsubmsg = &upb_nop;
+  if (!set->unknownval) set->unknownval = &upb_unknownval_nop;
   h->set = set;
 }
 
@@ -182,16 +238,19 @@ INLINE void upb_dispatcher_reset(upb_dispatcher *d, upb_handlers *h) {
   d->top->handlers = *h;
 }
 
-INLINE void upb_dispatch_startmsg(upb_dispatcher *d) {
+INLINE upb_flow_t upb_dispatch_startmsg(upb_dispatcher *d) {
   assert(d->stack == d->top);
-  d->top->handlers.set->startmsg(d->top->handlers.closure);
+  return d->top->handlers.set->startmsg(d->top->handlers.closure);
 }
 
-INLINE void upb_dispatch_endmsg(upb_dispatcher *d) {
+INLINE upb_flow_t upb_dispatch_endmsg(upb_dispatcher *d) {
   assert(d->stack == d->top);
-  d->top->handlers.set->endmsg(d->top->handlers.closure);
+  return d->top->handlers.set->endmsg(d->top->handlers.closure);
 }
 
+// TODO: several edge cases to fix:
+// - delegated start returns UPB_BREAK, should replay the start on resume.
+// - endsubmsg returns UPB_BREAK, should NOT replay the delegated endmsg.
 INLINE upb_flow_t upb_dispatch_startsubmsg(upb_dispatcher *d,
                                            struct _upb_fielddef *f) {
   upb_handlers handlers;
@@ -203,17 +262,18 @@ INLINE upb_flow_t upb_dispatch_startsubmsg(upb_dispatcher *d,
     ++d->top;
     d->top->handlers = handlers;
     d->top->depth = 0;
-    d->top->handlers.set->startmsg(d->top->handlers.closure);
-    ret = UPB_CONTINUE;
+    ret = d->top->handlers.set->startmsg(d->top->handlers.closure);
   }
-  ++d->top->depth;
+  if (ret == UPB_CONTINUE) ++d->top->depth;
   upb_handlers_uninit(&handlers);
   return ret;
 }
 
 INLINE upb_flow_t upb_dispatch_endsubmsg(upb_dispatcher *d) {
+  upb_flow_t ret;
   if (--d->top->depth == 0) {
-    d->top->handlers.set->endmsg(d->top->handlers.closure);
+    ret = d->top->handlers.set->endmsg(d->top->handlers.closure);
+    if (ret != UPB_CONTINUE) return ret;
     --d->top;
   }
   return d->top->handlers.set->endsubmsg(d->top->handlers.closure);

core/upb_string.c

@@ -61,13 +61,13 @@ void _upb_string_free(upb_string *str) {
   free(str);
 }
 
-upb_string *upb_string_tryrecycle(upb_string *str) {
+void upb_string_recycle(upb_string **_str) {
+  upb_string *str = *_str;
   if(str && upb_atomic_read(&str->refcount) == 1) {
     str->ptr = NULL;
     upb_string_release(str);
-    return str;
   } else {
-    return upb_string_new();
+    *_str = upb_string_new();
   }
 }
 
@@ -111,7 +111,7 @@ void upb_string_vprintf(upb_string *str, const char *format, va_list args) {
     // We don't care about the terminating NULL, but snprintf might
     // bail out of printing even other characters if it doesn't have
     // enough space to write the NULL also.
-    str = upb_string_tryrecycle(str);
+    upb_string_recycle(&str);
     buf = upb_string_getrwbuf(str, true_size + 1);
     vsnprintf(buf, true_size + 1, format, args);
   }

core/upb_string.h

@@ -133,7 +133,7 @@ INLINE void upb_string_endread(upb_string *str) { (void)str; }
 //       upb_src_getstr(str);
 //     }
 //   }
-upb_string *upb_string_recycle(upb_string **str);
+void upb_string_recycle(upb_string **str);
 
 // The options for setting the contents of a string.  These may only be called
 // when a string is first created or recycled; once other functions have been

tests/test_def.c

@@ -10,6 +10,7 @@ int main() {
   int count;
   upb_def **defs = upb_symtab_getdefs(s, &count, UPB_DEF_ANY);
   for (int i = 0; i < count; i++) {
+    printf("Def with name: " UPB_STRFMT "\n", UPB_STRARG(defs[i]->fqname));
     upb_def_unref(defs[i]);
   }
   free(defs);

tests/test_string.c

@@ -23,7 +23,8 @@ static void test_static() {
   upb_string_unref(&static_upbstr);
 
   // Recycling a static string returns a new string (that can be modified).
-  upb_string *str = upb_string_tryrecycle(&static_upbstr);
+  upb_string *str = &static_upbstr;
+  upb_string_recycle(&str);
   assert(str != &static_upbstr);
 
   upb_string_unref(str);
@@ -34,8 +35,9 @@ static void test_dynamic() {
   assert(str != NULL);
   upb_string_unref(str);
 
-  // Can also create a string by tryrecycle(NULL).
-  str = upb_string_tryrecycle(NULL);
+  // Can also create a string by recycle(NULL).
+  str = NULL;
+  upb_string_recycle(&str);
   assert(str != NULL);
 
   upb_strcpyc(str, static_str);
@@ -45,7 +47,8 @@ static void test_dynamic() {
   assert(upb_streqlc(str, static_str));
   upb_string_endread(str);
 
-  upb_string *str2 = upb_string_tryrecycle(str);
+  upb_string *str2 = str;
+  upb_string_recycle(&str2);
   // No other referents, so should return the same string.
   assert(str2 == str);
 
@@ -58,7 +61,7 @@ static void test_dynamic() {
 
   // Make string alias part of another string.
   str2 = upb_strdupc("WXYZ");
-  str = upb_string_tryrecycle(str);
+  upb_string_recycle(&str);
   upb_string_substr(str, str2, 1, 2);
   assert(upb_string_len(str) == 2);
   assert(upb_string_len(str2) == 4);
@@ -70,7 +73,7 @@ static void test_dynamic() {
   assert(upb_atomic_read(&str2->refcount) == 2);
 
   // Recycling str should eliminate the extra ref.
-  str = upb_string_tryrecycle(str);
+  upb_string_recycle(&str);
   assert(upb_atomic_read(&str2->refcount) == 1);
 
   // Resetting str should reuse its old data.
@@ -80,7 +83,7 @@ static void test_dynamic() {
 
   // Resetting str to something very long should require new data to be
   // allocated.
-  str = upb_string_tryrecycle(str);
+  upb_string_recycle(&str);
   const char longstring[] = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX";
   upb_strcpyc(str, longstring);
   const char *robuf6 = upb_string_getrobuf(str);
@@ -88,7 +91,7 @@ static void test_dynamic() {
   assert(upb_streqlc(str, longstring));
 
   // Test printf.
-  str = upb_string_tryrecycle(str);
+  upb_string_recycle(&str);
   upb_string_printf(str, "Number: %d, String: %s", 5, "YO!");
   assert(upb_streqlc(str, "Number: 5, String: YO!"));