From c0ffd2369bd456e868387b2e6cabaa8a35a5eb1d Mon Sep 17 00:00:00 2001 From: ykiko Date: Mon, 12 Jan 2026 00:21:35 +0800 Subject: [PATCH] refactor: unify the `CompilationUnitRef` usage (#346) --- include/Compiler/CompilationUnit.h | 11 +- src/Compiler/Compilation.cpp | 236 +++++++++++++++-------------- src/Compiler/CompilationUnit.cpp | 6 +- src/Compiler/Diagnostic.cpp | 4 +- src/Compiler/Directive.cpp | 42 +++-- src/Compiler/Implement.h | 49 +++--- 6 files changed, 176 insertions(+), 172 deletions(-) diff --git a/include/Compiler/CompilationUnit.h b/include/Compiler/CompilationUnit.h index 8187c624..3469c655 100644 --- a/include/Compiler/CompilationUnit.h +++ b/include/Compiler/CompilationUnit.h @@ -83,6 +83,8 @@ public: /// will be invalid file id. If the the content of the file doesn't have /// `#pragma once` or guard macro, each inclusion of the file will generate /// a new file id, return the first one. + auto file_id(clang::FileEntryRef file) -> clang::FileID; + auto file_id(llvm::StringRef file) -> clang::FileID; /// If the location represents file location, it is composed of a file id @@ -229,7 +231,7 @@ protected: /// All AST related information needed for language server. class CompilationUnit : public CompilationUnitRef { public: - explicit CompilationUnit(Self* impl) : CompilationUnitRef(impl) {} + explicit CompilationUnit(Self* self) : CompilationUnitRef(self) {} CompilationUnit(const CompilationUnit&) = delete; @@ -237,6 +239,13 @@ public: other.self = nullptr; } + CompilationUnit& operator=(const CompilationUnit&) = delete; + + CompilationUnit& operator=(CompilationUnit&& other) { + std::swap(self, other.self); + return *this; + } + ~CompilationUnit(); }; diff --git a/src/Compiler/Compilation.cpp b/src/Compiler/Compilation.cpp index 5c28bcf4..9981389b 100644 --- a/src/Compiler/Compilation.cpp +++ b/src/Compiler/Compilation.cpp @@ -13,97 +13,35 @@ namespace clice { -namespace { - -/// A wrapper ast consumer, so that we can cancel the ast parse -class ProxyASTConsumer final : public clang::MultiplexConsumer { -public: - ProxyASTConsumer(std::unique_ptr consumer, - clang::CompilerInstance& instance, - std::vector* top_level_decls, - std::shared_ptr stop) : - clang::MultiplexConsumer(std::move(consumer)), instance(instance), - src_mgr(instance.getSourceManager()), top_level_decls(top_level_decls), stop(stop) {} - - void collect_decl(clang::Decl* decl) { - if(!(ast::is_inside_main_file(decl->getLocation(), src_mgr))) { - return; - } - - if(const clang::NamedDecl* named_decl = dyn_cast(decl)) { - if(ast::is_implicit_template_instantiation(named_decl)) { - return; - } - } - - top_level_decls->push_back(decl); +CompilationUnitRef::Self::~Self() { + if(action) { + // We already notified the pp of end-of-file earlier, so detach it first. + // We must keep it alive until after EndSourceFile(), Sema relies on this. + std::shared_ptr pp = instance->getPreprocessorPtr(); + // Detach so we don't send EOF again + instance->setPreprocessor(nullptr); + action->EndSourceFile(); } +} - auto HandleTopLevelDecl(clang::DeclGroupRef group) -> bool final { - if(top_level_decls) { - if(group.isDeclGroup()) { - for(auto decl: group) { - collect_decl(decl); - } - } else { - collect_decl(group.getSingleDecl()); - } - } - - /// TODO: check atomic variable after the parse of each declaration - /// may result in performance issue, benchmark in the future. - if(stop && stop->load()) { - return false; - } - - return clang::MultiplexConsumer::HandleTopLevelDecl(group); - } - -private: - clang::CompilerInstance& instance; - clang::SourceManager& src_mgr; - - /// Non-nullptr if we need collect the top level declarations. - std::vector* top_level_decls; - - std::shared_ptr stop; -}; - -class ProxyAction final : public clang::WrapperFrontendAction { -public: - ProxyAction(std::unique_ptr action, - std::vector* top_level_decls, - std::shared_ptr stop) : - clang::WrapperFrontendAction(std::move(action)), top_level_decls(top_level_decls), - stop(std::move(stop)) {} - - auto CreateASTConsumer(clang::CompilerInstance& instance, llvm::StringRef file) - -> std::unique_ptr final { - return std::make_unique( - WrapperFrontendAction::CreateASTConsumer(instance, file), - instance, - top_level_decls, - std::move(stop)); - } - - /// Make this public. - using clang::WrapperFrontendAction::EndSourceFile; - -private: - std::vector* top_level_decls; - std::shared_ptr stop; -}; - -/// create a `clang::CompilerInvocation` for compilation, it set and reset -/// all necessary arguments and flags for clice compilation. -auto create_invocation(CompilationUnitRef::Self& self, - CompilationParams& params, - llvm::IntrusiveRefCntPtr& diagnostic_engine) - -> std::unique_ptr { +std::unique_ptr + CompilationUnitRef::Self::create_invocation(this Self& self, + CompilationParams& params, + clang::DiagnosticConsumer* consumer) { if(params.arguments.empty()) { LOG_ERROR_RET(nullptr, "Fail to create invocation: empty argument list from database"); } + /// Temporary diagnostic engine, only used for command line parsing. + /// For compilation, we need to create a new diagnostic engine. See also + /// https://github.com/llvm/llvm-project/pull/139584#issuecomment-2920704282. + clang::DiagnosticOptions options; + llvm::IntrusiveRefCntPtr diagnostic_engine = + clang::CompilerInstance::createDiagnostics(*params.vfs, options, consumer, false); + if(!diagnostic_engine) { + LOG_ERROR_RET(nullptr, "Fail to create diagnostics engine"); + } + std::unique_ptr invocation; /// Arguments from compilation database are already cc1 @@ -172,7 +110,7 @@ auto create_invocation(CompilationUnitRef::Self& self, front_opts.StatsFile = ""; front_opts.TimeTracePath = ""; front_opts.TimeTraceVerbose = false; - front_opts.TimeTraceGranularity = false; + front_opts.TimeTraceGranularity = 0; front_opts.PrintSupportedCPUs = false; front_opts.PrintEnabledExtensions = false; front_opts.PrintSupportedExtensions = false; @@ -205,26 +143,99 @@ auto create_invocation(CompilationUnitRef::Self& self, return invocation; } -CompilationStatus run_clang(CompilationUnitRef::Self& self, - CompilationParams& params, - std::unique_ptr action, - llvm::function_ref before_execute) { - std::unique_ptr diagnostic_consumer = create_diagnostic(&self); +void CompilationUnitRef::Self::configure_tidy(tidy::TidyParams tidy_params) { + checker = tidy::configure(*instance, tidy_params); +} - /// Temporary diagnostic engine, only used for command line parsing. - /// For compilation, we need to create a new diagnostic engine. See also - /// https://github.com/llvm/llvm-project/pull/139584#issuecomment-2920704282. - clang::DiagnosticOptions options; - llvm::IntrusiveRefCntPtr diagnostic_engine = - clang::CompilerInstance::createDiagnostics(*params.vfs, - options, - diagnostic_consumer.get(), - false); - if(!diagnostic_engine) { - return CompilationStatus::SetupFail; +void CompilationUnitRef::Self::run_tidy() { + if(checker) { + // AST traversals should exclude the preamble, to avoid performance cliffs. + // TODO: is it okay to affect the unit-level traversal scope here? + auto& Ctx = instance->getASTContext(); + Ctx.setTraversalScope(top_level_decls); + checker->finder.matchAST(Ctx); + + /// XXX: This is messy: clang-tidy checks flush some diagnostics at EOF. + /// However Action->EndSourceFile() would destroy the ASTContext! + /// So just inform the preprocessor of EOF, while keeping everything alive. + instance->getPreprocessor().EndSourceFile(); + } +} + +namespace { + +/// A wrapper ast consumer, so that we can cancel the ast parse +class ProxyASTConsumer final : public clang::MultiplexConsumer { +public: + ProxyASTConsumer(std::unique_ptr consumer, CompilationUnitRef unit) : + clang::MultiplexConsumer(std::move(consumer)), unit(unit) {} + + void collect_decl(clang::Decl* decl) { + if(unit.file_id(unit.expansion_location(decl->getLocation())) != unit.interested_file()) { + return; + } + + if(const clang::NamedDecl* named_decl = dyn_cast(decl)) { + if(ast::is_implicit_template_instantiation(named_decl)) { + return; + } + } + + unit->top_level_decls.push_back(decl); } - std::unique_ptr invocation = create_invocation(self, params, diagnostic_engine); + auto HandleTopLevelDecl(clang::DeclGroupRef group) -> bool final { + if(unit->kind == CompilationKind::Content) { + if(group.isDeclGroup()) { + for(auto decl: group) { + collect_decl(decl); + } + } else { + collect_decl(group.getSingleDecl()); + } + } + + /// TODO: check atomic variable after the parse of each declaration + /// may result in performance issue, benchmark in the future. + if(unit->stop && unit->stop->load()) { + return false; + } + + return clang::MultiplexConsumer::HandleTopLevelDecl(group); + } + +private: + CompilationUnitRef unit; +}; + +class ProxyAction final : public clang::WrapperFrontendAction { +public: + ProxyAction(std::unique_ptr action, CompilationUnitRef unit) : + clang::WrapperFrontendAction(std::move(action)), unit(unit) {} + + auto CreateASTConsumer(clang::CompilerInstance& instance, llvm::StringRef file) + -> std::unique_ptr final { + return std::make_unique( + WrapperFrontendAction::CreateASTConsumer(instance, file), + unit); + } + + /// Make this public. + using clang::WrapperFrontendAction::EndSourceFile; + +private: + CompilationUnitRef unit; +}; + +} // namespace + +CompilationStatus CompilationUnitRef::Self::run_clang( + this Self& self, + CompilationParams& params, + std::unique_ptr action, + llvm::function_ref before_execute) { + std::unique_ptr diagnostic_consumer = self.create_diagnostic(); + std::unique_ptr invocation = self.create_invocation(params, diagnostic_consumer.get()); if(!invocation) { return CompilationStatus::SetupFail; } @@ -247,14 +258,12 @@ CompilationStatus run_clang(CompilationUnitRef::Self& self, before_execute(instance); } - self.action = std::make_unique( - std::move(action), - /// We only collect top level declarations for parse main file. - (params.clang_tidy || params.kind == CompilationKind::Content) ? &self.top_level_decls - : nullptr, - params.stop); + self.action = std::make_unique(std::move(action), &self); if(!self.action->BeginSourceFile(instance, instance.getFrontendOpts().Inputs[0])) { + /// If the action is not empty, we will call `EndSourceFile` at the destructor of `Self`. + /// But if we fail to `BeginSourceFile` we don't need to call `EndSourceFile`. So just + /// reset it. self.action.reset(); return CompilationStatus::SetupFail; } @@ -294,9 +303,7 @@ CompilationStatus run_clang(CompilationUnitRef::Self& self, /// Check whether the compilation is canceled, if so we think /// it is an error. - if(params.stop && params.stop->load()) { - self.action->EndSourceFile(); - self.action.reset(); + if(self.stop && self.stop->load()) { return CompilationStatus::Cancelled; } @@ -319,12 +326,13 @@ CompilationUnit run_clang(CompilationParams& params, llvm::function_ref after_execute = {}) { auto self = new CompilationUnitRef::Self(); self->kind = params.kind; + self->stop = std::move(params.stop); using namespace std::chrono; self->build_at = duration_cast(system_clock::now().time_since_epoch()); auto build_start = steady_clock::now().time_since_epoch(); - self->status = run_clang(*self, params, std::move(action), before_execute); + self->status = self->run_clang(params, std::move(action), before_execute); auto build_end = steady_clock::now().time_since_epoch(); self->build_duration = duration_cast(build_end - build_start); @@ -336,8 +344,6 @@ CompilationUnit run_clang(CompilationParams& params, return CompilationUnit(self); } -} // namespace - CompilationUnit preprocess(CompilationParams& params) { return run_clang(params, std::make_unique()); } diff --git a/src/Compiler/CompilationUnit.cpp b/src/Compiler/CompilationUnit.cpp index ff95d57c..d1a8ed6a 100644 --- a/src/Compiler/CompilationUnit.cpp +++ b/src/Compiler/CompilationUnit.cpp @@ -12,10 +12,14 @@ CompilationStatus CompilationUnitRef::status() { return self->status; } +auto CompilationUnitRef::file_id(clang::FileEntryRef entry) -> clang::FileID { + return self->SM().translateFile(entry); +} + auto CompilationUnitRef::file_id(llvm::StringRef file) -> clang::FileID { auto entry = self->SM().getFileManager().getFileRef(file); if(entry) { - return self->SM().translateFile(*entry); + return file_id(*entry); } return clang::FileID(); diff --git a/src/Compiler/Diagnostic.cpp b/src/Compiler/Diagnostic.cpp index a091cc07..340cafcc 100644 --- a/src/Compiler/Diagnostic.cpp +++ b/src/Compiler/Diagnostic.cpp @@ -249,8 +249,8 @@ private: CompilationUnitRef unit; }; -std::unique_ptr create_diagnostic(CompilationUnitRef unit) { - return std::make_unique(unit); +std::unique_ptr CompilationUnitRef::Self::create_diagnostic() { + return std::make_unique(this); } } // namespace clice diff --git a/src/Compiler/Directive.cpp b/src/Compiler/Directive.cpp index d5c01fab..947c6df0 100644 --- a/src/Compiler/Directive.cpp +++ b/src/Compiler/Directive.cpp @@ -12,16 +12,14 @@ namespace { class DirectiveCollector : public clang::PPCallbacks { public: - DirectiveCollector(clang::Preprocessor& pp, - llvm::DenseMap& directives) : - pp(pp), sm(pp.getSourceManager()), directives(directives) {} + DirectiveCollector(CompilationUnitRef unit) : unit(unit) {} private: void add_condition(clang::SourceLocation location, Condition::BranchKind kind, Condition::ConditionValue value, clang::SourceRange cond_range) { - auto& directive = directives[sm.getFileID(location)]; + auto& directive = unit->directives[unit.file_id(location)]; directive.conditions.emplace_back(kind, value, location, cond_range); } @@ -54,12 +52,11 @@ private: return; } - if(sm.isWrittenInBuiltinFile(loc) || sm.isWrittenInCommandLineFile(loc) || - sm.isWrittenInScratchSpace(loc)) { + if(unit.is_builtin_file(unit.file_id(loc))) { return; } - auto& directive = directives[sm.getFileID(loc)]; + auto& directive = unit->directives[unit.file_id(loc)]; directive.macros.emplace_back(MacroRef{def, kind, loc}); } @@ -79,11 +76,11 @@ public: const clang::Module*, bool, clang::SrcMgr::CharacteristicKind) override { - prev_fid = sm.getFileID(hash_loc); + prev_fid = unit.file_id(hash_loc); /// An `IncludeDirective` call is always followed by either a `LexedFileChanged` /// or a `FileSkipped`. so we cannot get the file id of included file here. - directives[prev_fid].includes.emplace_back(Include{ + unit->directives[prev_fid].includes.emplace_back(Include{ .fid = {}, .location = include_tok.getLocation(), .filename_range = filename_range.getAsRange(), @@ -99,7 +96,7 @@ public: this->prev_fid.isValid() && prev_fid == this->prev_fid) { /// Once the file has changed, it means that the last include is not skipped. /// Therefore, we initialize its file id with the current file id. - auto& include = directives[prev_fid].includes.back(); + auto& include = unit->directives[prev_fid].includes.back(); include.skipped = false; include.fid = curr_fid; } @@ -111,20 +108,20 @@ public: if(prev_fid.isValid()) { /// File with guard will have only one file id in `SourceManager`, use /// `translateFile` to find it. - auto& include = directives[prev_fid].includes.back(); + auto& include = unit->directives[prev_fid].includes.back(); include.skipped = true; /// Get the FileID for the given file. If the source file is included multiple /// times, the FileID will be the first inclusion. - include.fid = sm.translateFile(file); + include.fid = unit.file_id(file); } } void moduleImport(clang::SourceLocation import_location, clang::ModuleIdPath names, const clang::Module*) override { - auto fid = sm.getFileID(sm.getExpansionLoc(import_location)); - auto& import = directives[fid].imports.emplace_back(); + auto fid = unit.file_id(unit.expansion_location(import_location)); + auto& import = unit->directives[fid].imports.emplace_back(); import.location = import_location; for(auto name: names) { import.name += name.getIdentifierInfo()->getName(); @@ -139,10 +136,10 @@ public: clang::SrcMgr::CharacteristicKind) override { clang::FileID fid; if(file) { - fid = sm.translateFile(*file); + fid = unit.file_id(*file); } - directives[sm.getFileID(location)].has_includes.emplace_back(fid, location); + unit->directives[unit.file_id(location)].has_includes.emplace_back(fid, location); } void PragmaDirective(clang::SourceLocation loc, @@ -151,16 +148,16 @@ public: if(introducer != clang::PragmaIntroducerKind::PIK_HashPragma) return; - clang::FileID fid = sm.getFileID(loc); + clang::FileID fid = unit.file_id(loc); - llvm::StringRef text_to_end = sm.getBufferData(fid).substr(sm.getFileOffset(loc)); + llvm::StringRef text_to_end = unit.file_content(fid).substr(unit.file_offset(loc)); llvm::StringRef that_line = text_to_end.take_until([](char ch) { return ch == '\n'; }); Pragma::Kind kind = that_line.contains("endregion") ? Pragma::EndRegion : that_line.contains("region") ? Pragma::Region : Pragma::Other; - auto& directive = directives[fid]; + auto& directive = unit->directives[fid]; directive.pragmas.emplace_back(Pragma{ that_line, kind, @@ -256,17 +253,14 @@ public: private: clang::FileID prev_fid; - clang::Preprocessor& pp; - clang::SourceManager& sm; - llvm::DenseMap& directives; + CompilationUnitRef unit; llvm::DenseMap macro_cache; }; } // namespace void CompilationUnitRef::Self::collect_directives() { - auto& pp = instance->getPreprocessor(); - pp.addPPCallbacks(std::make_unique(pp, directives)); + instance->getPreprocessor().addPPCallbacks(std::make_unique(this)); } } // namespace clice diff --git a/src/Compiler/Implement.h b/src/Compiler/Implement.h index 399386c2..678efb4d 100644 --- a/src/Compiler/Implement.h +++ b/src/Compiler/Implement.h @@ -57,6 +57,8 @@ struct CompilationUnitRef::Self { CompilationStatus status; + std::shared_ptr stop; + llvm::StringMap> remapped_buffers; /// The frontend action used to build the unit. @@ -98,40 +100,29 @@ struct CompilationUnitRef::Self { return instance->getSourceManager(); } +public: + ~Self(); + + std::unique_ptr create_diagnostic(); + + /// create a `clang::CompilerInvocation` for compilation, it set and reset + /// all necessary arguments and flags for clice compilation. + std::unique_ptr + create_invocation(this Self& self, + CompilationParams& params, + clang::DiagnosticConsumer* consumer); + void collect_directives(); - void configure_tidy(tidy::TidyParams tidy_params) { - checker = tidy::configure(*instance, tidy_params); - } + void configure_tidy(tidy::TidyParams tidy_params); // Must be called before EndSourceFile because the ast context can be destroyed later. - void run_tidy() { - if(checker) { - // AST traversals should exclude the preamble, to avoid performance cliffs. - // TODO: is it okay to affect the unit-level traversal scope here? - auto& Ctx = instance->getASTContext(); - Ctx.setTraversalScope(top_level_decls); - checker->finder.matchAST(Ctx); + void run_tidy(); - /// XXX: This is messy: clang-tidy checks flush some diagnostics at EOF. - /// However Action->EndSourceFile() would destroy the ASTContext! - /// So just inform the preprocessor of EOF, while keeping everything alive. - instance->getPreprocessor().EndSourceFile(); - } - } - - ~Self() { - if(action) { - // We already notified the pp of end-of-file earlier, so detach it first. - // We must keep it alive until after EndSourceFile(), Sema relies on this. - std::shared_ptr pp = instance->getPreprocessorPtr(); - // Detach so we don't send EOF again - instance->setPreprocessor(nullptr); - action->EndSourceFile(); - } - } + CompilationStatus run_clang(this Self& self, + CompilationParams& params, + std::unique_ptr action, + llvm::function_ref before_execute); }; -std::unique_ptr create_diagnostic(CompilationUnitRef unit); - } // namespace clice