Merge "Diff the fields located at the same offset in a class" am: 28f992d407

Original change: https://android-review.googlesource.com/c/platform/development/+/2390652

Change-Id: If832904123f290dd8ccef1b91bd075fac2cc817c
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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(
+  // Compare the fields whose names are not present in both records.
-      &old_fields_offset_map, old_fields,
+  result.removed_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 =
       utils::FindRemovedElements(old_fields_map, new_fields_map);
-
+  result.added_fields =
-  std::vector<const RecordFieldIR *> added_fields =
       utils::FindRemovedElements(new_fields_map, old_fields_map);
-
+  FilterOutRenamedRecordFields(type_queue, diff_kind, result.removed_fields,
-  auto predicate =
+                               result.added_fields);
-      [&](const RecordFieldIR *removed_field,
+  // Compare the fields whose names are present in both records.
-          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);
-
   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) {

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,

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);
+}

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);
+}

vndk/tools/header-checker/tests/integration/union/map.txt

@@ -0,0 +1,5 @@
+libunion {
+  global:
+    function;
+};
+

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}

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" : []
+}

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()