vsapsai created this revision.
Herald added a subscriber: ributzka.
vsapsai requested review of this revision.
Herald added a project: clang.
The test case is for Objective-C but it is likely other languages like
C++ are affected as well, just haven't created appropriate test cases.
The test case fails with the following error
clang/test/Modules/Output/ambiguous-enum-lookup.m.tmp/test.m:7:11: error:
reference to 'kEnumValue' is ambiguous
s.x = kEnumValue;
^
clang/test/Modules/Output/ambiguous-enum-lookup.m.tmp/Frameworks/NonModular.framework/Headers/NonModular.h:8:5:
note: candidate found by name lookup is 'kEnumValue'
kEnumValue = 1,
^
clang/test/Modules/Output/ambiguous-enum-lookup.m.tmp/Frameworks/NonModular.framework/Headers/NonModular.h:8:5:
note: candidate found by name lookup is 'kEnumValue'
kEnumValue = 1,
^
It happens because we have 2 EnumConstantDecl for 'kEnumValue' - one from
PiecewiseVisible module, another non-modular. And the the name lookup is
ambiguous because these decls have different canonical decls.
Non-modular decl believes it is the canonical decl because that's the
behavior for Mergeable and in the source code non-modular decl comes
before other EnumConstantDecl. Modular EnumConstantDecl has no intention
of being canonical decl but it is deserialized *before* non-modular decl
is created, so as the only decl it becomes the canonical decl.
The idea for the fix is to skip deserialization of decls from hidden
(sub)modules, thus allowing to establish correct canonical decl. This is
a proof of concept, the implementation isn't optimal, and there are
failing tests. I mostly wanted to share the approach to discuss it.
Another high-level approach I've considered is to enhance handling decls
from hidden modules and to delay connecting them with redecl chains and
canonical decls till the moment they become visible. I've decided to go
with "avoid deserialization" approach because this way we should be
doing less work and it seems less error-prone compared to juggling with
existing decls.
rdar://82908206
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114411
Files:
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/test/Modules/ambiguous-enum-lookup.m
Index: clang/test/Modules/ambiguous-enum-lookup.m
===================================================================
--- /dev/null
+++ clang/test/Modules/ambiguous-enum-lookup.m
@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -F %t/Frameworks %t/test.m
+
+//--- Frameworks/NonModular.framework/Headers/NonModular.h
+#ifndef NonModular_NonModular_h
+#define NonModular_NonModular_h
+struct SomeStruct {
+ int x;
+};
+
+enum SomeEnum {
+ kEnumValue = 1,
+};
+#endif
+
+//--- Frameworks/PiecewiseVisible.framework/Headers/InitiallyVisible.h
+// empty
+
+//--- Frameworks/PiecewiseVisible.framework/Headers/InitiallyHidden.h
+#include <NonModular/NonModular.h>
+
+//--- Frameworks/PiecewiseVisible.framework/Modules/module.modulemap
+framework module PiecewiseVisible {
+ header "InitiallyVisible.h"
+ export *
+
+ explicit module HiddenPiece {
+ header "InitiallyHidden.h"
+ export *
+ }
+}
+
+//--- test.m
+#include <PiecewiseVisible/InitiallyVisible.h>
+#include <NonModular/NonModular.h>
+#include <PiecewiseVisible/InitiallyHidden.h>
+
+void test() {
+ struct SomeStruct s;
+ s.x = kEnumValue;
+}
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===================================================================
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -269,6 +269,7 @@
}
void ASTDeclWriter::Visit(Decl *D) {
+ Record.push_back(Writer.getSubmoduleID(D->getOwningModule()));
DeclVisitor<ASTDeclWriter>::Visit(D);
// Source locations require array (variable-length) abbreviations. The
@@ -1911,6 +1912,7 @@
// Abbreviation for DECL_FIELD
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_FIELD));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Decl
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
Abv->Add(BitCodeAbbrevOp(0)); // LexicalDeclContext
@@ -1944,6 +1946,7 @@
// Abbreviation for DECL_OBJC_IVAR
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_OBJC_IVAR));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Decl
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclContext
Abv->Add(BitCodeAbbrevOp(0)); // LexicalDeclContext
@@ -1980,6 +1983,7 @@
// Abbreviation for DECL_ENUM
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_ENUM));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Redeclarable
Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration
// Decl
@@ -2030,6 +2034,7 @@
// Abbreviation for DECL_RECORD
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_RECORD));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Redeclarable
Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration
// Decl
@@ -2092,6 +2097,7 @@
// Abbreviation for DECL_PARM_VAR
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_PARM_VAR));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Redeclarable
Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration
// Decl
@@ -2140,6 +2146,7 @@
// Abbreviation for DECL_TYPEDEF
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_TYPEDEF));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Redeclarable
Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration
// Decl
@@ -2169,6 +2176,7 @@
// Abbreviation for DECL_VAR
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_VAR));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// Redeclarable
Abv->Add(BitCodeAbbrevOp(0)); // No redeclaration
// Decl
@@ -2221,6 +2229,7 @@
// Abbreviation for DECL_CXX_METHOD
Abv = std::make_shared<BitCodeAbbrev>();
Abv->Add(BitCodeAbbrevOp(serialization::DECL_CXX_METHOD));
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // SubmoduleID
// RedeclarableDecl
Abv->Add(BitCodeAbbrevOp(0)); // CanonicalDecl
// Decl
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3870,6 +3870,15 @@
llvm::report_fatal_error(
Twine("ASTReader::readDeclRecord failed reading decl code: ") +
toString(MaybeDeclCode.takeError()));
+ if (unsigned DeclSubmoduleID = getGlobalSubmoduleID(*Loc.F, Record.readInt())) {
+ if (SemaObj) {
+ Module *DeclOwner = getSubmodule(DeclSubmoduleID);
+ if (DeclOwner && !SemaObj->isModuleVisible(DeclOwner)) {
+ // Skip deserialization of Decls from a hidden module.
+ return nullptr;
+ }
+ }
+ }
switch ((DeclCode)MaybeDeclCode.get()) {
case DECL_CONTEXT_LEXICAL:
case DECL_CONTEXT_VISIBLE:
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8474,18 +8474,20 @@
continue;
}
- NamedDecl *D = cast<NamedDecl>(GetDecl(DeclIDs[I]));
+ if (Decl *DeserializedD = GetDecl(DeclIDs[I])) {
+ NamedDecl *D = cast<NamedDecl>(DeserializedD);
- // If we're simply supposed to record the declarations, do so now.
- if (Decls) {
- Decls->push_back(D);
- continue;
- }
+ // If we're simply supposed to record the declarations, do so now.
+ if (Decls) {
+ Decls->push_back(D);
+ continue;
+ }
- // Introduce this declaration into the translation-unit scope
- // and add it to the declaration chain for this identifier, so
- // that (unqualified) name lookup will find it.
- pushExternalDeclIntoScope(D, II);
+ // Introduce this declaration into the translation-unit scope
+ // and add it to the declaration chain for this identifier, so
+ // that (unqualified) name lookup will find it.
+ pushExternalDeclIntoScope(D, II);
+ }
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
