Merge pull request #4640 from googlefonts/gdef

[subset] When serializing GDEF ensure that varstore is packed last.

src/OT/Layout/GDEF/GDEF.hh

@@ -663,21 +663,16 @@ struct GDEFVersion1_2
     auto *out = c->serializer->start_embed (*this);
     if (unlikely (!c->serializer->extend_min (out))) return_trace (false);
 
-    out->version.major = version.major;
-    out->version.minor = version.minor;
-    bool subset_glyphclassdef = out->glyphClassDef.serialize_subset (c, glyphClassDef, this, nullptr, false, true);
-    bool subset_attachlist = out->attachList.serialize_subset (c, attachList, this);
-    bool subset_markattachclassdef = out->markAttachClassDef.serialize_subset (c, markAttachClassDef, this, nullptr, false, true);
-
-    bool subset_markglyphsetsdef = false;
+    // Push var store first (if it's needed) so that it's last in the
+    // serialization order. Some font consumers assume that varstore runs to
+    // the end of the GDEF table.
+    // See: https://github.com/harfbuzz/harfbuzz/issues/4636
     auto snapshot_version0 = c->serializer->snapshot ();
-    if (version.to_int () >= 0x00010002u)
-    {
-      if (unlikely (!c->serializer->embed (markGlyphSetsDef))) return_trace (false);
-      subset_markglyphsetsdef = out->markGlyphSetsDef.serialize_subset (c, markGlyphSetsDef, this);
-    }
+    if (unlikely (version.to_int () >= 0x00010002u && !c->serializer->embed (markGlyphSetsDef)))
+      return_trace (false);
 
     bool subset_varstore = false;
+    unsigned varstore_index = (unsigned) -1;
     auto snapshot_version2 = c->serializer->snapshot ();
     if (version.to_int () >= 0x00010003u)
     {
@@ -690,35 +685,58 @@ struct GDEFVersion1_2
         {
           item_variations_t item_vars;
           if (item_vars.instantiate (this+varStore, c->plan, true, true,
-                                     c->plan->gdef_varstore_inner_maps.as_array ()))
+                                     c->plan->gdef_varstore_inner_maps.as_array ())) {
             subset_varstore = out->varStore.serialize_serialize (c->serializer,
                                                                  item_vars.has_long_word (),
                                                                  c->plan->axis_tags,
                                                                  item_vars.get_region_list (),
                                                                  item_vars.get_vardata_encodings ());
+            varstore_index = c->serializer->last_added_child_index();
+          }
           remap_varidx_after_instantiation (item_vars.get_varidx_map (),
                                             c->plan->layout_variation_idx_delta_map);
         }
       }
       else
+      {
         subset_varstore = out->varStore.serialize_subset (c, varStore, this, c->plan->gdef_varstore_inner_maps.as_array ());
+        varstore_index = c->serializer->last_added_child_index();
+      }
+    }
+
+    out->version.major = version.major;
+    out->version.minor = version.minor;
+
+    if (!subset_varstore && version.to_int () >= 0x00010002u) {
+      c->serializer->revert (snapshot_version2);
     }
 
+    bool subset_markglyphsetsdef = false;
+    if (version.to_int () >= 0x00010002u)
+    {
+      subset_markglyphsetsdef = out->markGlyphSetsDef.serialize_subset (c, markGlyphSetsDef, this);
+    }
 
     if (subset_varstore)
     {
       out->version.minor = 3;
       c->plan->has_gdef_varstore = true;
     } else if (subset_markglyphsetsdef) {
-      out->version.minor = 2;
-      c->serializer->revert (snapshot_version2);
+      out->version.minor = 2;      
     } else  {
       out->version.minor = 0;
       c->serializer->revert (snapshot_version0);
     }
 
+    bool subset_glyphclassdef = out->glyphClassDef.serialize_subset (c, glyphClassDef, this, nullptr, false, true);
+    bool subset_attachlist = out->attachList.serialize_subset (c, attachList, this);
+    bool subset_markattachclassdef = out->markAttachClassDef.serialize_subset (c, markAttachClassDef, this, nullptr, false, true);
     bool subset_ligcaretlist = out->ligCaretList.serialize_subset (c, ligCaretList, this);
 
+    if (subset_varstore && varstore_index != (unsigned) -1) {
+      c->serializer->repack_last(varstore_index);
+    }
+
     return_trace (subset_glyphclassdef || subset_attachlist ||
 		  subset_ligcaretlist || subset_markattachclassdef ||
 		  (out->version.to_int () >= 0x00010002u && subset_markglyphsetsdef) ||
@@ -1013,7 +1031,7 @@ struct GDEF
     if (!has_var_store ()) return;
     const ItemVariationStore &var_store = get_var_store ();
     float *store_cache = var_store.create_cache ();
-    
+
     unsigned new_major = 0, new_minor = 0;
     unsigned last_major = (layout_variation_indices->get_min ()) >> 16;
     for (unsigned idx : layout_variation_indices->iter ())

src/hb-serialize.hh

@@ -91,6 +91,26 @@ struct hb_serialize_context_t
     }
 #endif
 
+    bool add_virtual_link (objidx_t objidx)
+    {
+      if (!objidx)
+        return false;
+
+      auto& link = *virtual_links.push ();
+      if (virtual_links.in_error ())
+        return false;
+
+      link.objidx = objidx;
+      // Remaining fields were previously zero'd by push():
+      // link.width = 0;
+      // link.is_signed = 0;
+      // link.whence = 0;
+      // link.position = 0;
+      // link.bias = 0;
+
+      return true;
+    }
+
     friend void swap (object_t& a, object_t& b) noexcept
     {
       hb_swap (a.head, b.head);
@@ -469,16 +489,40 @@ struct hb_serialize_context_t
 
     assert (current);
 
-    auto& link = *current->virtual_links.push ();
-    if (current->virtual_links.in_error ())
+    if (!current->add_virtual_link(objidx))
       err (HB_SERIALIZE_ERROR_OTHER);
+  }
 
-    link.width = 0;
-    link.objidx = objidx;
-    link.is_signed = 0;
-    link.whence = 0;
-    link.position = 0;
-    link.bias = 0;
+  objidx_t last_added_child_index() const {
+    if (unlikely (in_error ())) return (objidx_t) -1;
+
+    assert (current);
+    if (!bool(current->real_links)) {
+      return (objidx_t) -1;
+    }
+
+    return current->real_links[current->real_links.length - 1].objidx;
+  }
+
+  // For the current object ensure that the sub-table bytes for child objidx are always placed
+  // after the subtable bytes for any other existing children. This only ensures that the
+  // repacker will not move the target subtable before the other children
+  // (by adding virtual links). It is up to the caller to ensure the initial serialization
+  // order is correct.
+  void repack_last(objidx_t objidx) {
+    if (unlikely (in_error ())) return;
+
+    if (!objidx)
+      return;
+
+    assert (current);
+    for (auto& l : current->real_links) {
+      if (l.objidx == objidx) {
+        continue;
+      }
+
+      packed[l.objidx]->add_virtual_link(objidx);
+    }
   }
 
   template <typename T>

src/test-repacker.cc

@@ -433,16 +433,13 @@ struct MarkBasePosBuffers
   }
 };
 
-
-
-
-
 static void run_resolve_overflow_test (const char* name,
                                        hb_serialize_context_t& overflowing,
                                        hb_serialize_context_t& expected,
                                        unsigned num_iterations = 0,
                                        bool recalculate_extensions = false,
-                                       hb_tag_t tag = HB_TAG ('G', 'S', 'U', 'B'))
+                                       hb_tag_t tag = HB_TAG ('G', 'S', 'U', 'B'),
+                                       bool check_binary_equivalence = false)
 {
   printf (">>> Testing overflowing resolution for %s\n",
           name);
@@ -452,6 +449,10 @@ static void run_resolve_overflow_test (const char* name,
   graph_t expected_graph (expected.object_graph ());
   if (graph::will_overflow (expected_graph))
   {
+    if (check_binary_equivalence) {
+      printf("when binary equivalence checking is enabled, the expected graph cannot overflow.");
+      assert(!check_binary_equivalence);
+    }
     expected_graph.assign_spaces ();
     expected_graph.sort_shortest_distance ();
   }
@@ -464,12 +465,27 @@ static void run_resolve_overflow_test (const char* name,
                                       graph));
 
   // Check the graphs can be serialized.
-  hb_blob_t* out = graph::serialize (graph);
-  assert (out);
-  hb_blob_destroy (out);
-  out = graph::serialize (expected_graph);
-  assert (out);
-  hb_blob_destroy (out);
+  hb_blob_t* out1 = graph::serialize (graph);
+  assert (out1);
+  hb_blob_t* out2 = graph::serialize (expected_graph);
+  assert (out2);
+  if (check_binary_equivalence) {
+    unsigned l1, l2;
+    const char* d1 = hb_blob_get_data(out1, &l1);
+    const char* d2 = hb_blob_get_data(out2, &l2);
+
+    bool match = (l1 == l2) && (memcmp(d1, d2, l1) == 0);
+    if (!match) {
+      printf("## Result:\n");
+      graph.print();
+      printf("## Expected:\n");
+      expected_graph.print();
+      assert(match);
+    }
+  }
+
+  hb_blob_destroy (out1);
+  hb_blob_destroy (out2);
 
   // Check the graphs are equivalent
   graph.normalize ();
@@ -779,6 +795,54 @@ populate_serializer_with_isolation_overflow_spaces (hb_serialize_context_t* c)
   c->end_serialize();
 }
 
+static void
+populate_serializer_with_repack_last (hb_serialize_context_t* c, bool with_overflow)
+{
+  std::string large_string(70000, 'c');
+  c->start_serialize<char> ();
+  c->push();
+
+  // Obj E
+  unsigned obj_e_1, obj_e_2;
+  if (with_overflow) {
+    obj_e_1 = add_object("a", 1, c);
+    obj_e_2 = obj_e_1;
+  } else {
+    obj_e_2 = add_object("a", 1, c);
+  }
+
+  // Obj D
+  c->push();
+  add_offset(obj_e_2, c);
+  extend(large_string.c_str(), 30000, c);
+  unsigned obj_d = c->pop_pack(false);
+
+  add_offset(obj_d, c);
+  assert(c->last_added_child_index() == obj_d);
+
+  if (!with_overflow) {
+    obj_e_1 = add_object("a", 1, c);
+  }
+
+  // Obj C
+  c->push();
+  add_offset(obj_e_1, c);
+  extend(large_string.c_str(), 40000, c);
+  unsigned obj_c = c->pop_pack(false);
+
+  add_offset(obj_c, c);
+
+  // Obj B
+  unsigned obj_b = add_object("b", 1, c);
+  add_offset(obj_b, c);
+
+  // Obj A
+  c->repack_last(obj_d);
+  c->pop_pack(false);
+
+  c->end_serialize();
+}
+
 static void
 populate_serializer_spaces (hb_serialize_context_t* c, bool with_overflow)
 {
@@ -2167,6 +2231,31 @@ test_shared_node_with_virtual_links ()
   free(buffer);
 }
 
+static void
+test_repack_last ()
+{
+  size_t buffer_size = 200000;
+  void* buffer = malloc (buffer_size);
+  assert (buffer);
+  hb_serialize_context_t c (buffer, buffer_size);
+  populate_serializer_with_repack_last (&c, true);
+
+  void* expected_buffer = malloc (buffer_size);
+  assert (expected_buffer);
+  hb_serialize_context_t e (expected_buffer, buffer_size);
+  populate_serializer_with_repack_last (&e, false);
+
+  run_resolve_overflow_test ("test_repack_last",
+                             c,
+                             e,
+                             20,
+                             false,
+                             HB_TAG('a', 'b', 'c', 'd'),
+                             true);
+
+  free (buffer);
+  free (expected_buffer);
+}
 
 // TODO(garretrieger): update will_overflow tests to check the overflows array.
 // TODO(garretrieger): add tests for priority raising.
@@ -2195,6 +2284,7 @@ main (int argc, char **argv)
   test_duplicate_leaf ();
   test_duplicate_interior ();
   test_virtual_link ();
+  test_repack_last();
   test_shared_node_with_virtual_links ();
   test_resolve_with_extension_promotion ();
   test_resolve_with_shared_extension_promotion ();