[ASTImporter] Fix name conflict handling with different strategies

[martong]

Summary

There are numorous flaws about the name conflict handling, this patch
attempts fixes them. Changes in details:

• HandleNameConflict return with a false DeclarationName

Hitherto we effectively never returned with a NameConflict error, even
if the preceding StructuralMatch indicated a conflict.
Because we just simply returned with the parameter Name in
HandleNameConflict and that name is almost always true when converted to
bool.

• Add tests which indicate wrong NameConflict handling

• Add to ConflictingDecls only if decl kind is different

Note, we might not indicate an ODR error when there is an existing record decl
and a enum is imported with same name. But there are other cases. E.g. think
about the case when we import a FunctionTemplateDecl with name f and we found a
simple FunctionDecl with name f. They overload. Or in case of a
ClassTemplateDecl and CXXRecordDecl, the CXXRecordDecl could be the 'templated'
class, so it would be false to report error. So I think we should report a
name conflict error only when we are 100% sure of that. That is why I think it
should be a general pattern to report the error only if the kind is the same.

• Fix failing ctu test with EnumConstandDecl

In ctu-main.c we have the enum class 'A' which brings in the enum
constant 'x' with value 0 into the global namespace.
In ctu-other.c we had the enum class 'B' which brought in the same name
('x') as an enum constant but with a different enum value (42). This is clearly
an ODR violation in the global namespace. The solution was to rename the
second enum constant.

• Introduce ODR handling strategies

[a_sidorin]

Hi Gabor,

Thank you for addressing the problem!

lib/AST/ASTImporter.cpp

2256 ↗

(On Diff #191864)

if body is surrounded by braces, so it's better to surround else too.

2260 ↗

(On Diff #191864)

Do we push the same decl twice?

8532 ↗

(On Diff #191864)

Empty DeclarationName can be valid sometimes. Should we return ErrorOr<DeclarationName> instead? This can also simplify caller code a bit.

[martong]

lib/AST/ASTImporter.cpp

2260 ↗

(On Diff #191864)

Uhh, thanks, that is rebase error I've made.

8532 ↗

(On Diff #191864)

That's a good idea, thanks.
Though, I'd use Expected<T> rather, that would be more consistent with the rest of the code.

[martong]

• Add braces around else
• Remove falsely duplicated push_back
• Use Expected<T> in HandleNameConflict

[a_sidorin]

Thanks for the fixes! I'd like to clarify one moment, however.

lib/AST/ASTImporter.cpp

2154 ↗

(On Diff #192263)

Should we write else Name = *NameOrError?

[martong]

lib/AST/ASTImporter.cpp

2154 ↗

(On Diff #192263)

Atm, we implement the simplest strategy for name conflict handling: we just return with the error.
However, on a long term we should use the returned name indeed. I mean when we really do implement a renaming strategy via HandleNameConflict.

Also, for that we'd have to double check that the Name is indeed used when we create the AST node. So I'd rather leave this else branch up to that point. Hopefully, by that time we'll have unittests which would exercise this else branch, now we just don't have any.

[martong]

• Put back the call to GetOriginalDecl

[martong]

Ping @a_sidorin @shafik

[martong]

Ping

[a_sidorin]

LGTM, thanks!

[martong]

• Rebase to master

[shafik]

First round of review.

clang/lib/AST/ASTImporter.cpp

2699

Name -> SearchName

clang/unittests/AST/ASTImporterTest.cpp

2392

What about tests for name conflicts for:

NamespaceDecl
TypedefNameDecl
TypeAliasTemplateDecl
EnumConstantDecl
RecordDecl
VarDecl

Who were also modified above.

[shafik]

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

[martong]

In D59692#1632138, @shafik wrote:

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

Hey Shafik, I was busy lately with some non-ASTImporter related tasks, but just recently I could drive myself back to it.
I am planning to add the test cases in the following work days, but the latest by the end of the next week.

[martong]

• Name -> SearchName
• Add tests for conflicting declarations

[martong]

clang/unittests/AST/ASTImporterTest.cpp

2392

I added several new tests with a new test suite ConflictingDeclsTest, they cover all the modifications in ASTImporter.cpp except for VarTemplateDecls.
In case of VarTemplateDecls we don't have a proper structural eq check implemented yet, so I disabled that test.

[martong]

@shafik Ping

[martong]

• Use resulting Name from HandleNameConflict if set
• Add ODRHandling strategies
• Refactor tests, to avoid some code repetition

[martong]

@shafik I have updated the patch with ODR handling strategies as per our discusson.

For the record I copy our discussion here:

On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour <[e-mail]> wrote:

Apologies, I missed this earlier!

On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton <[e-mail]> wrote:

Hi Shafik,

Right now you will end up w/ an arbitrary one of them but we do want to support

a way to choose between them eventually.

Actually, right now (if the patch is not merged) we end up having both of them in the AST. Some parts of the AST reference the existing definition, while some other parts reference the new definition. Also, the regular lookup will find both definitions.

If the patch is merged then only the first (the existing) definition is kept and an error is reported.

AFAICT this would prevent such a solution. At least that is how the

new test case for RecordDecl make it appear

Yes, you will never be able to remove an existing node from the AST, so I don't think an either-or choosing mechanism is feasible. But you may be able to decide whether you want to add the new and conflicting node. And you may be able to use a different name for the new conflicting node. This may help clients to see which node they are using.
I want to create an additional patch which builds on this patch. Here is the essence of what I'd like to have:

if (!ConflictingDecls.empty()) {
  Expected<DeclarationName> Resolution = Importer.HandleNameConflict(
      Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
      ConflictingDecls.size());
  if (Resolution)
    Name = Resolution.get();
  else
    return Resolution.takeError();
}

Consider the "else" branch. I'd like to have such an "else" branch everywhere. The point is to use the result of HandleNameConflict (if it is set to something). This way it is possible to create any kind of ODR handling by overwriting HandleNameConflict in the descendant classes.

We identified 3 possible strategies so far:

Conservative. In case of name conflict propagate the error. This should be the default behavior.
Liberal. In case of name conflict create a new node with the same name and do not propagate the error.
Rename. In case of name conflict create a new node with a different name (e.g. with a prefix of the TU's name). Do not propagate the error.

If we add a new conflicting node beside the exiting one, then some clients of the AST which rely on lookup will be confused. The CTU client does not rely on lookup so that is not really a problem there, but I don't know what this could cause with LLDB. Perhaps the renaming strategy could work there too.
The Conservative and Liberal strategies are very easy to implement, and I am going to create patches for that if we have consensus.

We know currently we do have cases where we have ODR violations w/
RecordDecl due to use of an opaque struct in the API headers and a
concrete instance internally e.g.:

//opaque
struct A {

char buf[16];

};

//concrete
struct A {

double d;
int64_t x;

};

and we don't want this to become an error.

I think we would at least one the choice of Conservative or Liberal to
be configurable and maybe LLDB would default to Liberal. This would
enable us to keep the status quo and not break existing cases any
worse than they already are.

I would prefer that would be done in this PR since I don't want to be
in a situation where we branch internally and we have this change but
not the follow-up change.

I don't see how like you comment says this does not effect CXXRecordDecl

In my comment I do not say that CXXRecordDecls are exceptions from the general way of handling ODR violations.
The comment is about that we shall not report ODR errors if we are not 100% sure that we face one.
So we should report an ODR error only if the found Decl and the newly imported Decl have the same kind.
I.e. both are CXXRecordDecls.
For example, let's assume we import a CXXRecordDecl and we find an existing ClassTemplateDecl with the very same Name.
Then we should not report an ODR violation.

Thank you for the clarification, I misunderstood the comment, now it
makes more sense.

Thanks,
Gabor

On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour <[e-mail]> wrote:

Have a nice vacation :-)

On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton <[e-mail]> wrote:

Hi Shafik,

I'll have an answer for you on Wednesday, I'm on vacation until then.

Thanks,
Gábor

On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour, <[e-mail]> wrote:

Hello Gábor,

I was looking at;

https://reviews.llvm.org/D59692

again and I appreciate you added the new tests.

I don't know how I missed this before but we would probably like to
support ODR violations in some reasonable manner. For example we have
one API that is using an opaque struct for the public API but the
private API used a non-opaque definition.

I would have to come up with some sort of test case which is hard to
do, or at least I did not have success last time I tried. Right now
you will end up w/ an arbitrary one of them but we do want to support
a way to choose between them eventually.

AFAICT this would prevent such a solution. At least that is how the
new test case for RecordDecl make it appear and I don't see how like
you comment says this does not effect CXXRecordDecl.

-Shafik

[shafik]

Other than my two comment this LGTM

clang/lib/AST/ASTImporter.cpp

3453

I am a little concerned about this change. I am wondering if this was previously handled differently as a band-aid for some other issues that only showed up in rare cases when they iterated over all the results but not when they errored out on the first.

Could we make the changes to VisitFieldDecl a separate PR so it is easier to revert later on if we do find this causes a regression we are not currently covering?

clang/unittests/AST/ASTImporterTest.cpp

5615

Since the "Liberal" strategy should be the current status quo, I think it might make sense to have a first PR that just has the *LiberalStrategy tests to verify that indeed this is the current behavior as we expect.

[martong]

clang/lib/AST/ASTImporter.cpp

3453

Ok, I reverted these hunks. I am going to create a follow-up patch which will include these changes.

clang/unittests/AST/ASTImporterTest.cpp

5615

Ok, I removed the test suite ConflictingDeclsWithConservativeStrategy. I am going to create a subsequent patch which adds comprehensive testing for both strategies.

[martong]

• Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl
• Remove test suite ConflictingDeclsWithConservativeStrategy

[martong]

• Apply clang-format

[martong]

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

[labath]

In D59692#1646990, @martong wrote:

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

That ought to be fixed by r370057.

clang/include/clang/AST/ASTImporter.h

class ASTImporter {
     friend class ASTNodeImporter;
   public:
     using NonEquivalentDeclSet = llvm::DenseSet<std::pair<Decl *, Decl *>>;
     using ImportedCXXBaseSpecifierMap =
         llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
     using FileIDImportHandlerType =
         std::function<void(FileID /*ToID*/, FileID /*FromID*/)>;
 
+    enum class ODRHandlingType { Conservative, Liberal };
+
     // An ImportPath is the list of the AST nodes which we visit during an
     // Import call.
     // If node `A` depends on node `B` then the path contains an `A`->`B` edge.
     // From the call stack of the import functions we can read the very same
     // path.
     //
     // Now imagine the following AST, where the `->` represents dependency in
     // therms of the import.
private:
     ASTContext &ToContext, &FromContext;
 
     /// The file managers we're importing to and from.
     FileManager &ToFileManager, &FromFileManager;
 
     /// Whether to perform a minimal import.
     bool Minimal;
 
+    ODRHandlingType ODRHandling;
+
     /// Whether the last diagnostic came from the "from" context.
     bool LastDiagFromFrom = false;
 
     /// Mapping from the already-imported types in the "from" context
     /// to the corresponding types in the "to" context.
     llvm::DenseMap<const Type *, const Type *> ImportedTypes;
 
     /// Mapping from the already-imported declarations in the "from"
public:
     void setFileIDImportHandler(FileIDImportHandlerType H) {
       FileIDImportHandler = H;
     }
 
     /// Whether the importer will perform a minimal import, creating
     /// to-be-completed forward declarations when possible.
     bool isMinimalImport() const { return Minimal; }
 
+    void setODRHandling(ODRHandlingType T) { ODRHandling = T; }
+
     /// \brief Import the given object, returns the result.
     ///
     /// \param To Import the object into this variable.
     /// \param From Object to import.
     /// \return Error information (success or error).
     template <typename ImportT>
     LLVM_NODISCARD llvm::Error importInto(ImportT &To, const ImportT &From) {
       auto ToOrErr = Import(From);
public:
     ///
     /// \param IDNS the identifier namespace in which the name will be found.
     ///
     /// \param Decls the set of declarations with the same name as the
     /// declaration being imported.
     ///
     /// \param NumDecls the number of conflicting declarations in \p Decls.
     ///
-    /// \returns the name that the newly-imported declaration should have.
-    virtual DeclarationName HandleNameConflict(DeclarationName Name,
-                                               DeclContext *DC,
-                                               unsigned IDNS,
-                                               NamedDecl **Decls,
-                                               unsigned NumDecls);
+    /// \returns the name that the newly-imported declaration should have. Or
+    /// an error if we can't handle the name conflict.
+    virtual Expected<DeclarationName>
+    HandleNameConflict(DeclarationName Name, DeclContext *DC, unsigned IDNS,
+                       NamedDecl **Decls, unsigned NumDecls);
 
     /// Retrieve the context that AST nodes are being imported into.
     ASTContext &getToContext() const { return ToContext; }
 
     /// Retrieve the context that AST nodes are being imported from.
     ASTContext &getFromContext() const { return FromContext; }
 
     /// Retrieve the file manager that AST nodes are being imported into.

clang/lib/AST/ASTImporter.cpp

namespace clang {
   using llvm::make_error;
   using llvm::Error;
   using llvm::Expected;
   using ExpectedType = llvm::Expected<QualType>;
   using ExpectedStmt = llvm::Expected<Stmt *>;
   using ExpectedExpr = llvm::Expected<Expr *>;
   using ExpectedDecl = llvm::Expected<Decl *>;
   using ExpectedSLoc = llvm::Expected<SourceLocation>;
+  using ExpectedName = llvm::Expected<DeclarationName>;
 
   std::string ImportError::toString() const {
     // FIXME: Improve error texts.
     switch (Error) {
     case NameConflict:
       return "NameConflict";
     case UnsupportedConstruct:
       return "UnsupportedConstruct";
for (auto *FoundDecl : FoundDecls) {
         ConflictingDecls.clear();
         break;
       }
 
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, Decl::IDNS_Namespace, ConflictingDecls.data(),
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
for (auto *FoundDecl : FoundDecls) {
       if (auto *FoundTypedef = dyn_cast<TypedefNameDecl>(FoundDecl)) {
         QualType FromUT = D->getUnderlyingType();
         QualType FoundUT = FoundTypedef->getUnderlyingType();
         if (Importer.IsStructurallyEquivalent(FromUT, FoundUT)) {
           // If the "From" context has a complete underlying type but we
           // already have a complete underlying type then return with that.
           if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
             return Importer.MapImported(D, FoundTypedef);
-        }
           // FIXME Handle redecl chain. When you do that make consistent changes
           // in ASTImporterLookupTable too.
-        break;
-      }
-
+        } else {
           ConflictingDecls.push_back(FoundDecl);
         }
+      }
+    }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   QualType ToUnderlyingType;
   TypeSourceInfo *ToTypeSourceInfo;
   SourceLocation ToBeginLoc;
   if (auto Imp = importSeq(
       D->getUnderlyingType(), D->getTypeSourceInfo(), D->getBeginLoc()))
for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(IDNS))
         continue;
       if (auto *FoundAlias = dyn_cast<TypeAliasTemplateDecl>(FoundDecl))
         return Importer.MapImported(D, FoundAlias);
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   TemplateParameterList *ToTemplateParameters;
   TypeAliasDecl *ToTemplatedDecl;
   if (auto Imp = importSeq(D->getTemplateParameters(), D->getTemplatedDecl()))
     std::tie(ToTemplateParameters, ToTemplatedDecl) = *Imp;
   else
for (auto *FoundDecl : FoundDecls) {
           FoundDecl = Tag->getDecl();
       }
 
       if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
         if (!hasSameVisibilityContext(FoundEnum, D))
           continue;
         if (IsStructuralMatch(D, FoundEnum))
           return Importer.MapImported(D, FoundEnum);
-      }
-
         ConflictingDecls.push_back(FoundDecl);
       }
+    }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          SearchName, DC, IDNS, ConflictingDecls.data(),
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   SourceLocation ToBeginLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   QualType ToIntegerType;
   if (auto Imp = importSeq(
       D->getBeginLoc(), D->getQualifierLoc(), D->getIntegerType()))
for (auto *FoundDecl : FoundDecls) {
                 // because implicit methods are created only if they are used.
                 if (Error Err = ImportImplicitMethods(DCXX, FoundCXX))
                   return std::move(Err);
             }
           }
           PrevDecl = FoundRecord->getMostRecentDecl();
           break;
         }
-      }
-
         ConflictingDecls.push_back(FoundDecl);
+      } // kind is RecordDecl
     } // for
 
     if (!ConflictingDecls.empty() && SearchName) {
-      Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          SearchName, DC, IDNS, ConflictingDecls.data(),
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   // Create the record declaration.
if (!LexicalDC->isFunctionOrMethod()) {
     auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
     for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(IDNS))
         continue;
 
       if (auto *FoundEnumConstant = dyn_cast<EnumConstantDecl>(FoundDecl)) {
         if (IsStructuralMatch(D, FoundEnumConstant))
           return Importer.MapImported(D, FoundEnumConstant);
-      }
-
         ConflictingDecls.push_back(FoundDecl);
       }
+    }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   ExpectedType TypeOrErr = import(D->getType());
   if (!TypeOrErr)
     return TypeOrErr.takeError();
 
   ExpectedExpr InitOrErr = import(D->getInitExpr());
for (auto *FoundDecl : FoundDecls) {
         if (Importer.getToContext().getLangOpts().CPlusPlus)
           continue;
 
         // Complain about inconsistent function types.
         Importer.ToDiag(Loc, diag::warn_odr_function_type_inconsistent)
             << Name << D->getType() << FoundFunction->getType();
         Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
             << FoundFunction->getType();
-      }
-
         ConflictingDecls.push_back(FoundDecl);
       }
+    }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   // We do not allow more than one in-class declaration of a function. This is
   // because AST clients like VTableBuilder asserts on this. VTableBuilder
   // assumes there is only one in-class declaration. Building a redecl
   // chain would result in more than one in-class declaration for
   // overrides (even if they are part of the same redecl chain inside the
if (FieldDecl *FoundField = dyn_cast<FieldDecl>(FoundDecl)) {
       }
 
       // FIXME: Why is this case not handled with calling HandleNameConflict?
       Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
         << Name << D->getType() << FoundField->getType();
       Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
         << FoundField->getType();
 
       return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
   QualType ToType;
   TypeSourceInfo *ToTInfo;
   Expr *ToBitWidth;
   SourceLocation ToInnerLocStart;
   Expr *ToInitializer;
for (auto *FoundDecl : FoundDecls) {
             break;
           }
         }
 
         Importer.ToDiag(Loc, diag::warn_odr_variable_type_inconsistent)
           << Name << D->getType() << FoundVar->getType();
         Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
           << FoundVar->getType();
-      }
-
         ConflictingDecls.push_back(FoundDecl);
       }
+    }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
   QualType ToType;
   TypeSourceInfo *ToTypeSourceInfo;
   SourceLocation ToInnerLocStart;
   NestedNameSpecifierLoc ToQualifierLoc;
   if (auto Imp = importSeq(
for (auto *FoundDecl : FoundDecls) {
           if (D->isThisDeclarationADefinition() && TemplateWithDef)
             return Importer.MapImported(D, TemplateWithDef);
           if (!FoundByLookup)
             FoundByLookup = FoundTemplate;
           // Search in all matches because there may be multiple decl chains,
           // see ASTTests test ImportExistingFriendClassTemplateDef.
           continue;
         }
-      }
-
         ConflictingDecls.push_back(FoundDecl);
       }
+    }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, Decl::IDNS_Ordinary, ConflictingDecls.data(),
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
-
-    if (!Name)
-      return make_error<ImportError>(ImportError::NameConflict);
   }
 
   CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
 
   // Create the declaration that is being templated.
   CXXRecordDecl *ToTemplated;
   if (Error Err = importInto(ToTemplated, FromTemplated))
     return std::move(Err);
for (auto *FoundDecl : FoundDecls) {
     Decl *Found = FoundDecl;
     if (VarTemplateDecl *FoundTemplate = dyn_cast<VarTemplateDecl>(Found)) {
       if (IsStructuralMatch(D, FoundTemplate)) {
         // The variable templates structurally match; call it the same template.
         Importer.MapImported(D->getTemplatedDecl(),
                              FoundTemplate->getTemplatedDecl());
         return Importer.MapImported(D, FoundTemplate);
       }
-    }
-
       ConflictingDecls.push_back(FoundDecl);
     }
+  }
 
   if (!ConflictingDecls.empty()) {
-    Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
-                                       ConflictingDecls.data(),
-                                       ConflictingDecls.size());
+    ExpectedName NameOrErr = Importer.HandleNameConflict(
+        Name, DC, Decl::IDNS_Ordinary, ConflictingDecls.data(),
+    if (NameOrErr)
+      Name = NameOrErr.get();
+    else
+      return NameOrErr.takeError();
   }
 
-  if (!Name)
-    // FIXME: Is it possible to get other error than name conflict?
-    // (Put this `if` into the previous `if`?)
-    return make_error<ImportError>(ImportError::NameConflict);
-
   VarDecl *DTemplated = D->getTemplatedDecl();
 
   // Import the type.
   // FIXME: Value not used?
   ExpectedType TypeOrErr = import(DTemplated->getType());
   if (!TypeOrErr)
     return TypeOrErr.takeError();
 

 }
 
 ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          ASTContext &FromContext, FileManager &FromFileManager,
                          bool MinimalImport,
                          std::shared_ptr<ASTImporterSharedState> SharedState)
     : SharedState(SharedState), ToContext(ToContext), FromContext(FromContext),
       ToFileManager(ToFileManager), FromFileManager(FromFileManager),
-      Minimal(MinimalImport) {
+      Minimal(MinimalImport), ODRHandling(ODRHandlingType::Conservative) {
 
   // Create a default state without the lookup table: LLDB case.
   if (!SharedState) {
     this->SharedState = std::make_shared<ASTImporterSharedState>();
   }
 
   ImportedDecls[FromContext.getTranslationUnitDecl()] =
       ToContext.getTranslationUnitDecl();
Expected<Selector> ASTImporter::Import(Selector FromSel) {
 
   SmallVector<IdentifierInfo *, 4> Idents;
   Idents.push_back(Import(FromSel.getIdentifierInfoForSlot(0)));
   for (unsigned I = 1, N = FromSel.getNumArgs(); I < N; ++I)
     Idents.push_back(Import(FromSel.getIdentifierInfoForSlot(I)));
   return ToContext.Selectors.getSelector(FromSel.getNumArgs(), Idents.data());
 }
 
-DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
+Expected<DeclarationName> ASTImporter::HandleNameConflict(DeclarationName Name,
                                                           DeclContext *DC,
                                                           unsigned IDNS,
                                                           NamedDecl **Decls,
                                                           unsigned NumDecls) {
+  if (ODRHandling == ODRHandlingType::Conservative)
+    // Report error at any name conflict.
+    return make_error<ImportError>(ImportError::NameConflict);
+  else
+    // Allow to create the new Decl with the same name.
     return Name;
 }
 
 DiagnosticBuilder ASTImporter::ToDiag(SourceLocation Loc, unsigned DiagID) {
   if (LastDiagFromFrom)
     ToContext.getDiagnostics().notePriorDiagnosticFrom(
       FromContext.getDiagnostics());
   LastDiagFromFrom = false;
   return ToContext.getDiagnostics().Report(Loc, DiagID);

clang/test/Analysis/Inputs/ctu-other.c

 // Test typedef and global variable in function.
 typedef struct {
   int a;
   int b;
 } FooBar;
 FooBar fb;
 int f(int i) {
   if (fb.a) {
     fb.b = i;
   }
   return 1;
 }
 
 // Test enums.
-enum B { x = 42,
-         l,
-         s };
+enum B { x2 = 42,
+         y2,
+         z2 };
 int enumCheck(void) {
-  return x;
+  return x2;
 }
 
 // Test reporting an error in macro definition
 #define MYMACRO(ctx) \
   ctx->a;
 struct S {
   int a;
 };

clang/unittests/AST/ASTImporterFixtures.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_UNITTESTS_AST_IMPORTER_FIXTURES_H
 #define LLVM_CLANG_UNITTESTS_AST_IMPORTER_FIXTURES_H
 
 #include "gmock/gmock.h"
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/Frontend/ASTUnit.h"
 #include "clang/AST/ASTImporterSharedState.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 
 #include "DeclMatcher.h"
 #include "Language.h"
 
+#include <sstream>
+
 namespace clang {
 
 class ASTImporter;
 class ASTImporterSharedState;
 class ASTUnit;
 
 namespace ast_matchers {
 

 
 public:
   /// Allocates an ASTImporter (or one of its subclasses).
   typedef std::function<ASTImporter *(
       ASTContext &, FileManager &, ASTContext &, FileManager &, bool,
       const std::shared_ptr<ASTImporterSharedState> &SharedState)>
       ImporterConstructor;
 
+  // ODR handling type for the AST importer.
+  ASTImporter::ODRHandlingType ODRHandling;
+
   // The lambda that constructs the ASTImporter we use in this test.
   ImporterConstructor Creator;
 
 private:
   // Buffer for the To context, must live in the test scope.
   std::string ToCode;
 
   // Represents a "From" translation unit and holds an importer object which we
   // use to import from this translation unit.
   struct TU {
     // Buffer for the context, must live in the test scope.
     std::string Code;
     std::string FileName;
     std::unique_ptr<ASTUnit> Unit;
     TranslationUnitDecl *TUDecl = nullptr;
     std::unique_ptr<ASTImporter> Importer;
     ImporterConstructor Creator;
+    ASTImporter::ODRHandlingType ODRHandling;
 
     TU(StringRef Code, StringRef FileName, ArgVector Args,
-       ImporterConstructor C = ImporterConstructor());
+       ImporterConstructor C = ImporterConstructor(),
+       ASTImporter::ODRHandlingType ODRHandling =
+           ASTImporter::ODRHandlingType::Conservative);
     ~TU();
 
     void
     lazyInitImporter(const std::shared_ptr<ASTImporterSharedState> &SharedState,
                      ASTUnit *ToAST);
     Decl *import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
                  ASTUnit *ToAST, Decl *FromDecl);
+    llvm::Expected<Decl *>
+    importOrError(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                  ASTUnit *ToAST, Decl *FromDecl);
     QualType import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
                     ASTUnit *ToAST, QualType FromType);
   };
 
   // We may have several From contexts and related translation units. In each
   // AST, the buffers for the source are handled via references and are set
   // during the creation of the AST. These references must point to a valid
   // buffer until the AST is alive. Thus, we must use a list in order to avoid
public:
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
   Decl *Import(Decl *From, Language ToLang);
 
   template <class DeclT> DeclT *Import(DeclT *From, Language Lang) {
     return cast_or_null<DeclT>(Import(cast<Decl>(From), Lang));
   }
 
+  // Import the given Decl into the ToCtx.
+  // Same as Import but returns the result of the import which can be an error.
+  llvm::Expected<Decl *> importOrError(Decl *From, Language ToLang);
+
   QualType ImportType(QualType FromType, Decl *TUDecl, Language ToLang);
 
+  ASTImporterTestBase()
+      : ODRHandling(ASTImporter::ODRHandlingType::Conservative) {}
   ~ASTImporterTestBase();
 };
 
 class ASTImporterOptionSpecificTestBase
     : public ASTImporterTestBase,
       public ::testing::WithParamInterface<ArgVector> {
 protected:
   ArgVector getExtraArgs() const override { return GetParam(); }
 };
 
+template <class T>
+::testing::AssertionResult isSuccess(llvm::Expected<T> &ValOrErr) {
+  if (ValOrErr)
+    return ::testing::AssertionSuccess() << "Expected<> contains no error.";
+  else
+    return ::testing::AssertionFailure()
+           << "Expected<> contains error: " << toString(ValOrErr.takeError());
+}
+
+template <class T>
+::testing::AssertionResult isImportError(llvm::Expected<T> &ValOrErr,
+                                         ImportError::ErrorKind Kind) {
+  if (ValOrErr) {
+    return ::testing::AssertionFailure() << "Expected<> is expected to contain "
+                                            "error but does contain value \""
+                                         << (*ValOrErr) << "\"";
+  } else {
+    std::ostringstream OS;
+    bool Result = false;
+    auto Err = llvm::handleErrors(
+        ValOrErr.takeError(), [&OS, &Result, Kind](clang::ImportError &IE) {
+          if (IE.Error == Kind) {
+            Result = true;
+            OS << "Expected<> contains an ImportError " << IE.toString();
+          } else {
+            OS << "Expected<> contains an ImportError " << IE.toString()
+               << " instead of kind " << Kind;
+          }
+        });
+    if (Err) {
+      OS << "Expected<> contains unexpected error: "
+         << toString(std::move(Err));
+    }
+    if (Result)
+      return ::testing::AssertionSuccess() << OS.str();
+    else
+      return ::testing::AssertionFailure() << OS.str();
+  }
+}
+
 } // end namespace ast_matchers
 } // end namespace clang
 
 #endif

clang/unittests/AST/ASTImporterFixtures.cpp

 
 void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
                                StringRef Code) {
   return createVirtualFileIfNeeded(ToAST, FileName,
                                    llvm::MemoryBuffer::getMemBuffer(Code));
 }
 
 ASTImporterTestBase::TU::TU(StringRef Code, StringRef FileName, ArgVector Args,
-                            ImporterConstructor C)
+                            ImporterConstructor C,
+                            ASTImporter::ODRHandlingType ODRHandling)
     : Code(Code), FileName(FileName),
       Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args, this->FileName)),
-      TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C) {
+      TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C),
+      ODRHandling(ODRHandling) {
   Unit->enableSourceFileDiagnostics();
 
   // If the test doesn't need a specific ASTImporter, we just create a
   // normal ASTImporter with it.
   if (!Creator)
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
                  bool MinimalImport,
                  const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporter(ToContext, ToFileManager, FromContext,
                              FromFileManager, MinimalImport, SharedState);
     };
 }
 
 ASTImporterTestBase::TU::~TU() {}
 
 void ASTImporterTestBase::TU::lazyInitImporter(
     const std::shared_ptr<ASTImporterSharedState> &SharedState,
     ASTUnit *ToAST) {
   assert(ToAST);
-  if (!Importer)
+  if (!Importer) {
     Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
                            Unit->getASTContext(), Unit->getFileManager(), false,
                            SharedState));
+    Importer->setODRHandling(ODRHandling);
+  }
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
 
 Decl *ASTImporterTestBase::TU::import(
     const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
     Decl *FromDecl) {
   lazyInitImporter(SharedState, ToAST);
   if (auto ImportedOrErr = Importer->Import(FromDecl))
     return *ImportedOrErr;
   else {
     llvm::consumeError(ImportedOrErr.takeError());
     return nullptr;
   }
 }
 
+llvm::Expected<Decl *> ASTImporterTestBase::TU::importOrError(
+    const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
+    Decl *FromDecl) {
+  lazyInitImporter(SharedState, ToAST);
+  return Importer->Import(FromDecl);
+}
+
 QualType ASTImporterTestBase::TU::import(
     const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
     QualType FromType) {
   lazyInitImporter(SharedState, ToAST);
   if (auto ImportedOrErr = Importer->Import(FromType))
     return *ImportedOrErr;
   else {
     llvm::consumeError(ImportedOrErr.takeError());

 
 std::tuple<Decl *, Decl *>
 ASTImporterTestBase::getImportedDecl(StringRef FromSrcCode, Language FromLang,
                                      StringRef ToSrcCode, Language ToLang,
                                      StringRef Identifier) {
   ArgVector FromArgs = getArgVectorForLanguage(FromLang),
             ToArgs = getArgVectorForLanguage(ToLang);
 
-  FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Creator);
+  FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Creator,
+                       ODRHandling);
   TU &FromTU = FromTUs.back();
 
   assert(!ToAST);
   lazyInitToAST(ToLang, ToSrcCode, OutputFileName);
 
   ASTContext &FromCtx = FromTU.Unit->getASTContext();
 
   IdentifierInfo *ImportedII = &FromCtx.Idents.get(Identifier);

 TranslationUnitDecl *ASTImporterTestBase::getTuDecl(StringRef SrcCode,
                                                     Language Lang,
                                                     StringRef FileName) {
   assert(llvm::find_if(FromTUs, [FileName](const TU &E) {
            return E.FileName == FileName;
          }) == FromTUs.end());
 
   ArgVector Args = getArgVectorForLanguage(Lang);
-  FromTUs.emplace_back(SrcCode, FileName, Args, Creator);
+  FromTUs.emplace_back(SrcCode, FileName, Args, Creator, ODRHandling);
   TU &Tu = FromTUs.back();
 
   return Tu.TUDecl;
 }
 
 TranslationUnitDecl *ASTImporterTestBase::getToTuDecl(StringRef ToSrcCode,
                                                       Language ToLang) {
   ArgVector ToArgs = getArgVectorForLanguage(ToLang);
   assert(!ToAST);
   lazyInitToAST(ToLang, ToSrcCode, OutputFileName);
   return ToAST->getASTContext().getTranslationUnitDecl();
 }
 
 Decl *ASTImporterTestBase::Import(Decl *From, Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);
   TU *FromTU = findFromTU(From);
   assert(SharedStatePtr);
   Decl *To = FromTU->import(SharedStatePtr, ToAST.get(), From);
   return To;
 }
 
+llvm::Expected<Decl *> ASTImporterTestBase::importOrError(Decl *From,
+                                                          Language ToLang) {
+  lazyInitToAST(ToLang, "", OutputFileName);
+  TU *FromTU = findFromTU(From);
+  assert(SharedStatePtr);
+  llvm::Expected<Decl *> To =
+      FromTU->importOrError(SharedStatePtr, ToAST.get(), From);
+  return To;
+}
+
 QualType ASTImporterTestBase::ImportType(QualType FromType, Decl *TUDecl,
                                          Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);
   TU *FromTU = findFromTU(TUDecl);
   assert(SharedStatePtr);
   return FromTU->import(SharedStatePtr, ToAST.get(), FromType);
 }
 

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, AnonymousRecordsReversed) {
   auto *X1 =
       FirstDeclMatcher<RecordDecl>().match(FromTU1, recordDecl(hasName("X")));
   Import(X0, Lang_C);
   Import(X1, Lang_C);
 
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
             DeclCounter<RecordDecl>().match(ToTU, recordDecl(hasName("X"))));
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase, ImportDoesUpdateUsedFlag) {
   auto Pattern = varDecl(hasName("x"));
   VarDecl *Imported1;
   {
     Decl *FromTU = getTuDecl("extern int x;", Lang_CXX, "input0.cc");
TEST_P(ImportFunctions,
   auto *ToD = Import(FromD, Lang_CXX);
   EXPECT_TRUE(ToD);
   Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   EXPECT_TRUE(MatchVerifier<TranslationUnitDecl>().match(
       ToTU, translationUnitDecl(hasDescendant(
                 functionDecl(hasName("f"), hasDescendant(declRefExpr()))))));
 }
 
 struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(ImportFunctionTemplates, ImportFunctionTemplateInRecordDeclTwice) {
   auto Code =
       R"(
       class X {
         template <class T>
         void f(T t);
       };
TEST_P(ImportFunctionTemplates,
   auto *ToD1 = Import(FromD1, Lang_CXX);
   Decl *FromTU2 = getTuDecl(Code, Lang_CXX, "input2.cc");
   auto *FromD2 = FirstDeclMatcher<FunctionTemplateDecl>().match(
       FromTU2, functionTemplateDecl(hasName("f")));
   auto *ToD2 = Import(FromD2, Lang_CXX);
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+       ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+      R"(
+      template <typename T>
+      void foo(T) {}
+      void foo();
+      )",
+      Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+       ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+      R"(
+      struct Foo {
+        template <typename T>
+        Foo(T) {}
+        Foo();
+      };
+      )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+      LastDeclMatcher<CXXConstructorDecl>().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+       ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+      R"(
+      template <typename T>
+      void operator<(T,T) {}
+      struct X{};
+      void operator<(X, X);
+      )",
+      Lang_CXX);
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct X{};
+      void operator<(X, X);
+      )",
+      Lang_CXX);
+  auto *FromD = LastDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
   auto Pattern = functionDecl(hasName("f"));
 
   Decl *FromTU = getTuDecl("struct X { friend void f(); };"
                            "void f();",
                            Lang_CXX,
// Try to import  X again but from another TU.
     ASTImporter *Importer = findFromTU(FromX)->Importer.get();
     Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
     // And an error is set for this new X in the "from" ctx.
     ASSERT_TRUE(OptErr);
     EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-                        DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-                        ::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-                        ::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
       R"(
       void f() {
         auto L = [](){};
       }
       )",
       Lang_CXX11, "input0.cc");
TEST_P(ASTImporterOptionSpecificTestBase,
   ASSERT_EQ(FromClassDef->getMostRecentDecl(), FromClassDef);
   ASSERT_EQ(FromClassProto->getMostRecentDecl(), FromClassProto);
 
   auto *ImportedDef = Import(FromClassDef, Lang_CXX);
   // At import we should find the definition for 'Class' even if the
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
                  bool MinimalImport,
                  const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporter(ToContext, ToFileManager, FromContext,
                              FromFileManager, MinimalImport,
TEST_P(ASTImporterOptionSpecificTestBase,
 
   ASSERT_TRUE(ImportedX);
   EXPECT_TRUE(Constr1->isDefaulted());
   EXPECT_TRUE(Constr1->isExplicitlyDefaulted());
   EXPECT_TRUE(Constr2->isDeletedAsWritten());
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+    this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template <typename DeclTy, typename PatternTy>
+static void CheckImportedAsNew(llvm::Expected<Decl *> &Result, Decl *ToTU,
+                               PatternTy Pattern) {
+  ASSERT_TRUE(isSuccess(Result));
+  Decl *ImportedD = *Result;
+  ASSERT_TRUE(ImportedD);
+  auto *ToD = FirstDeclMatcher<DeclTy>().match(ToTU, Pattern);
+  EXPECT_NE(ImportedD, ToD);
+  EXPECT_FALSE(ImportedD->getPreviousDecl());
+  EXPECT_EQ(DeclCounter<DeclTy>().match(ToTU, Pattern), 2u);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      typedef int X;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      typedef double X;
+      )",
+      Lang_CXX11);
+  auto Pattern = typedefNameDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<TypedefNameDecl>().match(FromTU, Pattern);
+
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<TypedefNameDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, TypeAlias) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      using X = int;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      using X = double;
+      )",
+      Lang_CXX11);
+  auto Pattern = typedefNameDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<TypedefNameDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<TypedefNameDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, EnumDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      enum X { a, b };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      enum X { a, b, c };
+      )",
+      Lang_CXX11);
+  auto Pattern = enumDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<EnumDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<EnumDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, EnumConstantDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      enum E { X = 0 };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      enum E { X = 1 };
+      )",
+      Lang_CXX11);
+  auto Pattern = enumConstantDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<EnumConstantDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<EnumConstantDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, RecordDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      class X { int a; };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      class X { int b; };
+      )",
+      Lang_CXX11);
+  auto Pattern = cxxRecordDecl(hasName("X"), unless(isImplicit()));
+  auto *FromX = FirstDeclMatcher<RecordDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<RecordDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, VarDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      int X;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      double X;
+      )",
+      Lang_CXX11);
+  auto Pattern = varDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<VarDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, FunctionDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      void X(int);
+      )",
+      Lang_C); // C, no overloading!
+  Decl *FromTU = getTuDecl(
+      R"(
+      void X(double);
+      )",
+      Lang_C);
+  auto Pattern = functionDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<FunctionDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<FunctionDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, ClassTemplateDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class>
+      struct X;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <int>
+      struct X;
+      )",
+      Lang_CXX11);
+  auto Pattern = classTemplateDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<ClassTemplateDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<ClassTemplateDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, DISABLED_VarTemplateDecl) {
+  const internal::VariadicDynCastAllOfMatcher<Decl, VarTemplateDecl>
+      varTemplateDecl;
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class T>
+      constexpr T X;
+      )",
+      Lang_CXX14);
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <int>
+      constexpr int X = 0;
+      )",
+      Lang_CXX14);
+  auto Pattern = varTemplateDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("X")));
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<VarTemplateDecl>(Result, ToTU, Pattern);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins,
                         ::testing::Values(ArgVector{"-target",
                                                     "aarch64-linux-gnu"}), );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
+                        ::testing::Values(ArgVector()), );
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
+                        ::testing::Values(ArgVector()), );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportType,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportDecl,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterOptionSpecificTestBase,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, RedirectingImporterTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
                         DefaultTestValuesForRunOptions, );
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses,
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctions,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendClasses,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests,
                         ImportFunctionTemplateSpecializations,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ConflictingDeclsWithLiberalStrategy,
+                        DefaultTestValuesForRunOptions, );
 
 } // end namespace ast_matchers
 } // end namespace clang

lldb/include/lldb/Symbol/ClangASTImporter.h

private:
   /// their counterpart from a C++ module.
   class ASTImporterDelegate : public clang::ASTImporter {
   public:
     ASTImporterDelegate(ClangASTImporter &master, clang::ASTContext *target_ctx,
                         clang::ASTContext *source_ctx)
         : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
                              master.m_file_manager, true /*minimal*/),
           m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-          m_master(master), m_source_ctx(source_ctx) {}
+          m_master(master), m_source_ctx(source_ctx) {
+      setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+    }
 
     /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
     /// and deattaches it at the end of the scope. Supports being used multiple
     /// times on the same ASTImporterDelegate instance in nested scopes.
     class CxxModuleScope {
       /// The handler we attach to the ASTImporterDelegate.
       CxxModuleHandler m_handler;
       /// The ASTImporterDelegate we are supposed to attach the handler to.