Merge "Diff the fields located at the same offset in a class"

2023-02-15 08:42:25 +00:00
parent b1785dc0a8 08e0ce1066
commit 28f992d407
8 changed files with 347 additions and 51 deletions
--- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp
+++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp
@@ -390,16 +390,75 @@ AbiDiffHelper::CompareCommonRecordFields(
  return std::make_pair(field_diff_status, nullptr);
 }

+// This function filters out the pairs of old and new fields that meet the
+// following conditions:
+//   The old field's (offset, type) is unique in old_fields.
+//   The new field's (offset, type) is unique in new_fields.
+//   The two fields have compatible attributes except the name.
+void AbiDiffHelper::FilterOutRenamedRecordFields(
+    std::deque<std::string> *type_queue, DiffMessageIR::DiffKind diff_kind,
+    std::vector<const RecordFieldIR *> &old_fields,
+    std::vector<const RecordFieldIR *> &new_fields) {
+  const auto old_end = old_fields.end();
+  const auto new_end = new_fields.end();
+  auto is_less = [](const RecordFieldIR *first, const RecordFieldIR *second) {
+    if (first->GetOffset() != second->GetOffset()) {
+      return first->GetOffset() < second->GetOffset();
+    }
+    return first->GetReferencedType() < second->GetReferencedType();
+  };
+  std::sort(old_fields.begin(), old_end, is_less);
+  std::sort(new_fields.begin(), new_end, is_less);
+
+  std::vector<const RecordFieldIR *> out_old_fields;
+  std::vector<const RecordFieldIR *> out_new_fields;
+  auto old_it = old_fields.begin();
+  auto new_it = new_fields.begin();
+  while (old_it != old_end && new_it != new_end) {
+    auto next_old_it = std::next(old_it);
+    while (next_old_it != old_end && !is_less(*old_it, *next_old_it)) {
+      next_old_it++;
+    }
+    if (is_less(*old_it, *new_it) || next_old_it - old_it > 1) {
+      out_old_fields.insert(out_old_fields.end(), old_it, next_old_it);
+      old_it = next_old_it;
+      continue;
+    }
+
+    auto next_new_it = std::next(new_it);
+    while (next_new_it != new_end && !is_less(*new_it, *next_new_it)) {
+      next_new_it++;
+    }
+    if (is_less(*new_it, *old_it) || next_new_it - new_it > 1) {
+      out_new_fields.insert(out_new_fields.end(), new_it, next_new_it);
+      new_it = next_new_it;
+      continue;
+    }
+
+    auto comparison_result =
+        CompareCommonRecordFields(*old_it, *new_it, type_queue, diff_kind);
+    if (comparison_result.second != nullptr) {
+      out_old_fields.emplace_back(*old_it);
+      out_new_fields.emplace_back(*new_it);
+    }
+    old_it = next_old_it;
+    new_it = next_new_it;
+  }
+  out_old_fields.insert(out_old_fields.end(), old_it, old_end);
+  out_new_fields.insert(out_new_fields.end(), new_it, new_end);
+
+  old_fields = std::move(out_old_fields);
+  new_fields = std::move(out_new_fields);
+}
+
 RecordFieldDiffResult AbiDiffHelper::CompareRecordFields(
    const std::vector<RecordFieldIR> &old_fields,
    const std::vector<RecordFieldIR> &new_fields,
    std::deque<std::string> *type_queue, DiffMessageIR::DiffKind diff_kind) {
  RecordFieldDiffResult result;
+  // Map names to RecordFieldIR.
  AbiElementMap<const RecordFieldIR *> old_fields_map;
  AbiElementMap<const RecordFieldIR *> new_fields_map;
-  std::map<uint64_t, const RecordFieldIR *> old_fields_offset_map;
-  std::map<uint64_t, const RecordFieldIR *> new_fields_offset_map;
-
  utils::AddToMap(
      &old_fields_map, old_fields,
      [](const RecordFieldIR *f) {return f->GetName();},
@@ -408,55 +467,14 @@ RecordFieldDiffResult AbiDiffHelper::CompareRecordFields(
      &new_fields_map, new_fields,
      [](const RecordFieldIR *f) {return f->GetName();},
      [](const RecordFieldIR *f) {return f;});
-  utils::AddToMap(
-      &old_fields_offset_map, old_fields,
-      [](const RecordFieldIR *f) {return f->GetOffset();},
-      [](const RecordFieldIR *f) {return f;});
-  utils::AddToMap(
-      &new_fields_offset_map, new_fields,
-      [](const RecordFieldIR *f) {return f->GetOffset();},
-      [](const RecordFieldIR *f) {return f;});
-  // If a field is removed from the map field_name -> offset see if another
-  // field is present at the same offset and compare the size and type etc,
-  // remove it from the removed fields if they're compatible.
-  std::vector<const RecordFieldIR *> removed_fields =
+  // Compare the fields whose names are not present in both records.
+  result.removed_fields =
      utils::FindRemovedElements(old_fields_map, new_fields_map);
-
-  std::vector<const RecordFieldIR *> added_fields =
+  result.added_fields =
      utils::FindRemovedElements(new_fields_map, old_fields_map);
-
-  auto predicate =
-      [&](const RecordFieldIR *removed_field,
-          std::map<uint64_t, const RecordFieldIR *> &field_off_map) {
-        uint64_t old_field_offset = removed_field->GetOffset();
-        auto corresponding_field_at_same_offset =
-            field_off_map.find(old_field_offset);
-        // Correctly reported as removed, so do not remove.
-        if (corresponding_field_at_same_offset == field_off_map.end()) {
-          return false;
-        }
-
-        auto comparison_result = CompareCommonRecordFields(
-            removed_field, corresponding_field_at_same_offset->second,
-            type_queue, diff_kind);
-        // No actual diff, so remove it.
-        return (comparison_result.second == nullptr);
-      };
-
-  removed_fields.erase(
-      std::remove_if(
-          removed_fields.begin(), removed_fields.end(),
-          std::bind(predicate, std::placeholders::_1, new_fields_offset_map)),
-      removed_fields.end());
-  added_fields.erase(
-      std::remove_if(
-          added_fields.begin(), added_fields.end(),
-          std::bind(predicate, std::placeholders::_1, old_fields_offset_map)),
-      added_fields.end());
-
-  result.removed_fields = std::move(removed_fields);
-  result.added_fields = std::move(added_fields);
-
+  FilterOutRenamedRecordFields(type_queue, diff_kind, result.removed_fields,
+                               result.added_fields);
+  // Compare the fields whose names are present in both records.
  std::vector<std::pair<
      const RecordFieldIR *, const RecordFieldIR *>> cf =
      utils::FindCommonElements(old_fields_map, new_fields_map);
@@ -472,7 +490,7 @@ RecordFieldDiffResult AbiDiffHelper::CompareRecordFields(
          std::move(*(diffed_field_ptr.second.release())));
    }
  }
-
+  // Determine DiffStatus.
  DiffStatus &diff_status = result.status;
  diff_status = DiffStatus::kNoDiff;
  if (result.diffed_fields.size() != 0 || result.removed_fields.size() != 0) {
--- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.h
+++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.h
@@ -188,6 +188,11 @@ class AbiDiffHelper {
      std::deque<std::string> *type_queue,
      IRDiffDumper::DiffKind diff_kind);

+  void FilterOutRenamedRecordFields(
+      std::deque<std::string> *type_queue, DiffMessageIR::DiffKind diff_kind,
+      std::vector<const RecordFieldIR *> &old_fields,
+      std::vector<const RecordFieldIR *> &new_fields);
+
  RecordFieldDiffResult CompareRecordFields(
      const std::vector<RecordFieldIR> &old_fields,
      const std::vector<RecordFieldIR> &new_fields,
--- a/vndk/tools/header-checker/tests/integration/union/include/base.h
+++ b/vndk/tools/header-checker/tests/integration/union/include/base.h
@@ -0,0 +1,26 @@
+union ChangeType {
+  char member_1;
+  char member_2;
+  int member_3;
+};
+
+union Rename {
+  int member_1;
+  char member_2;
+};
+
+union Swap {
+  int member_1;
+  char member_2;
+};
+
+struct ChangeTypeInStruct {
+  int member_1;
+  char member_2[0];
+  char member_3[0];
+  int member_4[0];
+};
+
+extern "C" {
+void function(ChangeType, Rename, Swap, ChangeTypeInStruct);
+}
--- a/vndk/tools/header-checker/tests/integration/union/include/diff.h
+++ b/vndk/tools/header-checker/tests/integration/union/include/diff.h
@@ -0,0 +1,26 @@
+union ChangeType {
+  char member_1;
+  int member_2;
+  int member_3;
+};
+
+union Rename {
+  int rename_1;
+  char rename_2;
+};
+
+union Swap {
+  char member_2;
+  int rename_1;
+};
+
+struct ChangeTypeInStruct {
+  int member_1;
+  char member_2[0];
+  int member_3[0];
+  int member_4[0];
+};
+
+extern "C" {
+void function(ChangeType, Rename, Swap, ChangeTypeInStruct);
+}
--- a/vndk/tools/header-checker/tests/integration/union/map.txt
+++ b/vndk/tools/header-checker/tests/integration/union/map.txt
@@ -0,0 +1,5 @@
+libunion {
+  global:
+    function;
+};
+
--- a/vndk/tools/header-checker/tests/module.py
+++ b/vndk/tools/header-checker/tests/module.py
@@ -772,6 +772,23 @@ TEST_MODULES = [
        linker_flags=['-output-format', 'Json'],
        has_reference_dump=True,
    ),
+    LsdumpModule(
+        name='libunion',
+        arch='arm64',
+        srcs=['integration/union/include/base.h'],
+        version_script='integration/union/map.txt',
+        export_include_dirs=['integration/union/include'],
+        linker_flags=['-output-format', 'Json'],
+        has_reference_dump=True,
+    ),
+    LsdumpModule(
+        name='libunion_diff',
+        arch='arm64',
+        srcs=['integration/union/include/diff.h'],
+        version_script='integration/union/map.txt',
+        export_include_dirs=['integration/union/include'],
+        linker_flags=['-output-format', 'Json'],
+    ),
 ]

 TEST_MODULES = {m.name: m for m in TEST_MODULES}
--- a/vndk/tools/header-checker/tests/reference_dumps/arm64/libunion.so.lsdump
+++ b/vndk/tools/header-checker/tests/reference_dumps/arm64/libunion.so.lsdump
@@ -0,0 +1,188 @@
+{
+ "array_types" :
+ [
+  {
+   "alignment" : 1,
+   "linker_set_key" : "_ZTIA0_c",
+   "name" : "char[0]",
+   "referenced_type" : "_ZTIc",
+   "self_type" : "_ZTIA0_c",
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  },
+  {
+   "alignment" : 4,
+   "linker_set_key" : "_ZTIA0_i",
+   "name" : "int[0]",
+   "referenced_type" : "_ZTIi",
+   "self_type" : "_ZTIA0_i",
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  }
+ ],
+ "builtin_types" :
+ [
+  {
+   "alignment" : 1,
+   "is_integral" : true,
+   "is_unsigned" : true,
+   "linker_set_key" : "_ZTIc",
+   "name" : "char",
+   "referenced_type" : "_ZTIc",
+   "self_type" : "_ZTIc",
+   "size" : 1
+  },
+  {
+   "alignment" : 4,
+   "is_integral" : true,
+   "linker_set_key" : "_ZTIi",
+   "name" : "int",
+   "referenced_type" : "_ZTIi",
+   "self_type" : "_ZTIi",
+   "size" : 4
+  },
+  {
+   "linker_set_key" : "_ZTIv",
+   "name" : "void",
+   "referenced_type" : "_ZTIv",
+   "self_type" : "_ZTIv"
+  }
+ ],
+ "elf_functions" :
+ [
+  {
+   "name" : "function"
+  }
+ ],
+ "elf_objects" : [],
+ "enum_types" : [],
+ "function_types" : [],
+ "functions" :
+ [
+  {
+   "function_name" : "function",
+   "linker_set_key" : "function",
+   "parameters" :
+   [
+    {
+     "referenced_type" : "_ZTI10ChangeType"
+    },
+    {
+     "referenced_type" : "_ZTI6Rename"
+    },
+    {
+     "referenced_type" : "_ZTI4Swap"
+    },
+    {
+     "referenced_type" : "_ZTI18ChangeTypeInStruct"
+    }
+   ],
+   "return_type" : "_ZTIv",
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  }
+ ],
+ "global_vars" : [],
+ "lvalue_reference_types" : [],
+ "pointer_types" : [],
+ "qualified_types" : [],
+ "record_types" :
+ [
+  {
+   "alignment" : 4,
+   "fields" :
+   [
+    {
+     "field_name" : "member_1",
+     "referenced_type" : "_ZTIc"
+    },
+    {
+     "field_name" : "member_2",
+     "referenced_type" : "_ZTIc"
+    },
+    {
+     "field_name" : "member_3",
+     "referenced_type" : "_ZTIi"
+    }
+   ],
+   "linker_set_key" : "_ZTI10ChangeType",
+   "name" : "ChangeType",
+   "record_kind" : "union",
+   "referenced_type" : "_ZTI10ChangeType",
+   "self_type" : "_ZTI10ChangeType",
+   "size" : 4,
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  },
+  {
+   "alignment" : 4,
+   "fields" :
+   [
+    {
+     "field_name" : "member_1",
+     "referenced_type" : "_ZTIi"
+    },
+    {
+     "field_name" : "member_2",
+     "field_offset" : 32,
+     "referenced_type" : "_ZTIA0_c"
+    },
+    {
+     "field_name" : "member_3",
+     "field_offset" : 32,
+     "referenced_type" : "_ZTIA0_c"
+    },
+    {
+     "field_name" : "member_4",
+     "field_offset" : 32,
+     "referenced_type" : "_ZTIA0_i"
+    }
+   ],
+   "linker_set_key" : "_ZTI18ChangeTypeInStruct",
+   "name" : "ChangeTypeInStruct",
+   "referenced_type" : "_ZTI18ChangeTypeInStruct",
+   "self_type" : "_ZTI18ChangeTypeInStruct",
+   "size" : 4,
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  },
+  {
+   "alignment" : 4,
+   "fields" :
+   [
+    {
+     "field_name" : "member_1",
+     "referenced_type" : "_ZTIi"
+    },
+    {
+     "field_name" : "member_2",
+     "referenced_type" : "_ZTIc"
+    }
+   ],
+   "linker_set_key" : "_ZTI4Swap",
+   "name" : "Swap",
+   "record_kind" : "union",
+   "referenced_type" : "_ZTI4Swap",
+   "self_type" : "_ZTI4Swap",
+   "size" : 4,
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  },
+  {
+   "alignment" : 4,
+   "fields" :
+   [
+    {
+     "field_name" : "member_1",
+     "referenced_type" : "_ZTIi"
+    },
+    {
+     "field_name" : "member_2",
+     "referenced_type" : "_ZTIc"
+    }
+   ],
+   "linker_set_key" : "_ZTI6Rename",
+   "name" : "Rename",
+   "record_kind" : "union",
+   "referenced_type" : "_ZTI6Rename",
+   "self_type" : "_ZTI6Rename",
+   "size" : 4,
+   "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h"
+  }
+ ],
+ "rvalue_reference_types" : []
+}
--- a/vndk/tools/header-checker/tests/test.py
+++ b/vndk/tools/header-checker/tests/test.py
@@ -474,6 +474,17 @@ class HeaderCheckerTest(unittest.TestCase):
            self.assertIn(f'"{type_id}"', diff,
                          f'"{type_id}" should be in the diff report.')

+    def test_union_diff(self):
+        diff = self.prepare_and_run_abi_diff_all_archs(
+            "libunion", "libunion_diff", 8,
+            flags=["-input-format-new", "Json", "-input-format-old", "Json"],
+            create_old=False, create_new=True)
+        self.assertIn('"ChangeType"', diff)
+        self.assertIn('"ChangeTypeInStruct"', diff)
+        self.assertEqual(2, diff.count("fields_diff"))
+        self.assertNotIn("fields_added", diff)
+        self.assertNotIn("fields_removed", diff)
+

 if __name__ == '__main__':
    unittest.main()