CompatibilityMatrix::combine() combines everything correctly

* Combines <kernel> correctly in order to make assemble_vintf
add <kernel> in any order. That is, we now allow:
assemble_vintf -i base_matrix.xml -i kernel_config_base.xml
    -i kernel_config_x86.xml ...
and <kernel>'s will be added in the correct order even when
the order of kernel_config_base.xml and kernel_config_x86.xml
is swapped.

* Combine <sepolicy> and <avb> correctly. They can only
be defined once per level.

Test: libvintf_test
Test: vintf_object_test
Test: build framework compatibility matrix and inspect output manually

Bug: 78943004

Change-Id: I6c90849b8bef0b690fb292bcc5fa8c514f323d96

8 files changed, 233 insertions, 192 deletions

diff --git a/AssembleVintf.cpp b/AssembleVintf.cpp
index 272301e..cc8db31 100644
--- a/AssembleVintf.cpp
+++ b/AssembleVintf.cpp
@@ -412,6 +412,7 @@ class AssembleVintfImpl : public AssembleVintf {
         return true;
     }
 
+    // Parse --kernel arguments and write to output matrix.
     bool assembleFrameworkCompatibilityMatrixKernels(CompatibilityMatrix* matrix) {
         for (auto& pair : mKernels) {
             std::vector<ConditionedConfig> conditionedConfigs;
@@ -422,7 +423,11 @@ class AssembleVintfImpl : public AssembleVintf {
                 MatrixKernel kernel(KernelVersion{pair.first}, std::move(conditionedConfig.second));
                 if (conditionedConfig.first != nullptr)
                     kernel.mConditions.push_back(std::move(*conditionedConfig.first));
-                matrix->framework.mKernels.push_back(std::move(kernel));
+                std::string error;
+                if (!matrix->addKernel(std::move(kernel), &error)) {
+                    std::cerr << "Error:" << error << std::endl;
+                    return false;
+                };
             }
         }
         return true;
@@ -484,7 +489,24 @@ class AssembleVintfImpl : public AssembleVintf {
         std::string error;
         CompatibilityMatrix* matrix = nullptr;
         std::unique_ptr<HalManifest> checkManifest;
+        std::unique_ptr<CompatibilityMatrix> builtMatrix;
+
+        if (mCheckFile != nullptr) {
+            checkManifest = std::make_unique<HalManifest>();
+            if (!gHalManifestConverter(checkManifest.get(), read(*mCheckFile), &error)) {
+                std::cerr << "Cannot parse check file as a HAL manifest: " << error << std::endl;
+                return false;
+            }
+        }
+
         if (matrices->front().object.mType == SchemaType::DEVICE) {
+            if (matrices->size() > 1) {
+                std::cerr
+                    << "assemble_vintf does not support merging device compatibilility matrices."
+                    << std::endl;
+                return false;
+            }
+
             matrix = &matrices->front().object;
 
             auto vndkVersion = base::Trim(getEnv("REQUIRED_VNDK_VERSION"));
@@ -506,24 +528,20 @@ class AssembleVintfImpl : public AssembleVintf {
         }
 
         if (matrices->front().object.mType == SchemaType::FRAMEWORK) {
-            Level deviceLevel = Level::UNSPECIFIED;
-            if (mCheckFile != nullptr) {
-                checkManifest = std::make_unique<HalManifest>();
-                if (!gHalManifestConverter(checkManifest.get(), read(*mCheckFile), &error)) {
-                    std::cerr << "Cannot parse check file as a HAL manifest: " << error
-                              << std::endl;
-                    return false;
-                }
-                deviceLevel = checkManifest->level();
-            }
-
+            Level deviceLevel =
+                checkManifest != nullptr ? checkManifest->level() : Level::UNSPECIFIED;
             if (deviceLevel == Level::UNSPECIFIED) {
-                // For GSI build, legacy devices that do not have a HAL manifest,
-                // and devices in development, merge all compatibility matrices.
                 deviceLevel = getLowestFcmVersion(*matrices);
+                if (checkManifest != nullptr && deviceLevel != Level::UNSPECIFIED) {
+                    std::cerr << "Warning: No Target FCM Version for device. Assuming \""
+                              << to_string(deviceLevel)
+                              << "\" when building final framework compatibility matrix."
+                              << std::endl;
+                }
             }
+            builtMatrix = CompatibilityMatrix::combine(deviceLevel, matrices, &error);
+            matrix = builtMatrix.get();
 
-            matrix = CompatibilityMatrix::combine(deviceLevel, matrices, &error);
             if (matrix == nullptr) {
                 std::cerr << error << std::endl;
                 return false;
@@ -558,9 +576,9 @@ class AssembleVintfImpl : public AssembleVintf {
             }
 
             getFlagIfUnset("POLICYVERS", &matrix->framework.mSepolicy.mKernelSepolicyVersion,
-                           deviceLevel == Level::UNSPECIFIED /* log */);
+                           false /* log */);
             getFlagIfUnset("FRAMEWORK_VBMETA_VERSION", &matrix->framework.mAvbMetaVersion,
-                           deviceLevel == Level::UNSPECIFIED /* log */);
+                           false /* log */);
             // Hard-override existing AVB version
             getFlag("FRAMEWORK_VBMETA_VERSION_OVERRIDE", &matrix->framework.mAvbMetaVersion,
                     false /* log */);
diff --git a/CompatibilityMatrix.cpp b/CompatibilityMatrix.cpp
index 6a5645c..290ebf9 100644
--- a/CompatibilityMatrix.cpp
+++ b/CompatibilityMatrix.cpp
@@ -19,6 +19,8 @@
 #include <iostream>
 #include <utility>
 
+#include <android-base/strings.h>
+
 #include "parse_string.h"
 #include "parse_xml.h"
 #include "utils.h"
@@ -26,14 +28,59 @@
 namespace android {
 namespace vintf {
 
-bool CompatibilityMatrix::add(MatrixHal &&hal) {
-    return HalGroup<MatrixHal>::add(std::move(hal));
-}
-
-bool CompatibilityMatrix::add(MatrixKernel &&kernel) {
+bool CompatibilityMatrix::addKernel(MatrixKernel&& kernel, std::string* error) {
     if (mType != SchemaType::FRAMEWORK) {
+        if (error) {
+            *error = "Cannot add <kernel> to a " + to_string(mType) + " compatibility matrix.";
+        }
         return false;
     }
+
+    auto it = framework.mKernels.begin();
+    for (; it != framework.mKernels.end(); ++it) {
+        if (it->minLts() == kernel.minLts()) {
+            break;
+        }
+        if (it->minLts().version == kernel.minLts().version &&
+            it->minLts().majorRev == kernel.minLts().majorRev) {
+            if (error) {
+                *error = "Kernel version mismatch; cannot add " + to_string(kernel.minLts()) +
+                         " because " + to_string(it->minLts()) + " was added.";
+            }
+            return false;
+        }
+    }
+
+    bool seenVersion = it != framework.mKernels.end();
+
+    if (seenVersion) {
+        // If no conditions, must be the first among the same minLts
+        // because O libvintf only checks the first <kernel> tag that version matches.
+        if (kernel.conditions().empty()) {
+            // Found first <kernel> with the same minLts.
+            // Append config if it does not have <condition>s, else error.
+            if (it->conditions().empty()) {
+                const auto& configs = kernel.configs();
+                it->mConfigs.insert(it->mConfigs.end(), configs.begin(), configs.end());
+            } else {
+                if (error) {
+                    *error =
+                        "Base compatibility matrix has <condition> for the first <kernel> "
+                        "with minlts " +
+                        to_string(kernel.minLts()) + " for unknown reason.";
+                }
+                return false;
+            }
+            return true;
+        }
+    } else {
+        // First <kernel> of a minLts must not have <condition>'s for backwards compatibility
+        // with O libvintf.
+        if (!kernel.conditions().empty()) {
+            framework.mKernels.push_back(MatrixKernel(KernelVersion{kernel.minLts()}, {}));
+        }
+    }
+
     framework.mKernels.push_back(std::move(kernel));
     return true;
 }
@@ -196,6 +243,27 @@ bool CompatibilityMatrix::addAllXmlFilesAsOptional(CompatibilityMatrix* other, s
     return true;
 }
 
+// Merge Kernel.
+// Add <kernel> from exact "level", then optionally add <kernel> from high levels to low levels.
+// For example, (each letter is a kernel version x.y.z)
+// 1.xml: A1, B1
+// 2.xml: B2, C2, D2
+// 3.xml: D3, E3
+// Then the combined 1.xml should have
+// A1, B1 (from 1.xml, required), C2, D2, E3 (optional, use earliest possible).
+bool CompatibilityMatrix::addAllKernels(CompatibilityMatrix* other, std::string* error) {
+    for (MatrixKernel& kernel : other->framework.mKernels) {
+        KernelVersion ver = kernel.minLts();
+        if (!addKernel(std::move(kernel), error)) {
+            if (error) {
+                *error = "Cannot add kernel version " + to_string(ver) + ": " + *error;
+            }
+            return false;
+        }
+    }
+    return true;
+}
+
 bool CompatibilityMatrix::addAllKernelsAsOptional(CompatibilityMatrix* other, std::string* error) {
     if (other == nullptr || other->level() <= level()) {
         return true;
@@ -216,9 +284,9 @@ bool CompatibilityMatrix::addAllKernelsAsOptional(CompatibilityMatrix* other, st
         }
 
         KernelVersion minLts = kernelToAdd.minLts();
-        if (!add(std::move(kernelToAdd))) {
+        if (!addKernel(std::move(kernelToAdd), error)) {
             if (error) {
-                *error = "Cannot add " + to_string(minLts) + " for unknown reason.";
+                *error = "Cannot add " + to_string(minLts) + ": " + *error;
             }
             return false;
         }
@@ -226,6 +294,34 @@ bool CompatibilityMatrix::addAllKernelsAsOptional(CompatibilityMatrix* other, st
     return true;
 }
 
+template <typename T>
+static bool mergeField(T* dst, T* src) {
+    static const T kEmpty{};
+    if (*dst == *src) {
+        return true;  // no conflict
+    }
+    if (*src == kEmpty) {
+        return true;
+    }
+    if (*dst == kEmpty) {
+        *dst = std::move(*src);
+        return true;
+    }
+    return false;
+}
+
+bool CompatibilityMatrix::addSepolicy(CompatibilityMatrix* other, std::string* error) {
+    bool success = mergeField(&this->framework.mSepolicy, &other->framework.mSepolicy);
+    if (!success && error) *error = "<sepolicy> is already defined";
+    return success;
+}
+
+bool CompatibilityMatrix::addAvbMetaVersion(CompatibilityMatrix* other, std::string* error) {
+    bool success = mergeField(&this->framework.mAvbMetaVersion, &other->framework.mAvbMetaVersion);
+    if (!success && error) *error = "<avb><vbmeta-version> is already defined";
+    return success;
+}
+
 bool operator==(const CompatibilityMatrix &lft, const CompatibilityMatrix &rgt) {
     return lft.mType == rgt.mType && lft.mLevel == rgt.mLevel && lft.mHals == rgt.mHals &&
            lft.mXmlFiles == rgt.mXmlFiles &&
@@ -243,175 +339,83 @@ bool operator==(const CompatibilityMatrix &lft, const CompatibilityMatrix &rgt)
              lft.framework.mAvbMetaVersion == rgt.framework.mAvbMetaVersion));
 }
 
-// Find compatibility_matrix.empty.xml (which has unspecified level) and use it
-// as a base matrix.
-CompatibilityMatrix* CompatibilityMatrix::findOrInsertBaseMatrix(
-    std::vector<Named<CompatibilityMatrix>>* matrices, std::string* error) {
-    std::vector<CompatibilityMatrix*> matricesUnspecified;
-    std::vector<CompatibilityMatrix*> matricesEmpty;
-    for (auto& e : *matrices) {
-        if (e.object.level() == Level::UNSPECIFIED) {
-            matricesUnspecified.push_back(&e.object);
-
-            if (!e.object.mHals.empty()) {
-                continue;
-            }
-
-            if (!e.object.mXmlFiles.empty()) {
-                continue;
-            }
-
-            matricesEmpty.push_back(&e.object);
-        }
-    }
-
-    if (matricesEmpty.size() > 1) {
-        if (error) {
-            *error =
-                "Error: multiple framework compatibility matrix files have "
-                "unspecified level; there should only be one such file.\n";
-            for (auto& e : *matrices) {
-                if (e.object.level() == Level::UNSPECIFIED) {
-                    *error += "    " + e.name + "\n";
-                }
-            }
-        }
-        return nullptr;
-    }
-    if (matricesEmpty.size() == 1) {
-        return matricesEmpty.front();
-    }
-    if (!matricesUnspecified.empty()) {
-        return matricesUnspecified.front();
-    }
-    auto matrix = &matrices->emplace(matrices->end())->object;
-    matrix->mType = SchemaType::FRAMEWORK;
-    matrix->mLevel = Level::UNSPECIFIED;
-    return matrix;
-}
-
-// Check if there are two files declaring level="1", for example, because
-// combine() use this assumption to simplify a lot of logic.
-static bool checkDuplicateLevels(const std::vector<Named<CompatibilityMatrix>>& matrices,
-                                 std::string* error) {
-    std::map<Level, const std::string*> existingLevels;
-    for (const auto& e : matrices) {
-        if (e.object.level() != Level::UNSPECIFIED &&
-            existingLevels.find(e.object.level()) != existingLevels.end()) {
+std::unique_ptr<CompatibilityMatrix> CompatibilityMatrix::combine(
+    Level deviceLevel, std::vector<Named<CompatibilityMatrix>>* matrices, std::string* error) {
+    // Check type.
+    for (const auto& e : *matrices) {
+        if (e.object.type() != SchemaType::FRAMEWORK) {
             if (error) {
-                *error = "Conflict of levels: file \"" +
-                         *existingLevels.find(e.object.level())->second + "\" and \"" + e.name +
-                         " both have level " + to_string(e.object.level());
+                *error = "File \"" + e.name + "\" is not a framework compatibility matrix.";
+                return nullptr;
             }
-            return false;
         }
-        existingLevels.emplace(e.object.level(), &e.name);
     }
-    return true;
-}
 
-CompatibilityMatrix* CompatibilityMatrix::combine(Level deviceLevel,
-                                                  std::vector<Named<CompatibilityMatrix>>* matrices,
-                                                  std::string* error) {
-    if (!checkDuplicateLevels(*matrices, error)) {
-        return nullptr;
+    // Matrices with unspecified (empty) level are auto-filled with deviceLevel.
+    for (auto& e : *matrices) {
+        if (e.object.level() == Level::UNSPECIFIED) {
+            e.object.mLevel = deviceLevel;
+        }
     }
 
-    CompatibilityMatrix* matrix = findOrInsertBaseMatrix(matrices, error);
-    if (matrix == nullptr) {
-        return nullptr;
-    }
+    // Add from low to high FCM version so that optional <kernel> requirements are added correctly.
+    // See comment in addAllAsOptional.
+    std::sort(matrices->begin(), matrices->end(),
+              [](const auto& x, const auto& y) { return x.object.level() < y.object.level(); });
 
-    matrix->mLevel = deviceLevel;
+    auto baseMatrix = std::make_unique<CompatibilityMatrix>();
+    baseMatrix->mLevel = deviceLevel;
+    baseMatrix->mType = SchemaType::FRAMEWORK;
 
+    std::vector<std::string> parsedFiles;
     for (auto& e : *matrices) {
-        if (&e.object != matrix &&
-            (e.object.level() == deviceLevel || e.object.level() == Level::UNSPECIFIED)) {
-            if (!matrix->addAllHals(&e.object, error)) {
-                if (error) {
-                    *error = "File \"" + e.name + "\" cannot be added: HAL " + *error +
-                             " has a conflict.";
-                }
-                return nullptr;
-            }
-
-            if (!matrix->addAllXmlFiles(&e.object, error)) {
-                if (error) {
-                    *error = "File \"" + e.name + "\" cannot be added: XML File entry " + *error +
-                             " has a conflict.";
-                }
-                return nullptr;
-            }
+        if (e.object.level() < deviceLevel) {
+            continue;
         }
-    }
-
-    for (auto& e : *matrices) {
-        if (&e.object != matrix && e.object.level() != Level::UNSPECIFIED &&
-            e.object.level() > deviceLevel) {
-            if (!matrix->addAllHalsAsOptional(&e.object, error)) {
-                if (error) {
-                    *error = "File \"" + e.name + "\" cannot be added: " + *error +
-                             ". See <hal> with the same name " +
-                             "in previously parsed files or previously declared in this file.";
-                }
-                return nullptr;
-            }
 
-            if (!matrix->addAllXmlFilesAsOptional(&e.object, error)) {
-                if (error) {
-                    *error = "File \"" + e.name + "\" cannot be added: XML File entry " + *error +
-                             " has a conflict.";
-                }
-                return nullptr;
-            }
+        bool success = false;
+        if (e.object.level() == deviceLevel) {
+            success = baseMatrix->addAll(&e, error);
+        } else {
+            success = baseMatrix->addAllAsOptional(&e, error);
         }
-    }
-
-    // Add <kernel> from exact "level", then optionally add <kernel> from high levels to low levels.
-    // For example, (each letter is a kernel version x.y.z)
-    // 1.xml: A1, B1
-    // 2.xml: B2, C2, D2
-    // 3.xml: D3, E3
-    // Then the combined 1.xml should have
-    // A1, B1 (from 1.xml, required), C2, D2, E3 (optional, use earliest possible).
-    for (auto& e : *matrices) {
-        if (&e.object != matrix && e.object.level() == deviceLevel &&
-            e.object.type() == SchemaType::FRAMEWORK) {
-            for (MatrixKernel& kernel : e.object.framework.mKernels) {
-                KernelVersion ver = kernel.minLts();
-                if (!matrix->add(std::move(kernel))) {
-                    if (error) {
-                        *error = "Cannot add kernel version " + to_string(ver) +
-                                 " from FCM version " + to_string(deviceLevel);
-                    }
-                    return nullptr;
-                }
+        if (!success) {
+            if (error) {
+                *error = "Conflict when merging \"" + e.name + "\": " + *error + "\n" +
+                         "Previous files:\n" + base::Join(parsedFiles, "\n");
             }
+            return nullptr;
         }
+        parsedFiles.push_back(e.name);
     }
 
-    // There is only one file per level, hence a map is used instead of a multimap. Also, there is
-    // no good ordering (i.e. priority) for multiple files with the same level.
-    std::map<Level, Named<CompatibilityMatrix>*> matricesMap;
-    for (auto& e : *matrices) {
-        if (&e.object != matrix && e.object.level() != Level::UNSPECIFIED &&
-            e.object.level() > deviceLevel && e.object.type() == SchemaType::FRAMEWORK) {
-            matricesMap.emplace(e.object.level(), &e);
+    return baseMatrix;
+}
+
+bool CompatibilityMatrix::addAll(Named<CompatibilityMatrix>* inputMatrix, std::string* error) {
+    if (!addAllHals(&inputMatrix->object, error) || !addAllXmlFiles(&inputMatrix->object, error) ||
+        !addAllKernels(&inputMatrix->object, error) || !addSepolicy(&inputMatrix->object, error) ||
+        !addAvbMetaVersion(&inputMatrix->object, error)) {
+        if (error) {
+            *error = "File \"" + inputMatrix->name + "\" cannot be added: " + *error + ".";
         }
     }
+    return true;
+}
 
-    for (auto&& pair : matricesMap) {
-        if (!matrix->addAllKernelsAsOptional(&pair.second->object, error)) {
-            if (error) {
-                *error = "Cannot add new kernel versions from FCM version " +
-                         to_string(pair.first) + " (" + pair.second->name + ")" +
-                         " to FCM version " + to_string(deviceLevel) + ": " + *error;
-            }
-            return nullptr;
+bool CompatibilityMatrix::addAllAsOptional(Named<CompatibilityMatrix>* inputMatrix,
+                                           std::string* error) {
+    if (!addAllHalsAsOptional(&inputMatrix->object, error) ||
+        !addAllXmlFilesAsOptional(&inputMatrix->object, error) ||
+        !addAllKernelsAsOptional(&inputMatrix->object, error)) {
+        if (error) {
+            *error = "File \"" + inputMatrix->name + "\" cannot be added: " + *error;
         }
+        return false;
     }
-
-    return matrix;
+    // ignore <sepolicy> requirement from higher level
+    // ignore <avb> requirement from higher level
+    return true;
 }
 
 bool CompatibilityMatrix::forEachInstanceOfVersion(
diff --git a/VintfObject.cpp b/VintfObject.cpp
index 1086590..aed9745 100644
--- a/VintfObject.cpp
+++ b/VintfObject.cpp
@@ -196,8 +196,7 @@ status_t VintfObject::getCombinedFrameworkMatrix(
         return NAME_NOT_FOUND;
     }
 
-    CompatibilityMatrix* combined =
-        CompatibilityMatrix::combine(deviceLevel, &matrixFragments, error);
+    auto combined = CompatibilityMatrix::combine(deviceLevel, &matrixFragments, error);
     if (combined == nullptr) {
         return BAD_VALUE;
     }
diff --git a/include/vintf/CompatibilityMatrix.h b/include/vintf/CompatibilityMatrix.h
index 512d278..feb8ac8 100644
--- a/include/vintf/CompatibilityMatrix.h
+++ b/include/vintf/CompatibilityMatrix.h
@@ -18,6 +18,7 @@
 #define ANDROID_VINTF_COMPATIBILITY_MATRIX_H
 
 #include <map>
+#include <memory>
 #include <string>
 
 #include <utils/Errors.h>
@@ -67,8 +68,23 @@ struct CompatibilityMatrix : public HalGroup<MatrixHal>, public XmlFileGroup<Mat
     std::string getVendorNdkVersion() const;
 
    private:
-    bool add(MatrixHal &&hal);
-    bool add(MatrixKernel &&kernel);
+    // Add everything in inputMatrix to "this" as requirements.
+    bool addAll(Named<CompatibilityMatrix>* inputMatrix, std::string* error);
+
+    // Add all <kernel> from other to "this". Error if there is a conflict.
+    bool addAllKernels(CompatibilityMatrix* other, std::string* error);
+
+    // Add a <kernel> tag to "this". Error if there is a conflict.
+    bool addKernel(MatrixKernel&& kernel, std::string* error);
+
+    // Merge <sepolicy> with other's <sepolicy>. Error if there is a conflict.
+    bool addSepolicy(CompatibilityMatrix* other, std::string* error);
+
+    // Merge <avb><vbmeta-version> with other's <avb><vbmeta-version>. Error if there is a conflict.
+    bool addAvbMetaVersion(CompatibilityMatrix* other, std::string* error);
+
+    // Add everything in inputMatrix to "this" as optional.
+    bool addAllAsOptional(Named<CompatibilityMatrix>* inputMatrix, std::string* error);
 
     // Add all HALs as optional HALs from "other". This function moves MatrixHal objects
     // from "other".
@@ -81,23 +97,23 @@ struct CompatibilityMatrix : public HalGroup<MatrixHal>, public XmlFileGroup<Mat
     // Similar to addAllHalsAsOptional but on <kernel> entries.
     bool addAllKernelsAsOptional(CompatibilityMatrix* other, std::string* error);
 
+    // Combine a set of framework compatibility matrices. For each CompatibilityMatrix in matrices
+    // (in the order of level(), where UNSPECIFIED (empty) is treated as deviceLevel)
+    // - If level() < deviceLevel, ignore
+    // - If level() == UNSPECIFIED or level() == deviceLevel,
+    //   - Add as hard requirements. See combineSameFcmVersion
+    // - If level() > deviceLevel,
+    //   - all <hal> versions and <xmlfile>s are added as optional.
+    //   - <kernel minlts="x.y.z"> is added only if x.y does not exist in a file
+    //     with lower level()
+    //   - <sepolicy>, <avb><vbmeta-version> is ignored
+    // Return the combined matrix, nullptr if any error (e.g. conflict of information).
+    static std::unique_ptr<CompatibilityMatrix> combine(
+        Level deviceLevel, std::vector<Named<CompatibilityMatrix>>* matrices, std::string* error);
+
     status_t fetchAllInformation(const FileSystem* fileSystem, const std::string& path,
                                  std::string* error = nullptr);
 
-    // Combine a subset of "matrices". For each CompatibilityMatrix in matrices,
-    // - If level() == UNSPECIFIED, use it as the base matrix (for non-HAL, non-XML-file
-    //   requirements).
-    // - If level() < deviceLevel, ignore
-    // - If level() == deviceLevel, all HAL versions and XML files are added as is
-    //   (optionality is kept)
-    // - If level() > deviceLevel, all HAL versions and XML files are added as optional.
-    // Return a pointer into one of the elements in "matrices".
-    static CompatibilityMatrix* combine(Level deviceLevel,
-                                        std::vector<Named<CompatibilityMatrix>>* matrices,
-                                        std::string* error);
-    static CompatibilityMatrix* findOrInsertBaseMatrix(
-        std::vector<Named<CompatibilityMatrix>>* matrices, std::string* error);
-
     MatrixHal* splitInstance(MatrixHal* existingHal, const std::string& interface,
                              const std::string& instance, bool isRegex);
 
diff --git a/include/vintf/HalGroup.h b/include/vintf/HalGroup.h
index a0a14cd..5cad867 100644
--- a/include/vintf/HalGroup.h
+++ b/include/vintf/HalGroup.h
@@ -39,7 +39,7 @@ struct HalGroup {
         for (auto& pair : other->mHals) {
             if (!add(std::move(pair.second))) {
                 if (error) {
-                    *error = pair.first;
+                    *error = "HAL \"" + pair.first + "\" has a conflict.";
                 }
                 return false;
             }
diff --git a/include/vintf/MatrixKernel.h b/include/vintf/MatrixKernel.h
index cee7e32..14cf5bf 100644
--- a/include/vintf/MatrixKernel.h
+++ b/include/vintf/MatrixKernel.h
@@ -58,6 +58,7 @@ struct MatrixKernel {
    private:
     friend struct MatrixKernelConverter;
     friend struct MatrixKernelConditionsConverter;
+    friend struct CompatibilityMatrix;
     friend class AssembleVintfImpl;
 
     KernelVersion mMinLts;
diff --git a/include/vintf/XmlFileGroup.h b/include/vintf/XmlFileGroup.h
index 524efc9..dd15647 100644
--- a/include/vintf/XmlFileGroup.h
+++ b/include/vintf/XmlFileGroup.h
@@ -62,7 +62,7 @@ struct XmlFileGroup {
         for (auto& pair : other->mXmlFiles) {
             if (!addXmlFile(std::move(pair.second))) {
                 if (error) {
-                    *error = pair.first;
+                    *error = "XML File \"" + pair.first + "\" has a conflict.";
                 }
                 return false;
             }
diff --git a/test/LibVintfTest.cpp b/test/LibVintfTest.cpp
index 68af9bb..a3a98b8 100644
--- a/test/LibVintfTest.cpp
+++ b/test/LibVintfTest.cpp
@@ -59,7 +59,10 @@ public:
         return cm.add(std::move(hal));
     }
     bool add(CompatibilityMatrix &cm, MatrixKernel &&kernel) {
-        return cm.add(std::move(kernel));
+        std::string error;
+        bool success = cm.addKernel(std::move(kernel), &error);
+        EXPECT_EQ(success, error == "") << "success: " << success << ", error: " << error;
+        return success;
     }
     bool add(HalManifest &vm, ManifestHal &&hal) {
         return vm.add(std::move(hal));