Skip to content

Implement preliminary support for deserializing cc1 flags in Swift mo… #9088

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.

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 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 38 additions & 2 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1573,7 +1573,21 @@ bool ShouldUnique(StringRef arg) {

// static
void SwiftASTContext::AddExtraClangArgs(const std::vector<std::string> &source,
std::vector<std::string> &dest) {
std::vector<std::string> &dest,
bool cc1) {
// FIXME: Support for cc1 flags isn't complete. The uniquing

Choose a reason for hiding this comment

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

The flags from a normal build (that is planned by swift dependency scanner), this cc1 list is absolutely deterministic and have a stable ordering already but probably lldb should not rely on this fact.

What really should happen is for lldb to create a clang::CompilerInvocation and parse the args then it can be systematically collect/modify the flag values. I am ok to leave that for future improvement

Copy link
Author

Choose a reason for hiding this comment

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

What really should happen is for lldb to create a clang::CompilerInvocation and parse the args then it can be systematically collect/modify the flag values. I am ok to leave that for future improvement

That might work, and if it does that would be a better implementation. It's possible that some modifications have to happen before parsing though, because the unmodified arguments wouldn't pass validation.

Choose a reason for hiding this comment

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

Oh, you mean some old cc1 args that are no longer valid for current clang version? That would be tricky (even though we expect a version matching lldb for debugging swift).

Copy link
Author

Choose a reason for hiding this comment

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

That case is hopeless for explicit modules anyway (the pcms would be incompatible), and I wouldn't expect this mode to be used for anything but explicit modules.

Copy link
Author

Choose a reason for hiding this comment

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

That might work, and if it does that would be a better implementation. It's possible that some modifications have to happen before parsing though, because the unmodified arguments wouldn't pass validation.

I was thinking about a hypothetical scenario of the driver complaining if a path doesn't exist (which it probably doesn't do)

// algortihm below does not work for cc1 flags. Since cc1 flags are
// not stable it's not feasible to keep a list of all multi-arg
// flags, for example. It also makes it difficult to correctly
// identify where workng directories and path remappings should
// applied. For all these reasons, using cc1 flags for anything but
// a local build with explicit modules and precise compiler
// invocations isn't supported yet.
if (cc1) {
dest.insert(dest.end(), source.begin(), source.end());
return;
}

llvm::StringSet<> unique_flags;
for (auto &arg : dest)
unique_flags.insert(arg);
Expand Down Expand Up @@ -1751,11 +1765,27 @@ static void applyOverrideOptions(std::vector<std::string> &args,
void SwiftASTContext::AddExtraClangArgs(
const std::vector<std::string> &ExtraArgs, StringRef overrideOpts) {
swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
AddExtraClangArgs(ExtraArgs, importer_options.ExtraArgs);

// Detect cc1 flags. When DirectClangCC1ModuleBuild is on then the
// clang arguments in the serialized invocation are clang cc1 flags,
// which are very specific to one compiler version and cannot
// be merged with driver options.
bool fresh_invocation = importer_options.ExtraArgs.empty();
if (fresh_invocation && !ExtraArgs.empty() && ExtraArgs.front() == "-cc1")
importer_options.DirectClangCC1ModuleBuild = true;

Choose a reason for hiding this comment

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

should DirectClangCC1ModuleBuild always be set to try when ExtraArgs.front() == "-cc1"?

in other words, should the mixed mode treat extra args as frontend flags when -cc1 is first?

Copy link
Author

Choose a reason for hiding this comment

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

I think such a case is hopeless anyway. I'm planning to clean this up on the rebranch branch, where I'm going to rip out support for non-precise compiler invocations, which should allow us to simplify all this code quite a bit.

if (!importer_options.DirectClangCC1ModuleBuild && !ExtraArgs.empty() &&
ExtraArgs.front() == "-cc1")
AddDiagnostic(
eSeverityWarning,
"Mixing and matching of driver and cc1 Clang options detected");

AddExtraClangArgs(ExtraArgs, importer_options.ExtraArgs,
importer_options.DirectClangCC1ModuleBuild);
applyOverrideOptions(importer_options.ExtraArgs, overrideOpts);
if (HasNonexistentExplicitModule(importer_options.ExtraArgs))
RemoveExplicitModules(importer_options.ExtraArgs);

// Detect explicitly-built modules.
m_has_explicit_modules =
llvm::any_of(importer_options.ExtraArgs, [](const std::string &arg) {
return StringRef(arg).starts_with("-fmodule-file=");
Expand Down Expand Up @@ -1864,6 +1894,11 @@ void SwiftASTContext::FilterClangImporterOptions(
arg_sr == "-fno-implicit-module-maps")
continue;

// This is not a cc1 option.
if (arg_sr.starts_with("--target=") && ctx &&
ctx->GetClangImporterOptions().DirectClangCC1ModuleBuild)
continue;

// The VFS options turn into fatal errors when the referenced file
// is not found. Since the Xcode build system tends to create a
// lot of VFS overlays by default, stat them and emit a warning if
Expand All @@ -1889,6 +1924,7 @@ void SwiftASTContext::FilterClangImporterOptions(
// Keep it.
extra_args.push_back(ivfs_arg);
}

extra_args.push_back(std::move(arg));
}
}
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ class SwiftASTContext : public TypeSystemSwift {
void AddExtraClangArgs(const std::vector<std::string> &ExtraArgs,
llvm::StringRef overrideOpts = "");
static void AddExtraClangArgs(const std::vector<std::string>& source,
std::vector<std::string>& dest);
std::vector<std::string>& dest, bool cc1);
static std::string GetPluginServer(llvm::StringRef plugin_library_path);
/// Removes nonexisting VFS overlay options.
static void FilterClangImporterOptions(std::vector<std::string> &extra_args,
Expand Down
5 changes: 5 additions & 0 deletions lldb/test/API/lang/swift/clangimporter/explicit_cc1/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
SWIFT_SOURCES := main.swift
SWIFT_ENABLE_EXPLICIT_MODULES := YES
SWIFTFLAGS_EXTRAS = -I$(SRCDIR) -Xfrontend -experimental-clang-importer-direct-cc1-scan

include Makefile.rules
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import lldb
from lldbsuite.test.lldbtest import *
from lldbsuite.test.decorators import *
import lldbsuite.test.lldbutil as lldbutil
import unittest2

class TestSwiftClangImporterExplicitCC1(TestBase):

NO_DEBUG_INFO_TESTCASE = True

# Don't run ClangImporter tests if Clangimporter is disabled.
@skipIf(setting=('symbols.use-swift-clangimporter', 'false'))
@skipIf(setting=('symbols.swift-precise-compiler-invocation', 'false'))
@skipIf(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'true'), bugnumber='rdar://128094135')
@skipUnlessDarwin
@swiftTest
def test(self):
"""
Test flipping on/off implicit modules.
"""
self.build()
lldbutil.run_to_source_breakpoint(self, "break here",
lldb.SBFileSpec('main.swift'))
log = self.getBuildArtifact("types.log")
self.expect('log enable lldb types -f "%s"' % log)
self.expect("expression obj", DATA_TYPES_DISPLAYED_CORRECTLY,
substrs=["b ="])
self.filecheck('platform shell cat "%s"' % log, __file__)
# CHECK: SwiftASTContextForExpressions(module: "a", cu: "main.swift")::LogConfiguration() -- -cc1
5 changes: 5 additions & 0 deletions lldb/test/API/lang/swift/clangimporter/explicit_cc1/a.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#include "b.h"

struct A {
struct B b;
};
1 change: 1 addition & 0 deletions lldb/test/API/lang/swift/clangimporter/explicit_cc1/b.h
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
struct B {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import ClangA
let obj = A()
print("break here \(obj)")
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module ClangA {
header "a.h"
}

module ClangB {
header "b.h"
}
6 changes: 3 additions & 3 deletions lldb/unittests/Symbol/TestSwiftASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -234,15 +234,15 @@ const std::vector<std::string> uniqued_flags = {
TEST_F(ClangArgs, UniquingCollisionWithExistingFlags) {
const std::vector<std::string> source = duplicated_flags;
std::vector<std::string> dest = uniqued_flags;
SwiftASTContext::AddExtraClangArgs(source, dest);
SwiftASTContext::AddExtraClangArgs(source, dest, false);

EXPECT_EQ(dest, uniqued_flags);
}

TEST_F(ClangArgs, UniquingCollisionWithAddedFlags) {
const std::vector<std::string> source = duplicated_flags;
std::vector<std::string> dest;
SwiftASTContext::AddExtraClangArgs(source, dest);
SwiftASTContext::AddExtraClangArgs(source, dest, false);

EXPECT_EQ(dest, uniqued_flags);
}
Expand All @@ -251,7 +251,7 @@ TEST_F(ClangArgs, DoubleDash) {
// -v with all currently ignored arguments following.
const std::vector<std::string> source{"-v", "--", "-Werror", ""};
std::vector<std::string> dest;
SwiftASTContext::AddExtraClangArgs(source, dest);
SwiftASTContext::AddExtraClangArgs(source, dest, false);

// Check that all ignored arguments got removed.
EXPECT_EQ(dest, std::vector<std::string>({"-v"}));
Expand Down