diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index 5016625c2..dfe9c44d6 100644 --- a/src/hb-repacker.hh +++ b/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 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 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; diff --git a/src/test-repacker.cc b/src/test-repacker.cc index a143cac98..1cfa487d5 100644 --- a/src/test-repacker.cc +++ b/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 (); + + 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 (); + + 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 (); }