From 806ba1c80d86bd59759cf59efc057662eecbcf65 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sat, 5 Feb 2011 22:07:10 -0800 Subject: [PATCH] Another round of fixes. test_vs_proto2.googlemessage1 passes again, with no memory leaks! --- core/upb.c | 42 ++++++++++++++++++++--------------------- core/upb.h | 2 ++ core/upb_def.c | 7 +++---- core/upb_msg.c | 9 +++++---- core/upb_msg.h | 21 +++++++++++++++++---- core/upb_stream.h | 3 ++- core/upb_stream_vtbl.h | 7 +++++-- core/upb_string.c | 2 +- core/upb_string.h | 10 +++++----- stream/upb_decoder.c | 10 ++++++---- tests/test_stream.c | 2 +- tests/test_vs_proto2.cc | 12 +++++++++++- 12 files changed, 79 insertions(+), 48 deletions(-) diff --git a/core/upb.c b/core/upb.c index 525c8a8cdc..897ca4e98c 100644 --- a/core/upb.c +++ b/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, diff --git a/core/upb.h b/core/upb.h index 779f85ad39..837fc52bec 100644 --- a/core/upb.h +++ b/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)); diff --git a/core/upb_def.c b/core/upb_def.c index 993d9e3dba..298ede1079 100644 --- a/core/upb_def.c +++ b/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) { diff --git a/core/upb_msg.c b/core/upb_msg.c index 1ad73bc574..f628e3cb29 100644 --- a/core/upb_msg.c +++ b/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) { diff --git a/core/upb_msg.h b/core/upb_msg.h index 8e04c95750..475b346398 100644 --- a/core/upb_msg.h +++ b/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); diff --git a/core/upb_stream.h b/core/upb_stream.h index aa23549855..3f7c843636 100644 --- a/core/upb_stream.h +++ b/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, diff --git a/core/upb_stream_vtbl.h b/core/upb_stream_vtbl.h index da3eac820d..e1f9cb8ecd 100644 --- a/core/upb_stream_vtbl.h +++ b/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; } diff --git a/core/upb_string.c b/core/upb_string.c index bc5b7729fd..297583bf3c 100644 --- a/core/upb_string.c +++ b/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 { diff --git a/core/upb_string.h b/core/upb_string.h index 7d0ae87927..4943cbf187 100644 --- a/core/upb_string.h +++ b/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. diff --git a/stream/upb_decoder.c b/stream/upb_decoder.c index 78710b9a60..b85abb93ff 100644 --- a/stream/upb_decoder.c +++ b/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; diff --git a/tests/test_stream.c b/tests/test_stream.c index b6d511c042..468d40cd8e 100644 --- a/tests/test_stream.c +++ b/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; diff --git a/tests/test_vs_proto2.cc b/tests/test_vs_proto2.cc index bf0296cb02..9633420ae7 100644 --- a/tests/test_vs_proto2.cc +++ b/tests/test_vs_proto2.cc @@ -79,6 +79,7 @@ void compare_arrays(const google::protobuf::Reflection *r, } } +#include 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; }