[subset] add virtual links to keep the gdef varstore last.

src/OT/Layout/GDEF/GDEF.hh

@@ -663,14 +663,15 @@ struct GDEFVersion1_2
     auto *out = c->serializer->start_embed (*this);
     if (unlikely (!c->serializer->extend_min (out))) return_trace (false);
 
-    // Serialize var store first (if it's needed) so that it's serialized last. Some font parsers assume
-    // that varstore runs to the end of the GDEF table.
-    // TODO(garretrieger): add a virtual link from all non-var store sub tables to the var store.
+    // 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.
     auto snapshot_version0 = c->serializer->snapshot ();
     if (unlikely (version.to_int () >= 0x00010002u && !c->serializer->embed (markGlyphSetsDef)))
       return_trace (false);
 
     bool subset_varstore = false;
+    unsigned varstore_index = -1;
     auto snapshot_version2 = c->serializer->snapshot ();
     if (version.to_int () >= 0x00010003u)
     {
@@ -683,21 +684,25 @@ 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;
 
@@ -727,6 +732,10 @@ struct GDEFVersion1_2
     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) ||

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>