Skip to content

Commit

Permalink
[ASTImporter] Fix name conflict handling with different strategies
Browse files Browse the repository at this point in the history 
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

Reviewers: a_sidorin, shafik

Differential Revision: https://reviews.llvm.org/D59692

llvm-svn: 370045
  • Loading branch information
Gabor Marton committed Aug 27, 2019
1 parent c397a26 commit f035b75
Show file tree
Hide file tree
Showing 7 changed files with 450 additions and 114 deletions.
17 changes: 11 additions & 6 deletions clang/include/clang/AST/ASTImporter.h
View file
Expand Up @@ -90,6 +90,8 @@ class TypeSourceInfo;
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.
Expand Down Expand Up @@ -236,6 +238,8 @@ class TypeSourceInfo;
/// Whether to perform a minimal import.
bool Minimal;

ODRHandlingType ODRHandling;

/// Whether the last diagnostic came from the "from" context.
bool LastDiagFromFrom = false;

Expand Down Expand Up @@ -326,6 +330,8 @@ class TypeSourceInfo;
/// 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.
Expand Down Expand Up @@ -517,12 +523,11 @@ class TypeSourceInfo;
///
/// \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; }
Expand Down
171 changes: 90 additions & 81 deletions clang/lib/AST/ASTImporter.cpp
View file
Expand Up @@ -80,6 +80,7 @@ namespace clang {
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.
Expand Down Expand Up @@ -2247,11 +2248,13 @@ ExpectedDecl ASTNodeImporter::VisitNamespaceDecl(NamespaceDecl *D) {
}

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(),
ConflictingDecls.size());
if (NameOrErr)
Name = NameOrErr.get();
else
return NameOrErr.takeError();
}
}

Expand Down Expand Up @@ -2355,21 +2358,21 @@ ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
// 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.
} else {
ConflictingDecls.push_back(FoundDecl);
}
// FIXME Handle redecl chain. When you do that make consistent changes
// in ASTImporterLookupTable too.
break;
}

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

Expand Down Expand Up @@ -2442,11 +2445,12 @@ ASTNodeImporter::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
}

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

Expand Down Expand Up @@ -2550,17 +2554,18 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
continue;
if (IsStructuralMatch(D, FoundEnum))
return Importer.MapImported(D, FoundEnum);
ConflictingDecls.push_back(FoundDecl);
}

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(),
ConflictingDecls.size());
if (NameOrErr)
Name = NameOrErr.get();
else
return NameOrErr.takeError();
}
}

Expand Down Expand Up @@ -2685,17 +2690,18 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
PrevDecl = FoundRecord->getMostRecentDecl();
break;
}
}

ConflictingDecls.push_back(FoundDecl);
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(),
ConflictingDecls.size());
if (NameOrErr)
Name = NameOrErr.get();
else
return NameOrErr.takeError();
}
}

Expand Down Expand Up @@ -2854,17 +2860,17 @@ ExpectedDecl ASTNodeImporter::VisitEnumConstantDecl(EnumConstantDecl *D) {
if (auto *FoundEnumConstant = dyn_cast<EnumConstantDecl>(FoundDecl)) {
if (IsStructuralMatch(D, FoundEnumConstant))
return Importer.MapImported(D, FoundEnumConstant);
ConflictingDecls.push_back(FoundDecl);
}

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

Expand Down Expand Up @@ -3102,17 +3108,17 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
<< Name << D->getType() << FoundFunction->getType();
Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
<< FoundFunction->getType();
ConflictingDecls.push_back(FoundDecl);
}

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

Expand Down Expand Up @@ -3758,17 +3764,17 @@ ExpectedDecl ASTNodeImporter::VisitVarDecl(VarDecl *D) {
<< Name << D->getType() << FoundVar->getType();
Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
<< FoundVar->getType();
ConflictingDecls.push_back(FoundDecl);
}

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

Expand Down Expand Up @@ -5106,19 +5112,19 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
// see ASTTests test ImportExistingFriendClassTemplateDef.
continue;
}
ConflictingDecls.push_back(FoundDecl);
}

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(),
ConflictingDecls.size());
if (NameOrErr)
Name = NameOrErr.get();
else
return NameOrErr.takeError();
}

if (!Name)
return make_error<ImportError>(ImportError::NameConflict);
}

CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
Expand Down Expand Up @@ -5391,22 +5397,20 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
FoundTemplate->getTemplatedDecl());
return Importer.MapImported(D, FoundTemplate);
}
ConflictingDecls.push_back(FoundDecl);
}

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(),
ConflictingDecls.size());
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.
Expand Down Expand Up @@ -7817,7 +7821,7 @@ ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
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) {
Expand Down Expand Up @@ -8744,12 +8748,17 @@ Expected<Selector> ASTImporter::Import(Selector FromSel) {
return ToContext.Selectors.getSelector(FromSel.getNumArgs(), Idents.data());
}

DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
DeclContext *DC,
unsigned IDNS,
NamedDecl **Decls,
unsigned NumDecls) {
return 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) {
Expand Down
8 changes: 4 additions & 4 deletions clang/test/Analysis/Inputs/ctu-other.c
View file
Expand Up @@ -12,11 +12,11 @@ int f(int i) {
}

// 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
Expand Down

0 comments on commit f035b75

Please sign in to comment.