Merge pull request #525 from haberman/enum-fixes

Fixed two bugs with proto2 enums

upb/def.c

@@ -1623,7 +1623,10 @@ static void make_layout(symtab_addctx* ctx, const upb_MessageDef* m) {
   l->size = UPB_ALIGN_UP(l->size, 8);
 
   /* Sort fields by number. */
-  qsort(fields, upb_MessageDef_numfields(m), sizeof(*fields), field_number_cmp);
+  if (fields) {
+    qsort(fields, upb_MessageDef_numfields(m), sizeof(*fields),
+          field_number_cmp);
+  }
   assign_layout_indices(m, l, fields);
 }
 
@@ -2397,6 +2400,12 @@ static int count_bits_debug(uint64_t x) {
   return n;
 }
 
+static int compare_int32(const void* a_ptr, const void* b_ptr) {
+  int32_t a = *(int32_t*)a_ptr;
+  int32_t b = *(int32_t*)b_ptr;
+  return a < b ? -1 : (a == b ? 0 : 1);
+}
+
 upb_MiniTable_Enum* create_enumlayout(symtab_addctx* ctx,
                                       const upb_EnumDef* e) {
   int n = 0;
@@ -2405,7 +2414,7 @@ upb_MiniTable_Enum* create_enumlayout(symtab_addctx* ctx,
   for (int i = 0; i < e->value_count; i++) {
     uint32_t val = (uint32_t)e->values[i].number;
     if (val < 64) {
-      mask |= 1 << val;
+      mask |= 1ULL << val;
     } else {
       n++;
     }
@@ -2427,6 +2436,17 @@ upb_MiniTable_Enum* create_enumlayout(symtab_addctx* ctx,
     UPB_ASSERT(p == values + n);
   }
 
+  // Enums can have duplicate values; we must sort+uniq them.
+  if (values) qsort(values, n, sizeof(*values), &compare_int32);
+
+  int dst = 0;
+  for (int i = 0; i < n; dst++) {
+    int32_t val = values[i];
+    while (i < n && values[i] == val) i++;  // Skip duplicates.
+    values[dst] = val;
+  }
+  n = dst;
+
   UPB_ASSERT(upb_inttable_count(&e->iton) == n + count_bits_debug(mask));
 
   upb_MiniTable_Enum* layout = symtab_alloc(ctx, sizeof(*layout));
@@ -2510,7 +2530,7 @@ static void create_enumdef(
     if (ctx->layout) {
       UPB_ASSERT(ctx->enum_count < ctx->layout->enum_count);
       e->layout = ctx->layout->enums[ctx->enum_count++];
-      UPB_ASSERT(n ==
+      UPB_ASSERT(upb_inttable_count(&e->iton) ==
                  e->layout->value_count + count_bits_debug(e->layout->mask));
     } else {
       e->layout = create_enumlayout(ctx, e);
@@ -3085,7 +3105,8 @@ const upb_FileDef* upb_DefPool_AddFile(
 /* Include here since we want most of this file to be stdio-free. */
 #include <stdio.h>
 
-bool _upb_DefPool_LoadDefInit(upb_DefPool* s, const _upb_DefPool_Init* init) {
+bool _upb_DefPool_LoadDefInitEx(upb_DefPool* s, const _upb_DefPool_Init* init,
+                                bool rebuild_minitable) {
   /* Since this function should never fail (it would indicate a bug in upb) we
    * print errors to stderr instead of returning error status to the user. */
   _upb_DefPool_Init** deps = init->deps;
@@ -3102,7 +3123,7 @@ bool _upb_DefPool_LoadDefInit(upb_DefPool* s, const _upb_DefPool_Init* init) {
   arena = upb_Arena_New();
 
   for (; *deps; deps++) {
-    if (!_upb_DefPool_LoadDefInit(s, *deps)) goto err;
+    if (!_upb_DefPool_LoadDefInitEx(s, *deps, rebuild_minitable)) goto err;
   }
 
   file = google_protobuf_FileDescriptorProto_parse_ex(
@@ -3119,7 +3140,8 @@ bool _upb_DefPool_LoadDefInit(upb_DefPool* s, const _upb_DefPool_Init* init) {
     goto err;
   }
 
-  if (!_upb_DefPool_AddFile(s, file, init->layout, &status)) {
+  const upb_MiniTable_File* mt = rebuild_minitable ? NULL : init->layout;
+  if (!_upb_DefPool_AddFile(s, file, mt, &status)) {
     goto err;
   }
 

upb/def.h

@@ -389,7 +389,15 @@ typedef struct _upb_DefPool_Init {
   upb_StringView descriptor; /* Serialized descriptor. */
 } _upb_DefPool_Init;
 
-bool _upb_DefPool_LoadDefInit(upb_DefPool* s, const _upb_DefPool_Init* init);
+// Should only be directly called by tests.  This variant lets us suppress
+// the use of compiled-in tables, forcing a rebuild of the tables at runtime.
+bool _upb_DefPool_LoadDefInitEx(upb_DefPool* s, const _upb_DefPool_Init* init,
+                                bool rebuild_minitable);
+
+UPB_INLINE bool _upb_DefPool_LoadDefInit(upb_DefPool* s,
+                                         const _upb_DefPool_Init* init) {
+  return _upb_DefPool_LoadDefInitEx(s, init, false);
+}
 
 #include "upb/port_undef.inc"
 

upb/util/def_to_proto_test.cc

@@ -125,3 +125,19 @@ TEST(DefToProto, Test) {
   upb::FileDefPtr file = msgdef.file();
   CheckFile(file, file_desc);
 }
+
+// Like the previous test, but uses a message layout built at runtime.
+TEST(DefToProto, TestRuntimeReflection) {
+  upb::Arena arena;
+  upb::SymbolTable symtab;
+  upb_StringView test_file_desc =
+      upb_util_def_to_proto_test_proto_upbdefinit.descriptor;
+  const auto* file_desc = google_protobuf_FileDescriptorProto_parse(
+      test_file_desc.data, test_file_desc.size, arena.ptr());
+
+  _upb_DefPool_LoadDefInitEx(
+      symtab.ptr(), &upb_util_def_to_proto_test_proto_upbdefinit, true);
+  upb::FileDefPtr file = symtab.FindFileByName(
+      upb_util_def_to_proto_test_proto_upbdefinit.filename);
+  CheckFile(file, file_desc);
+}

upb/util/def_to_proto_test.proto

@@ -69,6 +69,26 @@ enum Enum {
   NEGATIVE_ONE = -1;
 }
 
+enum EnumUpper32Value {
+  UPPER32_VALUE = 40;
+}
+
+enum HasDuplicateValues {
+  option allow_alias = true;
+  A = 0;
+  B = 1;
+  C = 120;
+  D = 130;
+
+  G = 120;
+  F = 1;
+  E = 0;
+  H = 121;
+  I = 121;
+  J = 121;
+  K = 121;
+}
+
 service Service {
   rpc Bar(Message) returns (Message);
 }
@@ -77,6 +97,10 @@ extend Message {
   optional int32 ext = 1001;
 }
 
+enum Has31 {
+  VALUE_31 = 31;
+}
+
 message PretendMessageSet {
   option message_set_wire_format = true;
   // Since this is message_set_wire_format, "max" here means INT32_MAX.

upbc/protoc-gen-upb.cc

@@ -95,6 +95,18 @@ std::vector<const protobuf::EnumDescriptor*> SortedEnums(
   return enums;
 }
 
+std::vector<int32_t> SortedUniqueEnumNumbers(
+    const protobuf::EnumDescriptor* e) {
+  std::vector<int32_t> values;
+  for (int i = 0; i < e->value_count(); i++) {
+    values.push_back(e->value(i)->number());
+  }
+  std::sort(values.begin(), values.end());
+  auto last = std::unique(values.begin(), values.end());
+  values.erase(last, values.end());
+  return values;
+}
+
 void AddMessages(const protobuf::Descriptor* message,
                  std::vector<const protobuf::Descriptor*>* messages) {
   messages->push_back(message);
@@ -1300,23 +1312,19 @@ int WriteEnums(const protobuf::FileDescriptor* file, Output& output) {
 
   for (const auto* e : this_file_enums) {
     uint64_t mask = 0;
-    absl::flat_hash_set<int32_t> values;
+    std::vector<int32_t> values;
-    for (int i = 0; i < e->value_count(); i++) {
+    for (auto number : SortedUniqueEnumNumbers(e)) {
-      int32_t number = e->value(i)->number();
       if (static_cast<uint32_t>(number) < 64) {
-        mask |= 1 << number;
+        mask |= 1ULL << number;
       } else {
-        values.insert(number);
+        values.push_back(number);
       }
     }
-    std::vector<int32_t> values_vec(values.begin(), values.end());
-    std::sort(values_vec.begin(), values_vec.end());
 
-    if (!values_vec.empty()) {
+    if (!values.empty()) {
       values_init = EnumInit(e) + "_values";
-      output("static const int32_t $0[$1] = {\n", values_init,
+      output("static const int32_t $0[$1] = {\n", values_init, values.size());
-             values_vec.size());
+      for (int32_t value : values) {
-      for (auto value : values_vec) {
         output("  $0,\n", value);
       }
       output("};\n\n");
@@ -1325,7 +1333,7 @@ int WriteEnums(const protobuf::FileDescriptor* file, Output& output) {
     output("const upb_MiniTable_Enum $0 = {\n", EnumInit(e));
     output("  $0,\n", values_init);
     output("  0x$0ULL,\n", absl::Hex(mask));
-    output("  $0,\n", values_vec.size());
+    output("  $0,\n", values.size());
 
     output("};\n\n");
   }