[repacker] Handle the case where a subgraph root has an incoming 32 and 16 bit edge.

In this case the entire subgraph from that root will be duplicated.

src/hb-repacker.hh

@@ -416,22 +416,21 @@ struct graph_t
    * that originate from outside of the subgraph will be removed by duplicating the linked to
    * object.
    */
-  bool isolate_subgraph (hb_set_t roots)
+  bool isolate_subgraph (const hb_set_t& roots)
   {
     update_parents ();
     hb_hashmap_t<unsigned, unsigned> subgraph;
 
     // incoming edges to root_idx should be all 32 bit in length so we don't need to de-dup these
     // set the subgraph incoming edge count to match all of root_idx's incoming edges
-    //
-    // TODO(grieger): the above assumption does not always hold, as there are 16 bit incoming
-    //                edges, handle that case here by not including them in the count.
+    hb_set_t parents;
     for (unsigned root_idx : roots)
     {
-      subgraph.set (root_idx, vertices_[root_idx].incoming_edges ());
+      subgraph.set (root_idx, wide_parents (root_idx, parents));
       find_subgraph (root_idx, subgraph);
     }
 
+    unsigned original_root_idx = root_idx ();
     hb_hashmap_t<unsigned, unsigned> index_map;
     bool made_changes = false;
     for (auto entry : subgraph.iter ())
@@ -450,6 +449,14 @@ struct graph_t
     if (!made_changes)
       return false;
 
+    if (original_root_idx != root_idx ()
+        && parents.has (original_root_idx))
+    {
+      // If the root idx has changed since parents was determined, update root idx in parents
+      parents.add (root_idx ());
+      parents.del (original_root_idx);
+    }
+
     auto new_subgraph =
         + subgraph.keys ()
         | hb_map([&] (unsigned node_idx) {
@@ -457,7 +464,10 @@ struct graph_t
           return node_idx;
         })
         ;
+
     remap_obj_indices (index_map, new_subgraph);
+    remap_obj_indices (index_map, parents.iter ());
+
     return true;
   }
 
@@ -538,6 +548,10 @@ struct graph_t
     vertices_[clone_idx] = *clone;
     vertices_[vertices_.length - 1] = root;
 
+    // Since the root moved, update the parents arrays of all children on the root.
+    for (const auto& l : root.obj.links)
+      vertices_[l.objidx].remap_parent (root_idx () - 1, root_idx ());
+
     return clone_idx;
   }
 
@@ -571,15 +585,12 @@ struct graph_t
     unsigned clone_idx = duplicate (child_idx);
     if (clone_idx == (unsigned) -1) return false;
     // duplicate shifts the root node idx, so if parent_idx was root update it.
-    unsigned original_parent_idx = parent_idx;
     if (parent_idx == clone_idx) parent_idx++;
 
     auto& parent = vertices_[parent_idx];
     for (unsigned i = 0; i < parent.obj.links.length; i++)
     {
       auto& l = parent.obj.links[i];
-      if (original_parent_idx != parent_idx)
-        vertices_[l.objidx].remap_parent (original_parent_idx, parent_idx);
       if (l.objidx != child_idx)
         continue;
 
@@ -657,6 +668,26 @@ struct graph_t
 
  private:
 
+  /*
+   * Returns the numbers of incoming edges that are 32bits wide.
+   */
+  unsigned wide_parents (unsigned node_idx, hb_set_t& parents) const
+  {
+    unsigned count = 0;
+    for (unsigned p : vertices_[node_idx].parents)
+    {
+      for (const auto& l : vertices_[p].obj.links)
+      {
+        if (l.objidx == node_idx && l.width == 4 && !l.is_signed)
+        {
+          count++;
+          parents.add (p);
+        }
+      }
+    }
+    return count;
+  }
+
   bool check_success (bool success)
   { return this->successful && (success || (err_other_error (), false)); }
 
@@ -673,7 +704,9 @@ struct graph_t
     for (unsigned p = 0; p < vertices_.length; p++)
     {
       for (auto& l : vertices_[p].obj.links)
+      {
         vertices_[l.objidx].parents.push (p);
+      }
     }
 
     parents_invalid = false;

src/test-repacker.cc

@@ -415,6 +415,74 @@ populate_serializer_spaces_16bit_connection_expected (hb_serialize_context_t* c)
   c->end_serialize ();
 }
 
+static void
+populate_serializer_short_and_wide_subgraph_root (hb_serialize_context_t* c)
+{
+  std::string large_string(70000, 'a');
+  c->start_serialize<char> ();
+
+  unsigned obj_e = add_object ("e", 1, c);
+
+  start_object (large_string.c_str (), 40000, c);
+  add_offset (obj_e, c);
+  unsigned obj_c = c->pop_pack (false);
+
+  start_object (large_string.c_str (), 40000, c);
+  add_offset (obj_c, c);
+  unsigned obj_d = c->pop_pack (false);
+
+  start_object ("b", 1, c);
+  add_offset (obj_c, c);
+  add_offset (obj_e, c);
+  unsigned obj_b = c->pop_pack (false);
+
+  start_object ("a", 1, c);
+  add_offset (obj_b, c);
+  add_wide_offset (obj_c, c);
+  add_wide_offset (obj_d, c);
+  c->pop_pack (false);
+
+  c->end_serialize();
+}
+
+static void
+populate_serializer_short_and_wide_subgraph_root_expected (hb_serialize_context_t* c)
+{
+  std::string large_string(70000, 'a');
+  c->start_serialize<char> ();
+
+  unsigned obj_e_prime = add_object ("e", 1, c);
+
+  start_object (large_string.c_str (), 40000, c);
+  add_offset (obj_e_prime, c);
+  unsigned obj_c_prime = c->pop_pack (false);
+
+  start_object (large_string.c_str (), 40000, c);
+  add_offset (obj_c_prime, c);
+  unsigned obj_d = c->pop_pack (false);
+
+  unsigned obj_e = add_object ("e", 1, c);
+
+  start_object (large_string.c_str (), 40000, c);
+  add_offset (obj_e, c);
+  unsigned obj_c = c->pop_pack (false);
+
+
+  start_object ("b", 1, c);
+  add_offset (obj_c, c);
+  add_offset (obj_e, c);
+  unsigned obj_b = c->pop_pack (false);
+
+  start_object ("a", 1, c);
+  add_offset (obj_b, c);
+  add_wide_offset (obj_c_prime, c);
+  add_wide_offset (obj_d, c);
+  c->pop_pack (false);
+
+  c->end_serialize();
+}
+
+
 static void
 populate_serializer_complex_1 (hb_serialize_context_t* c)
 {
@@ -890,6 +958,42 @@ static void test_resolve_overflows_via_isolating_16bit_space ()
   free (out_buffer);
 }
 
+static void test_resolve_overflows_via_isolating_16bit_space_2 ()
+{
+  size_t buffer_size = 160000;
+  void* buffer = malloc (buffer_size);
+  hb_serialize_context_t c (buffer, buffer_size);
+  populate_serializer_short_and_wide_subgraph_root (&c);
+  graph_t graph (c.object_graph ());
+
+  void* out_buffer = malloc (buffer_size);
+  hb_serialize_context_t out (out_buffer, buffer_size);
+
+  assert (c.offset_overflow ());
+  hb_resolve_overflows (c.object_graph (), HB_TAG ('G', 'S', 'U', 'B'), &out, 0);
+  assert (!out.offset_overflow ());
+  hb_bytes_t result = out.copy_bytes ();
+
+  void* expected_buffer = malloc (buffer_size);
+  hb_serialize_context_t e (expected_buffer, buffer_size);
+  assert (!e.offset_overflow ());
+  populate_serializer_short_and_wide_subgraph_root_expected (&e);
+  hb_bytes_t expected_result = e.copy_bytes ();
+
+  assert (result.length == expected_result.length);
+  for (unsigned i = 0; i < expected_result.length; i++)
+  {
+    assert (result[i] == expected_result[i]);
+  }
+
+  result.fini ();
+  expected_result.fini ();
+  free (buffer);
+  free (expected_buffer);
+  free (out_buffer);
+}
+
+
 
 static void test_resolve_overflows_via_isolation_spaces ()
 {
@@ -936,6 +1040,7 @@ main (int argc, char **argv)
   test_resolve_overflows_via_isolation_with_recursive_duplication ();
   test_resolve_overflows_via_isolation_spaces ();
   test_resolve_overflows_via_isolating_16bit_space ();
+  test_resolve_overflows_via_isolating_16bit_space_2 ();
   test_duplicate_leaf ();
   test_duplicate_interior ();
 }