From a417be0f8780fd596b06159079d7c377500026c6 Mon Sep 17 00:00:00 2001
From: Joshua Haberman <joshua@reverberate.org>
Date: Sat, 3 Jul 2010 12:34:09 -0700
Subject: [PATCH] More work on upb_def.
---
src/upb_def.c | 168 +++++++++++++++++++++++++++++++++--------------
src/upb_def.h | 2 +-
src/upb_string.h | 2 +-
3 files changed, 119 insertions(+), 53 deletions(-)
diff --git a/src/upb_def.c b/src/upb_def.c
index e78fb0782d..088dd0d6ee 100644
--- a/src/upb_def.c
+++ b/src/upb_def.c
@@ -8,8 +8,6 @@
#include "descriptor_const.h"
#include "upb_def.h"
-/* Rounds p up to the next multiple of t. */
-#define ALIGN_UP(p, t) ((p) % (t) == 0 ? (p) : (p) + ((t) - ((p) % (t))))
#define CHECKSRC(x) if(!(x)) goto src_err
#define CHECK(x) if(!(x)) goto err
@@ -111,9 +109,6 @@ static void def_free(upb_def *def)
case UPB_DEF_SVC:
assert(false); /* Unimplemented. */
break;
- case UPB_DEF_EXT:
- assert(false); /* Unimplemented. */
- break;
case UPB_DEF_UNRESOLVED:
upb_unresolveddef_free(upb_downcast_unresolveddef(def));
break;
@@ -202,14 +197,20 @@ static void upb_def_uninit(upb_def *def) {
/* upb_unresolveddef **********************************************************/
+// Unresolved defs are used as temporary placeholders for a def whose name has
+// not been resolved yet. During the name resolution step, all unresolved defs
+// are replaced with pointers to the actual def being referenced.
typedef struct _upb_unresolveddef {
upb_def base;
+
+ // The target type name. This may or may not be fully qualified.
+ upb_string *name;
} upb_unresolveddef;
static upb_unresolveddef *upb_unresolveddef_new(upb_string *str) {
upb_unresolveddef *def = malloc(sizeof(*def));
upb_def_init(&def->base, UPB_DEF_UNRESOLVED);
- def->base.fqname = upb_string_getref(str);
+ def->name = upb_string_getref(str);
return def;
}
@@ -615,6 +616,59 @@ static bool upb_symtab_findcycles(upb_msgdef *m, int depth, upb_status *status)
}
}
+// Given a table of pending defs "tmptab" and a table of existing defs "symtab",
+// resolves all of the unresolved refs for the defs in tmptab.
+bool upb_resolverefs(upb_strtable *tmptab, upb_strtable *symtab,
+ upb_status *status)
+{
+ upb_symtab_ent *e;
+ for(e = upb_strtable_begin(tmptab); e; e = upb_strtable_next(tmptab, &e->e)) {
+ upb_msgdef *m = upb_dyncast_msgdef(e->def);
+ if(!m) continue;
+ // Type names are resolved relative to the message in which they appear.
+ upb_string *base = e->e.key;
+
+ for(upb_field_count_t i = 0; i < m->num_fields; i++) {
+ upb_fielddef *f = &m->fields[i];
+ if(!upb_hasdef(f)) continue; // No resolving necessary.
+ upb_string *name = upb_downcast_unresolveddef(f->def)->name;
+
+ // Resolve from either the tmptab (pending adds) or symtab (existing
+ // defs). If both exist, prefer the pending add, because it will be
+ // overwriting the existing def.
+ upb_symtab_ent *found;
+ if(!(found = upb_resolve(tmptab, base, name)) &&
+ !(found = upb_resolve(symtab, base, name))) {
+ upb_seterr(status, UPB_STATUS_ERROR,
+ "could not resolve symbol '" UPB_STRFMT "'"
+ " in context '" UPB_STRFMT "'",
+ UPB_STRARG(name), UPB_STRARG(base));
+ return false;
+ }
+
+ // Check the type of the found def.
+ upb_field_type_t expected = upb_issubmsg(f) ? UPB_DEF_MSG : UPB_DEF_ENUM;
+ if(found->def->type != expected) {
+ upb_seterr(status, UPB_STATUS_ERROR, "Unexpected type");
+ return false;
+ }
+ upb_msgdef_resolve(m, f, found->def);
+ }
+ }
+
+ // Deal with type cycles.
+ for(e = upb_strtable_begin(tmptab); e; e = upb_strtable_next(tmptab, &e->e)) {
+ upb_msgdef *m = upb_dyncast_msgdef(e->def);
+ if(!m) continue;
+ // The findcycles() call will decrement the external refcount of the
+ if(!upb_symtab_findcycles(m, 0, status)) return false;
+ upb_msgdef *open_defs[UPB_MAX_TYPE_CYCLE_LEN];
+ cycle_ref_or_unref(m, NULL, open_defs, 0, true);
+ }
+
+ return true;
+}
+
// Given a list of defs, a list of extensions (in the future), and a flag
// indicating whether the new defs can overwrite existing defs in the symtab,
// attempts to add the given defs to the symtab. The whole operation either
@@ -622,55 +676,67 @@ static bool upb_symtab_findcycles(upb_msgdef *m, int depth, upb_status *status)
bool upb_symtab_add_defs(upb_symtab *s, upb_deflist *defs, bool allow_redef,
upb_status *status)
{
- (void)s;
- (void)defs;
- (void)allow_redef;
- (void)status;
- return true;
-#if 0
- // Build a table, for duplicate detection and name resolution.
-
- { // Write lock scope.
- // Attempt to resolve all references.
- upb_symtab_ent *e;
- for(e = upb_strtable_begin(addto); e; e = upb_strtable_next(addto, &e->e)) {
- upb_msgdef *m = upb_dyncast_msgdef(e->def);
- if(!m) continue;
- upb_string *base = e->e.key;
- for(upb_field_count_t i = 0; i < m->num_fields; i++) {
- upb_fielddef *f = &m->fields[i];
- if(!upb_hasdef(f)) continue; // No resolving necessary.
- upb_string *name = upb_downcast_unresolveddef(f->def)->name;
- upb_symtab_ent *found = resolve(existingdefs, base, name);
- if(!found) found = resolve(addto, base, name);
- upb_field_type_t expected = upb_issubmsg(f) ? UPB_DEF_MSG : UPB_DEF_ENUM;
- if(!found) {
- upb_seterr(status, UPB_STATUS_ERROR,
- "could not resolve symbol '" UPB_STRFMT "'"
- " in context '" UPB_STRFMT "'",
- UPB_STRARG(name), UPB_STRARG(base));
- return;
- } else if(found->def->type != expected) {
- upb_seterr(status, UPB_STATUS_ERROR, "Unexpected type");
- return;
- }
- upb_msgdef_resolve(m, f, found->def);
- }
+ upb_rwlock_wrlock(&s->lock);
+
+ // Build a table of the defs we mean to add, for duplicate detection and name
+ // resolution.
+ upb_strtable tmptab;
+ upb_strtable_init(&tmptab, defs->len, sizeof(upb_symtab_ent));
+ for (uint32_t i = 0; i < defs->len; i++) {
+ upb_def *def = defs->defs[i];
+ upb_symtab_ent e = {{def->fqname, 0}, def};
+
+ // Redefinition is never allowed within a single FileDescriptorSet.
+ // Additionally, we only allow overwriting of an existing definition if
+ // allow_redef is set.
+ if (upb_strtable_lookup(&tmptab, def->fqname) ||
+ (!allow_redef && upb_strtable_lookup(&s->symtab, def->fqname))) {
+ upb_seterr(status, UPB_STATUS_ERROR, "Redefinition of symbol " UPB_STRFMT,
+ UPB_STRARG(def->fqname));
+ goto err;
}
- // Deal with type cycles.
- for(e = upb_strtable_begin(addto); e; e = upb_strtable_next(addto, &e->e)) {
- upb_msgdef *m = upb_dyncast_msgdef(e->def);
- if(!m) continue;
- // The findcycles() call will decrement the external refcount of the
- upb_symtab_findcycles(m, 0, status);
- upb_msgdef *open_defs[UPB_MAX_TYPE_CYCLE_LEN];
- cycle_ref_or_unref(m, NULL, open_defs, 0, true);
- }
+ // Pass ownership from the deflist to the strtable.
+ upb_strtable_insert(&tmptab, &e.e);
+ defs->defs[i] = NULL;
+ }
- // Add all defs to the symtab.
+ // TODO: process the list of extensions by modifying entries from
+ // tmptab in-place (copying them from the symtab first if necessary).
+
+ CHECK(upb_resolverefs(&tmptab, &s->symtab, status));
+
+ // The defs in tmptab have been vetted, and can be added to the symtab
+ // without causing errors. Now add all tmptab defs to the symtab,
+ // overwriting (and releasing a ref on) any existing defs with the same
+ // names. Ownership for tmptab defs passes from the tmptab to the symtab.
+ upb_symtab_ent *tmptab_e;
+ for(tmptab_e = upb_strtable_begin(&tmptab); tmptab_e;
+ tmptab_e = upb_strtable_next(&tmptab, &tmptab_e->e)) {
+ upb_symtab_ent *symtab_e =
+ upb_strtable_lookup(&s->symtab, tmptab_e->def->fqname);
+ if(symtab_e) {
+ upb_def_unref(symtab_e->def);
+ symtab_e->def = tmptab_e->def;
+ } else {
+ upb_strtable_insert(&s->symtab, &tmptab_e->e);
+ }
}
-#endif
+
+ upb_rwlock_unlock(&s->lock);
+ upb_strtable_free(&tmptab);
+ return true;
+
+err:
+ // We need to free all defs that are in either "defs" or "tmptab."
+ upb_rwlock_unlock(&s->lock);
+ for (uint32_t i = 0; i < defs->len; i++)
+ if(defs->defs[i] != NULL) free(defs->defs[i]);
+ for(upb_symtab_ent *e = upb_strtable_begin(&tmptab); e;
+ e = upb_strtable_next(&tmptab, &e->e))
+ upb_def_unref(e->def);
+ upb_strtable_free(&tmptab);
+ return false;
}
diff --git a/src/upb_def.h b/src/upb_def.h
index 3063386210..033dcdec82 100644
--- a/src/upb_def.h
+++ b/src/upb_def.h
@@ -248,7 +248,7 @@ upb_def *upb_symtab_lookup(upb_symtab *s, upb_string *sym);
upb_def **upb_symtab_getdefs(upb_symtab *s, int *count, upb_def_type_t type);
// "fds" is a upb_src that will yield data from the
-// google.protobuf.FileDescriptorSet message type. upb_symtab_add_fds() adds
+// google.protobuf.FileDescriptorSet message type. upb_symtab_addfds() adds
// all the definitions from the given FileDescriptorSet and adds them to the
// symtab. status indicates whether the operation was successful or not, and
// the error message (if any).
diff --git a/src/upb_string.h b/src/upb_string.h
index 5e5d6bc0c3..af1f8ce490 100644
--- a/src/upb_string.h
+++ b/src/upb_string.h
@@ -101,7 +101,7 @@ void upb_string_detach(upb_string *str);
// Allows using upb_strings in printf, ie:
// upb_strptr str = UPB_STRLIT("Hello, World!\n");
// printf("String is: " UPB_STRFMT, UPB_STRARG(str)); */
-#define UPB_STRARG(str) upb_strlen(str), upb_string_getrobuf(str)
+#define UPB_STRARG(str) upb_string_len(str), upb_string_getrobuf(str)
#define UPB_STRFMT "%.*s"
/* upb_string library functions ***********************************************/