Skip to content

Frontend: add a front-end option to specify module names for which we prefer loading via interfaces #26894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 28, 2019
Merged

Frontend: add a front-end option to specify module names for which we prefer loading via interfaces #26894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/swift/Frontend/FrontendOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ class FrontendOptions {
/// binary module has already been built for use by the compiler.
std::string PrebuiltModuleCachePath;

/// For these modules, we should prefer using Swift interface when importing them.
std::vector<std::string> PreferInterfaceForModules;

/// Emit index data for imported serialized swift system modules.
bool IndexSystemModules = false;

Expand Down
5 changes: 4 additions & 1 deletion include/swift/Frontend/ParseableInterfaceModuleLoader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,9 @@ class ParseableInterfaceModuleLoader : public SerializedModuleLoaderBase {
explicit ParseableInterfaceModuleLoader(
ASTContext &ctx, StringRef cacheDir, StringRef prebuiltCacheDir,
DependencyTracker *tracker, ModuleLoadingMode loadMode,
ArrayRef<std::string> PreferInterfaceForModules,
bool RemarkOnRebuildFromInterface)
: SerializedModuleLoaderBase(ctx, tracker, loadMode),
: SerializedModuleLoaderBase(ctx, tracker, loadMode, PreferInterfaceForModules),
CacheDir(cacheDir), PrebuiltCacheDir(prebuiltCacheDir),
RemarkOnRebuildFromInterface(RemarkOnRebuildFromInterface)
{}
Expand All @@ -154,10 +155,12 @@ class ParseableInterfaceModuleLoader : public SerializedModuleLoaderBase {
static std::unique_ptr<ParseableInterfaceModuleLoader>
create(ASTContext &ctx, StringRef cacheDir, StringRef prebuiltCacheDir,
DependencyTracker *tracker, ModuleLoadingMode loadMode,
ArrayRef<std::string> PreferInterfaceForModules = {},
bool RemarkOnRebuildFromInterface = false) {
return std::unique_ptr<ParseableInterfaceModuleLoader>(
new ParseableInterfaceModuleLoader(ctx, cacheDir, prebuiltCacheDir,
tracker, loadMode,
PreferInterfaceForModules,
RemarkOnRebuildFromInterface));
}

Expand Down
4 changes: 3 additions & 1 deletion include/swift/Serialization/SerializedModuleLoader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ class SerializedModuleLoaderBase : public ModuleLoader {
protected:
ASTContext &Ctx;
ModuleLoadingMode LoadMode;
ArrayRef<std::string> PreferInterfaceForModules;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this the first time around. This doesn't belong in SerializedModuleLoaderBase; please move it up to ParseableInterfaceModuleLoader.

SerializedModuleLoaderBase(ASTContext &ctx, DependencyTracker *tracker,
ModuleLoadingMode LoadMode);
ModuleLoadingMode LoadMode,
ArrayRef<std::string> PreferInterfaceForModules = {});

void collectVisibleTopLevelModuleNamesImpl(SmallVectorImpl<Identifier> &names,
StringRef extension) const;
Expand Down
3 changes: 2 additions & 1 deletion lib/Frontend/Frontend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,8 @@ bool CompilerInstance::setUpModuleLoaders() {
StringRef PrebuiltModuleCachePath = FEOpts.PrebuiltModuleCachePath;
auto PIML = ParseableInterfaceModuleLoader::create(
*Context, ModuleCachePath, PrebuiltModuleCachePath,
getDependencyTracker(), MLM, FEOpts.RemarkOnRebuildFromModuleInterface);
getDependencyTracker(), MLM, FEOpts.PreferInterfaceForModules,
FEOpts.RemarkOnRebuildFromModuleInterface);
Context->addModuleLoader(std::move(PIML));
}
Context->addModuleLoader(std::move(SML));
Expand Down
7 changes: 5 additions & 2 deletions lib/Frontend/ParseableInterfaceModuleLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,11 +1435,14 @@ std::error_code ParseableInterfaceModuleLoader::findModuleFilesInDirectory(
}

// Create an instance of the Impl to do the heavy lifting.
auto ModuleName = ModuleID.first.str();
ParseableInterfaceModuleLoaderImpl Impl(
Ctx, ModPath, InPath, ModuleID.first.str(),
Ctx, ModPath, InPath, ModuleName,
CacheDir, PrebuiltCacheDir, ModuleID.second,
RemarkOnRebuildFromInterface, dependencyTracker,
LoadMode);
llvm::is_contained(PreferInterfaceForModules,
ModuleName)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a space again.

ModuleLoadingMode::PreferParseable : LoadMode);

// Ask the impl to find us a module that we can load or give us an error
// telling us that we couldn't load it.
Expand Down
6 changes: 4 additions & 2 deletions lib/Serialization/SerializedModuleLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,10 @@ Optional<bool> forEachModuleSearchPath(

// Defined out-of-line so that we can see ~ModuleFile.
SerializedModuleLoaderBase::SerializedModuleLoaderBase(
ASTContext &ctx, DependencyTracker *tracker, ModuleLoadingMode loadMode)
: ModuleLoader(tracker), Ctx(ctx), LoadMode(loadMode) {}
ASTContext &ctx, DependencyTracker *tracker, ModuleLoadingMode loadMode,
ArrayRef<std::string> PreferInterfaceForModules)
: ModuleLoader(tracker), Ctx(ctx), LoadMode(loadMode),
PreferInterfaceForModules(PreferInterfaceForModules) {}

SerializedModuleLoaderBase::~SerializedModuleLoaderBase() = default;
SerializedModuleLoader::~SerializedModuleLoader() = default;
Expand Down
12 changes: 12 additions & 0 deletions test/api-digester/Outputs/Cake-interface-vs-binary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
cake: Func FrozenKind.__derived_enum_equals(_:_:) has been removed
cake: Accessor GlobalLetChangedToVar.Get() is a new API without @available attribute
cake: Accessor GlobalVarChangedToLet.Get() is a new API without @available attribute
cake: Accessor GlobalVarChangedToLet.Modify() is a new API without @available attribute
cake: Accessor GlobalVarChangedToLet.Set() is a new API without @available attribute
cake: Class C4 is now with @objc
cake: Enum FrozenKind is now with @frozen
cake: Enum IceKind is now with @frozen
cake: Func FrozenKind.==(_:_:) is a new API without @available attribute
cake: Var C1.CIIns1 is no longer a stored property
cake: Var C1.CIIns2 is no longer a stored property
cake: Var RemoveSetters.Value is no longer a stored property
23 changes: 23 additions & 0 deletions test/api-digester/compare-dump-interface-vs-binary.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// RUN: %empty-directory(%t.mod1)
// RUN: %empty-directory(%t.mod2)
// RUN: %empty-directory(%t.sdk)
// RUN: %empty-directory(%t.module-cache)

// Generate .swiftinterface file for module cake
// RUN: %target-swift-frontend -typecheck -emit-parseable-module-interface-path %t.mod1/cake.swiftinterface %S/Inputs/cake_baseline/cake.swift -I %S/Inputs/APINotesLeft %clang-importer-sdk-nosource -parse-as-library -enable-library-evolution -disable-objc-attr-requires-foundation-module -module-cache-path %t.module-cache

// Generate .swiftmodule file for module cake
// RUN: %target-swift-frontend -emit-module -o %t.mod1/cake.swiftmodule %S/Inputs/cake_baseline/cake.swift -I %S/Inputs/APINotesLeft %clang-importer-sdk-nosource -parse-as-library -disable-objc-attr-requires-foundation-module -module-cache-path %t.module-cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. You forgot -enable-library-evolution here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the other changes should not happen, and if they do that's a problem.


// Dump Json file for cake ABI via .swiftmodule file
// RUN: %api-digester -dump-sdk -module cake -o - -module-cache-path %t.module-cache %clang-importer-sdk-nosource -I %t.mod1 -I %S/Inputs/APINotesLeft -abi > %t.dump1.json

// Dump Json file for cake ABI via .swiftinteface file
// RUN: %api-digester -dump-sdk -module cake -o - -module-cache-path %t.module-cache %clang-importer-sdk-nosource -I %t.mod1 -I %S/Inputs/APINotesLeft -abi -use-interface-for-module cake > %t.dump2.json

// Compare two Json files and we should see some differences.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we seeing differences here? It looks like you're building the swiftmodule and the swiftinterface from the same build settings, so they should have identical public contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice here we are comparing two Json files: the first Json file is a dump from loading the interface; the second Json file is a dump from loading the binary format generated directly from the source code. The difference is due to that source-generated swiftmodule files can diverge from the swiftmodule files generated from interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they do, that's a bug. How are these differences coming up?

// RUN: %api-digester -diagnose-sdk -print-module --input-paths %t.dump1.json -input-paths %t.dump2.json -abi -o %t.result

// RUN: %clang -E -P -x c %S/Outputs/Cake-interface-vs-binary.txt -o - | sed '/^\s*$/d' > %t.expected
// RUN: %clang -E -P -x c %t.result -o - | sed '/^\s*$/d' > %t.result.tmp
// RUN: diff -u %t.expected %t.result.tmp
8 changes: 8 additions & 0 deletions tools/swift-api-digester/swift-api-digester.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ static llvm::cl::opt<bool>
Migrator("migrator",
llvm::cl::desc("Dump Json suitable for generating migration script"),
llvm::cl::cat(Category));

static llvm::cl::list<std::string>
PreferInterfaceForModules("use-interface-for-module", llvm::cl::ZeroOrMore,
llvm::cl::desc("Prefer loading these modules via interface"),
llvm::cl::cat(Category));
} // namespace options

namespace {
Expand Down Expand Up @@ -2431,6 +2436,9 @@ static int prepareForDump(const char *Main,
for (auto M : options::ModuleNames) {
Modules.insert(M);
}
for (auto M: options::PreferInterfaceForModules) {
InitInvok.getFrontendOptions().PreferInterfaceForModules.push_back(M);
}
if (Modules.empty()) {
llvm::errs() << "Need to specify -include-all or -module <name>\n";
exit(1);
Expand Down