From 02e11b7ee92d78b7451a3e70e79c147da1a33524 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Mon, 11 Jul 2022 13:59:38 -0700 Subject: [PATCH 1/7] Docs: Add links from query help to query pack changelog for each language --- docs/codeql/query-help/cpp.rst | 4 +++- docs/codeql/query-help/csharp.rst | 4 +++- docs/codeql/query-help/go.rst | 4 +++- docs/codeql/query-help/java.rst | 4 +++- docs/codeql/query-help/javascript.rst | 4 +++- docs/codeql/query-help/python.rst | 4 +++- docs/codeql/query-help/ruby.rst | 4 +++- 7 files changed, 21 insertions(+), 7 deletions(-) diff --git a/docs/codeql/query-help/cpp.rst b/docs/codeql/query-help/cpp.rst index 7c3cbe304d7..12d2dfbf53e 100644 --- a/docs/codeql/query-help/cpp.rst +++ b/docs/codeql/query-help/cpp.rst @@ -3,7 +3,9 @@ CodeQL query help for C and C++ .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/cpp-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-cpp.rst \ No newline at end of file diff --git a/docs/codeql/query-help/csharp.rst b/docs/codeql/query-help/csharp.rst index 9c5c6351ce3..5d7f732a588 100644 --- a/docs/codeql/query-help/csharp.rst +++ b/docs/codeql/query-help/csharp.rst @@ -3,6 +3,8 @@ CodeQL query help for C# .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/csharp-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-csharp.rst \ No newline at end of file diff --git a/docs/codeql/query-help/go.rst b/docs/codeql/query-help/go.rst index 9e3050f74d0..bcd4aba9b51 100644 --- a/docs/codeql/query-help/go.rst +++ b/docs/codeql/query-help/go.rst @@ -3,6 +3,8 @@ CodeQL query help for Go .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/go-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-go.rst diff --git a/docs/codeql/query-help/java.rst b/docs/codeql/query-help/java.rst index 8af2ee52890..4876546d2dc 100644 --- a/docs/codeql/query-help/java.rst +++ b/docs/codeql/query-help/java.rst @@ -3,6 +3,8 @@ CodeQL query help for Java .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/java-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-java.rst diff --git a/docs/codeql/query-help/javascript.rst b/docs/codeql/query-help/javascript.rst index d7cf6797852..ae85de99f7b 100644 --- a/docs/codeql/query-help/javascript.rst +++ b/docs/codeql/query-help/javascript.rst @@ -3,6 +3,8 @@ CodeQL query help for JavaScript .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/javascript-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-javascript.rst \ No newline at end of file diff --git a/docs/codeql/query-help/python.rst b/docs/codeql/query-help/python.rst index da68c1caa9b..9d915d443f3 100644 --- a/docs/codeql/query-help/python.rst +++ b/docs/codeql/query-help/python.rst @@ -3,6 +3,8 @@ CodeQL query help for Python .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/python-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-python.rst \ No newline at end of file diff --git a/docs/codeql/query-help/ruby.rst b/docs/codeql/query-help/ruby.rst index 3ce1304ba76..e733aecaf79 100644 --- a/docs/codeql/query-help/ruby.rst +++ b/docs/codeql/query-help/ruby.rst @@ -3,6 +3,8 @@ CodeQL query help for Ruby .. include:: ../reusables/query-help-overview.rst -For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. +These queries are published in the CodeQL query pack ``codeql/ruby-queries`` (`changelog `__, `source `__). + +For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__. .. include:: toc-ruby.rst From f7dca4d70fd58016671a3a0dde5158fd3a716993 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 12 Jul 2022 17:31:02 +0200 Subject: [PATCH 2/7] Swift: trap output rework Firstly, this change reworks how inter-process races are resolved. Moreover some responsability reorganization has led to merging `TrapArena` and `TrapOutput` again into a `TrapDomain` class. A `TargetFile` class is introduced, that is successfully created only for the first process that starts processing a given trap output file. From then on `TargetFile` simply wraps around `<<` stream operations, dumping them to a temporary file. When `TargetFile::commit` is called, the temporary file is moved on to the actual target trap file. Processes that lose the race can now just ignore the unneeded extraction and go on, while previously all processes would carry out all extractions overwriting each other at the end. Some of the file system logic contained in `SwiftExtractor.cpp` has been moved to this class, and two TODOs are solved: * introducing a better inter process file collision avoidance strategy * better error handling for trap output operations: if unable to write to the trap file (or carry out other basic file operations), we just abort. The changes to `ExprVisitor` and `StmtVisitor` are due to wanting to hide the raw `TrapDomain::createLabel` from them, and bring more funcionality under the generic caching/dispatching mechanism. --- swift/codegen/schema.yml | 1 + swift/extractor/SwiftExtractor.cpp | 70 +++++------------- swift/extractor/infra/BUILD.bazel | 2 + swift/extractor/infra/SwiftDispatcher.h | 46 ++++++------ swift/extractor/infra/SwiftTagTraits.h | 2 + swift/extractor/infra/TargetFile.cpp | 73 +++++++++++++++++++ swift/extractor/infra/TargetFile.h | 40 ++++++++++ swift/extractor/trap/TrapArena.h | 23 ------ .../trap/{TrapOutput.h => TrapDomain.h} | 68 +++++++++++------ swift/extractor/visitors/ExprVisitor.cpp | 10 +-- swift/extractor/visitors/StmtVisitor.cpp | 34 ++++----- swift/extractor/visitors/StmtVisitor.h | 4 +- swift/extractor/visitors/SwiftVisitor.h | 7 +- .../codeql/swift/generated/expr/Argument.qll | 4 +- swift/ql/lib/swift.dbscheme | 6 +- .../DotSyntaxCallExpr_getArgument.expected | 4 +- 16 files changed, 238 insertions(+), 156 deletions(-) create mode 100644 swift/extractor/infra/TargetFile.cpp create mode 100644 swift/extractor/infra/TargetFile.h delete mode 100644 swift/extractor/trap/TrapArena.h rename swift/extractor/trap/{TrapOutput.h => TrapDomain.h} (56%) diff --git a/swift/codegen/schema.yml b/swift/codegen/schema.yml index 01bb0c2feba..1c76a1c7a65 100644 --- a/swift/codegen/schema.yml +++ b/swift/codegen/schema.yml @@ -319,6 +319,7 @@ AppliedPropertyWrapperExpr: _extends: Expr Argument: + _extends: Locatable label: string _children: expr: Expr diff --git a/swift/extractor/SwiftExtractor.cpp b/swift/extractor/SwiftExtractor.cpp index 44d2309cb2a..88ad2152473 100644 --- a/swift/extractor/SwiftExtractor.cpp +++ b/swift/extractor/SwiftExtractor.cpp @@ -16,8 +16,9 @@ #include #include "swift/extractor/trap/generated/TrapClasses.h" -#include "swift/extractor/trap/TrapOutput.h" +#include "swift/extractor/trap/TrapDomain.h" #include "swift/extractor/visitors/SwiftVisitor.h" +#include "swift/extractor/infra/TargetFile.h" using namespace codeql; @@ -52,7 +53,7 @@ static void archiveFile(const SwiftExtractorConfiguration& config, swift::Source } } -static std::string getTrapFilename(swift::ModuleDecl& module, swift::SourceFile* primaryFile) { +static std::string getFilename(swift::ModuleDecl& module, swift::SourceFile* primaryFile) { if (primaryFile) { return primaryFile->getFilename().str(); } @@ -80,56 +81,39 @@ static void extractDeclarations(const SwiftExtractorConfiguration& config, swift::CompilerInstance& compiler, swift::ModuleDecl& module, swift::SourceFile* primaryFile = nullptr) { + auto filename = getFilename(module, primaryFile); + // The extractor can be called several times from different processes with - // the same input file(s) - // We are using PID to avoid concurrent access - // TODO: find a more robust approach to avoid collisions? - auto name = getTrapFilename(module, primaryFile); - llvm::StringRef filename(name); - std::string tempTrapName = filename.str() + '.' + std::to_string(getpid()) + ".trap"; - llvm::SmallString tempTrapPath(config.getTempTrapDir()); - llvm::sys::path::append(tempTrapPath, tempTrapName); - - llvm::StringRef tempTrapParent = llvm::sys::path::parent_path(tempTrapPath); - if (std::error_code ec = llvm::sys::fs::create_directories(tempTrapParent)) { - std::cerr << "Cannot create temp trap directory '" << tempTrapParent.str() - << "': " << ec.message() << "\n"; + // the same input file(s). Using `TargetFile` the first process will win, and the following + // will just skip the work + TargetFile trapStream{filename + ".trap", config.trapDir, config.getTempTrapDir()}; + if (!trapStream.good()) { + // another process arrived first, nothing to do for us return; } - std::ofstream trapStream(tempTrapPath.str().str()); - if (!trapStream) { - std::error_code ec; - ec.assign(errno, std::generic_category()); - std::cerr << "Cannot create temp trap file '" << tempTrapPath.str().str() - << "': " << ec.message() << "\n"; - return; - } trapStream << "/* extractor-args:\n"; - for (auto opt : config.frontendOptions) { + for (const auto& opt : config.frontendOptions) { trapStream << " " << std::quoted(opt) << " \\\n"; } trapStream << "\n*/\n"; trapStream << "/* swift-frontend-args:\n"; - for (auto opt : config.patchedFrontendOptions) { + for (const auto& opt : config.patchedFrontendOptions) { trapStream << " " << std::quoted(opt) << " \\\n"; } trapStream << "\n*/\n"; - TrapOutput trap{trapStream}; - TrapArena arena{}; + TrapDomain trap{trapStream}; // TODO: move default location emission elsewhere, possibly in a separate global trap file - auto unknownFileLabel = arena.allocateLabel(); // the following cannot conflict with actual files as those have an absolute path starting with / - trap.assignKey(unknownFileLabel, "unknown"); + auto unknownFileLabel = trap.createLabel("unknown"); + auto unknownLocationLabel = trap.createLabel("unknown"); trap.emit(FilesTrap{unknownFileLabel}); - auto unknownLocationLabel = arena.allocateLabel(); - trap.assignKey(unknownLocationLabel, "unknown"); trap.emit(LocationsTrap{unknownLocationLabel, unknownFileLabel}); - SwiftVisitor visitor(compiler.getSourceMgr(), arena, trap, module, primaryFile); + SwiftVisitor visitor(compiler.getSourceMgr(), trap, module, primaryFile); auto topLevelDecls = getTopLevelDecls(module, primaryFile); for (auto decl : topLevelDecls) { visitor.extract(decl); @@ -142,28 +126,10 @@ static void extractDeclarations(const SwiftExtractorConfiguration& config, // fact that the file was extracted llvm::SmallString name(filename); llvm::sys::fs::make_absolute(name); - auto fileLabel = arena.allocateLabel(); - trap.assignKey(fileLabel, name.str().str()); + auto fileLabel = trap.createLabel(name.str().str()); trap.emit(FilesTrap{fileLabel, name.str().str()}); } - - // TODO: Pick a better name to avoid collisions - std::string trapName = filename.str() + ".trap"; - llvm::SmallString trapPath(config.trapDir); - llvm::sys::path::append(trapPath, trapName); - - llvm::StringRef trapParent = llvm::sys::path::parent_path(trapPath); - if (std::error_code ec = llvm::sys::fs::create_directories(trapParent)) { - std::cerr << "Cannot create trap directory '" << trapParent.str() << "': " << ec.message() - << "\n"; - return; - } - - // TODO: The last process wins. Should we do better than that? - if (std::error_code ec = llvm::sys::fs::rename(tempTrapPath, trapPath)) { - std::cerr << "Cannot rename temp trap file '" << tempTrapPath.str().str() << "' -> '" - << trapPath.str().str() << "': " << ec.message() << "\n"; - } + trapStream.commit(); } static std::unordered_set collectInputFilenames(swift::CompilerInstance& compiler) { diff --git a/swift/extractor/infra/BUILD.bazel b/swift/extractor/infra/BUILD.bazel index 33098eb76a4..115fb06b745 100644 --- a/swift/extractor/infra/BUILD.bazel +++ b/swift/extractor/infra/BUILD.bazel @@ -2,9 +2,11 @@ load("//swift:rules.bzl", "swift_cc_library") swift_cc_library( name = "infra", + srcs = glob(["*.cpp"]), hdrs = glob(["*.h"]), visibility = ["//swift:__subpackages__"], deps = [ "//swift/extractor/trap", + "//swift/tools/prebuilt:swift-llvm-support", ], ) diff --git a/swift/extractor/infra/SwiftDispatcher.h b/swift/extractor/infra/SwiftDispatcher.h index c7ca43a1614..a9ecb6996ff 100644 --- a/swift/extractor/infra/SwiftDispatcher.h +++ b/swift/extractor/infra/SwiftDispatcher.h @@ -4,9 +4,8 @@ #include #include -#include "swift/extractor/trap/TrapArena.h" #include "swift/extractor/trap/TrapLabelStore.h" -#include "swift/extractor/trap/TrapOutput.h" +#include "swift/extractor/trap/TrapDomain.h" #include "swift/extractor/infra/SwiftTagTraits.h" #include "swift/extractor/trap/generated/TrapClasses.h" @@ -22,12 +21,10 @@ class SwiftDispatcher { // all references and pointers passed as parameters to this constructor are supposed to outlive // the SwiftDispatcher SwiftDispatcher(const swift::SourceManager& sourceManager, - TrapArena& arena, - TrapOutput& trap, + TrapDomain& trap, swift::ModuleDecl& currentModule, swift::SourceFile* currentPrimarySourceFile = nullptr) : sourceManager{sourceManager}, - arena{arena}, trap{trap}, currentModule{currentModule}, currentPrimarySourceFile{currentPrimarySourceFile} {} @@ -77,6 +74,7 @@ class SwiftDispatcher { } waitingForNewLabel = e; visit(e); + // TODO when everything is moved to structured C++ classes, this should be moved to createEntry if (auto l = store.get(e)) { if constexpr (!std::is_base_of_v) { attachLocation(e, *l); @@ -95,13 +93,17 @@ class SwiftDispatcher { return fetchLabelFromUnion(node); } + TrapLabel fetchLabel(const swift::StmtConditionElement& element) { + return fetchLabel(&element); + } + // Due to the lazy emission approach, we must assign a label to a corresponding AST node before // it actually gets emitted to handle recursive cases such as recursive calls, or recursive type // declarations template TrapLabelOf assignNewLabel(E* e, Args&&... args) { assert(waitingForNewLabel == Store::Handle{e} && "assignNewLabel called on wrong entity"); - auto label = createLabel>(std::forward(args)...); + auto label = trap.createLabel>(std::forward(args)...); store.insert(e, label); waitingForNewLabel = std::monostate{}; return label; @@ -118,18 +120,13 @@ class SwiftDispatcher { return TrapClassOf{assignNewLabel(&e, std::forward(args)...)}; } - template - TrapLabel createLabel() { - auto ret = arena.allocateLabel(); - trap.assignStar(ret); - return ret; - } - - template - TrapLabel createLabel(Args&&... args) { - auto ret = arena.allocateLabel(); - trap.assignKey(ret, std::forward(args)...); - return ret; + // used to create a new entry for entities that should not be cached + // an example is swift::Argument, that are created on the fly and thus have no stable pointer + template >* = nullptr> + auto createUncachedEntry(const E& e, Args&&... args) { + auto label = trap.createLabel>(std::forward(args)...); + attachLocation(&e, label); + return TrapClassOf{label}; } template @@ -213,6 +210,7 @@ class SwiftDispatcher { using Store = TrapLabelStore(filepath); + auto fileLabel = trap.createLabel(filepath); // TODO: do not emit duplicate trap entries for Files trap.emit(FilesTrap{fileLabel, filepath}); auto [startLine, startColumn] = sourceManager.getLineAndColumnInBuffer(start); auto [endLine, endColumn] = sourceManager.getLineAndColumnInBuffer(end); - auto locLabel = createLabel('{', fileLabel, "}:", startLine, ':', startColumn, ':', - endLine, ':', endColumn); + auto locLabel = trap.createLabel('{', fileLabel, "}:", startLine, ':', startColumn, + ':', endLine, ':', endColumn); trap.emit(LocationsTrap{locLabel, fileLabel, startLine, startColumn, endLine, endColumn}); trap.emit(LocatableLocationsTrap{locatableLabel, locLabel}); } @@ -275,7 +273,8 @@ class SwiftDispatcher { // which are to be introduced in follow-up PRs virtual void visit(swift::Decl* decl) = 0; virtual void visit(swift::Stmt* stmt) = 0; - virtual void visit(swift::StmtCondition* cond) = 0; + virtual void visit(const swift::StmtCondition* cond) = 0; + virtual void visit(const swift::StmtConditionElement* cond) = 0; virtual void visit(swift::CaseLabelItem* item) = 0; virtual void visit(swift::Expr* expr) = 0; virtual void visit(swift::Pattern* pattern) = 0; @@ -283,8 +282,7 @@ class SwiftDispatcher { virtual void visit(swift::TypeBase* type) = 0; const swift::SourceManager& sourceManager; - TrapArena& arena; - TrapOutput& trap; + TrapDomain& trap; Store store; Store::Handle waitingForNewLabel{std::monostate{}}; swift::ModuleDecl& currentModule; diff --git a/swift/extractor/infra/SwiftTagTraits.h b/swift/extractor/infra/SwiftTagTraits.h index 8a2e489683d..7447fa8f9b3 100644 --- a/swift/extractor/infra/SwiftTagTraits.h +++ b/swift/extractor/infra/SwiftTagTraits.h @@ -36,12 +36,14 @@ using SILBoxTypeReprTag = SilBoxTypeReprTag; MAP_TAG(Stmt); MAP_TAG(StmtCondition); +MAP_TYPE_TO_TAG(StmtConditionElement, ConditionElementTag); MAP_TAG(CaseLabelItem); #define ABSTRACT_STMT(CLASS, PARENT) MAP_SUBTAG(CLASS##Stmt, PARENT) #define STMT(CLASS, PARENT) ABSTRACT_STMT(CLASS, PARENT) #include MAP_TAG(Expr); +MAP_TAG(Argument); #define ABSTRACT_EXPR(CLASS, PARENT) MAP_SUBTAG(CLASS##Expr, PARENT) #define EXPR(CLASS, PARENT) ABSTRACT_EXPR(CLASS, PARENT) #include diff --git a/swift/extractor/infra/TargetFile.cpp b/swift/extractor/infra/TargetFile.cpp new file mode 100644 index 00000000000..5051c0c191f --- /dev/null +++ b/swift/extractor/infra/TargetFile.cpp @@ -0,0 +1,73 @@ +#include "swift/extractor/infra/TargetFile.h" + +#include +#include + +namespace codeql { +namespace { +[[noreturn]] void error(const char* action, const std::string& arg, std::error_code ec) { + std::cerr << "Unable to " << action << ": " << arg << " (" << ec.message() << ")\n"; + std::abort(); +} + +[[noreturn]] void error(const char* action, const std::string& arg) { + error(action, arg, {errno, std::system_category()}); +} + +void ensureParentDir(const std::string& path) { + auto parent = llvm::sys::path::parent_path(path); + if (auto ec = llvm::sys::fs::create_directories(parent)) { + error("create directory", parent.str(), ec); + } +} + +std::string initPath(std::string_view target, std::string_view dir) { + std::string ret{dir}; + assert(!target.empty() && "target must be a non-empty path"); + if (target[0] != '/') { + ret += '/'; + } + ret.append(target); + ensureParentDir(ret); + return ret; +} +} // namespace + +TargetFile::TargetFile(std::string_view target, + std::string_view targetDir, + std::string_view workingDir) + : workingPath{initPath(target, workingDir)}, targetPath{initPath(target, targetDir)} { + errno = 0; + // since C++17 "x" mode opens with O_EXCL (fails if file already exists) + if (auto f = std::fopen(targetPath.c_str(), "wx")) { + std::fclose(f); + out.open(workingPath); + checkOutput("open file for writing"); + } else { + if (errno != EEXIST) { + error("open file for writing", targetPath); + } + // else we just lost the race, do nothing (good() will return false to signal this) + } +} + +bool TargetFile::good() const { + return out && out.is_open(); +} + +void TargetFile::commit() { + assert(good()); + out.close(); + errno = 0; + if (std::rename(workingPath.c_str(), targetPath.c_str()) != 0) { + error("rename file", targetPath); + } +} + +void TargetFile::checkOutput(const char* action) { + if (!out) { + error(action, workingPath); + } +} + +} // namespace codeql diff --git a/swift/extractor/infra/TargetFile.h b/swift/extractor/infra/TargetFile.h new file mode 100644 index 00000000000..91a6f1651f1 --- /dev/null +++ b/swift/extractor/infra/TargetFile.h @@ -0,0 +1,40 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +namespace codeql { + +// Only the first process trying to open an `TargetFile` for a given `target` is allowed to do +// so, all others will have an instance with `good() == false` and failing on any other operation. +// The content streamed to the `TargetFile` is written to `workingDir/target`, and is moved onto +// `targetDir/target` only when `commit()` is called +class TargetFile { + std::string workingPath; + std::string targetPath; + std::ofstream out; + + public: + TargetFile(std::string_view target, std::string_view targetDir, std::string_view workingDir); + + bool good() const; + void commit(); + + template + TargetFile& operator<<(T&& value) { + errno = 0; + out << value; + checkOutput("write to file"); + return *this; + } + + private: + void checkOutput(const char* action); +}; + +} // namespace codeql diff --git a/swift/extractor/trap/TrapArena.h b/swift/extractor/trap/TrapArena.h deleted file mode 100644 index 99d0b4a5d3b..00000000000 --- a/swift/extractor/trap/TrapArena.h +++ /dev/null @@ -1,23 +0,0 @@ -#pragma once - -#include -#include -#include - -#include "swift/extractor/trap/TrapLabel.h" - -namespace codeql { - -// TrapArena has the responsibilities to allocate distinct trap #-labels -// TODO this is now a small functionality that will be moved to code upcoming from other PRs -class TrapArena { - uint64_t id_{0}; - - public: - template - TrapLabel allocateLabel() { - return TrapLabel::unsafeCreateFromExplicitId(id_++); - } -}; - -} // namespace codeql diff --git a/swift/extractor/trap/TrapOutput.h b/swift/extractor/trap/TrapDomain.h similarity index 56% rename from swift/extractor/trap/TrapOutput.h rename to swift/extractor/trap/TrapDomain.h index 0e3f61d39e7..42aed53d499 100644 --- a/swift/extractor/trap/TrapOutput.h +++ b/swift/extractor/trap/TrapDomain.h @@ -2,18 +2,55 @@ #include #include "swift/extractor/trap/TrapLabel.h" +#include "swift/extractor/infra/TargetFile.h" namespace codeql { -// Sink for trap emissions and label assignments. This abstracts away `ofstream` operations -// like `ofstream`, an explicit bool operator is provided, that return false if something -// went wrong -// TODO better error handling -class TrapOutput { - std::ostream& out_; +// Abstracts a given trap output file, with its own universe of trap labels +class TrapDomain { + TargetFile& out_; public: - explicit TrapOutput(std::ostream& out) : out_{out} {} + explicit TrapDomain(TargetFile& out) : out_{out} {} + + template + void emit(const Entry& e) { + print(e); + } + + template + void debug(const Args&... args) { + print("/* DEBUG:"); + print(args...); + print("*/"); + } + + template + TrapLabel createLabel() { + auto ret = allocateLabel(); + assignStar(ret); + return ret; + } + + template + TrapLabel createLabel(Args&&... args) { + auto ret = allocateLabel(); + assignKey(ret, std::forward(args)...); + return ret; + } + + private: + uint64_t id_{0}; + + template + TrapLabel allocateLabel() { + return TrapLabel::unsafeCreateFromExplicitId(id_++); + } + + template + void print(const Args&... args) { + (out_ << ... << args) << '\n'; + } template void assignStar(TrapLabel label) { @@ -33,23 +70,6 @@ class TrapOutput { (oss << ... << keyParts); assignKey(label, oss.str()); } - - template - void emit(const Entry& e) { - print(e); - } - - template - void debug(const Args&... args) { - out_ << "/* DEBUG:\n"; - (out_ << ... << args) << "\n*/\n"; - } - - private: - template - void print(const Args&... args) { - (out_ << ... << args) << '\n'; - } }; } // namespace codeql diff --git a/swift/extractor/visitors/ExprVisitor.cpp b/swift/extractor/visitors/ExprVisitor.cpp index e6aabef4cea..730996a2739 100644 --- a/swift/extractor/visitors/ExprVisitor.cpp +++ b/swift/extractor/visitors/ExprVisitor.cpp @@ -611,11 +611,11 @@ void ExprVisitor::fillAbstractClosureExpr(const swift::AbstractClosureExpr& expr } TrapLabel ExprVisitor::emitArgument(const swift::Argument& arg) { - auto argLabel = dispatcher_.createLabel(); - assert(arg.getExpr() && "Argument has getExpr"); - dispatcher_.emit( - ArgumentsTrap{argLabel, arg.getLabel().str().str(), dispatcher_.fetchLabel(arg.getExpr())}); - return argLabel; + auto entry = dispatcher_.createUncachedEntry(arg); + entry.label = arg.getLabel().str().str(); + entry.expr = dispatcher_.fetchLabel(arg.getExpr()); + dispatcher_.emit(entry); + return entry.id; } void ExprVisitor::emitImplicitConversionExpr(swift::ImplicitConversionExpr* expr, diff --git a/swift/extractor/visitors/StmtVisitor.cpp b/swift/extractor/visitors/StmtVisitor.cpp index 21f93828ed7..bc8a8a30933 100644 --- a/swift/extractor/visitors/StmtVisitor.cpp +++ b/swift/extractor/visitors/StmtVisitor.cpp @@ -7,26 +7,22 @@ void StmtVisitor::visitLabeledStmt(swift::LabeledStmt* stmt) { emitLabeledStmt(stmt, label); } -void StmtVisitor::visitStmtCondition(swift::StmtCondition* cond) { - auto label = dispatcher_.assignNewLabel(cond); - dispatcher_.emit(StmtConditionsTrap{label}); - unsigned index = 0; - for (const auto& cond : *cond) { - auto condLabel = dispatcher_.createLabel(); - dispatcher_.attachLocation(cond, condLabel); - dispatcher_.emit(ConditionElementsTrap{condLabel}); - dispatcher_.emit(StmtConditionElementsTrap{label, index++, condLabel}); - if (auto boolean = cond.getBooleanOrNull()) { - auto elementLabel = dispatcher_.fetchLabel(boolean); - dispatcher_.emit(ConditionElementBooleansTrap{condLabel, elementLabel}); - } else if (auto pattern = cond.getPatternOrNull()) { - auto patternLabel = dispatcher_.fetchLabel(pattern); - auto initilizerLabel = dispatcher_.fetchLabel(cond.getInitializer()); - dispatcher_.emit(ConditionElementPatternsTrap{condLabel, patternLabel}); - dispatcher_.emit(ConditionElementInitializersTrap{condLabel, initilizerLabel}); - } - /// TODO: Implement availability +codeql::StmtCondition StmtVisitor::translateStmtCondition(const swift::StmtCondition& cond) { + auto entry = dispatcher_.createEntry(cond); + entry.elements = dispatcher_.fetchRepeatedLabels(cond); + return entry; +} + +codeql::ConditionElement StmtVisitor::translateStmtConditionElement( + const swift::StmtConditionElement& element) { + auto entry = dispatcher_.createEntry(element); + if (auto boolean = element.getBooleanOrNull()) { + entry.boolean = dispatcher_.fetchLabel(boolean); + } else if (auto pattern = element.getPatternOrNull()) { + entry.pattern = dispatcher_.fetchLabel(pattern); + entry.initializer = dispatcher_.fetchLabel(element.getInitializer()); } + return entry; } void StmtVisitor::visitLabeledConditionalStmt(swift::LabeledConditionalStmt* stmt) { diff --git a/swift/extractor/visitors/StmtVisitor.h b/swift/extractor/visitors/StmtVisitor.h index 78273366d9e..e929a364399 100644 --- a/swift/extractor/visitors/StmtVisitor.h +++ b/swift/extractor/visitors/StmtVisitor.h @@ -10,7 +10,9 @@ class StmtVisitor : public AstVisitorBase { using AstVisitorBase::AstVisitorBase; void visitLabeledStmt(swift::LabeledStmt* stmt); - void visitStmtCondition(swift::StmtCondition* cond); + codeql::StmtCondition translateStmtCondition(const swift::StmtCondition& cond); + codeql::ConditionElement translateStmtConditionElement( + const swift::StmtConditionElement& element); void visitLabeledConditionalStmt(swift::LabeledConditionalStmt* stmt); void visitCaseLabelItem(swift::CaseLabelItem* labelItem); void visitBraceStmt(swift::BraceStmt* stmt); diff --git a/swift/extractor/visitors/SwiftVisitor.h b/swift/extractor/visitors/SwiftVisitor.h index 6380390af82..a13479246f4 100644 --- a/swift/extractor/visitors/SwiftVisitor.h +++ b/swift/extractor/visitors/SwiftVisitor.h @@ -22,7 +22,12 @@ class SwiftVisitor : private SwiftDispatcher { private: void visit(swift::Decl* decl) override { declVisitor.visit(decl); } void visit(swift::Stmt* stmt) override { stmtVisitor.visit(stmt); } - void visit(swift::StmtCondition* cond) override { stmtVisitor.visitStmtCondition(cond); } + void visit(const swift::StmtCondition* cond) override { + emit(stmtVisitor.translateStmtCondition(*cond)); + } + void visit(const swift::StmtConditionElement* element) override { + emit(stmtVisitor.translateStmtConditionElement(*element)); + } void visit(swift::CaseLabelItem* item) override { stmtVisitor.visitCaseLabelItem(item); } void visit(swift::Expr* expr) override { exprVisitor.visit(expr); } void visit(swift::Pattern* pattern) override { patternVisitor.visit(pattern); } diff --git a/swift/ql/lib/codeql/swift/generated/expr/Argument.qll b/swift/ql/lib/codeql/swift/generated/expr/Argument.qll index 9f16b90e3fe..7e8d6ea0ed3 100644 --- a/swift/ql/lib/codeql/swift/generated/expr/Argument.qll +++ b/swift/ql/lib/codeql/swift/generated/expr/Argument.qll @@ -1,8 +1,8 @@ // generated by codegen/codegen.py -import codeql.swift.elements.Element import codeql.swift.elements.expr.Expr +import codeql.swift.elements.Locatable -class ArgumentBase extends @argument, Element { +class ArgumentBase extends @argument, Locatable { override string getAPrimaryQlClass() { result = "Argument" } string getLabel() { arguments(this, result, _) } diff --git a/swift/ql/lib/swift.dbscheme b/swift/ql/lib/swift.dbscheme index 83ee8412131..ae57f4e6fbb 100644 --- a/swift/ql/lib/swift.dbscheme +++ b/swift/ql/lib/swift.dbscheme @@ -13,8 +13,7 @@ sourceLocationPrefix( // from codegen/schema.yml @element = - @argument -| @callable + @callable | @file | @generic_context | @iterable_decl_context @@ -34,7 +33,8 @@ files( ); @locatable = - @ast_node + @argument +| @ast_node | @condition_element ; diff --git a/swift/ql/test/extractor-tests/generated/expr/DotSyntaxCallExpr/DotSyntaxCallExpr_getArgument.expected b/swift/ql/test/extractor-tests/generated/expr/DotSyntaxCallExpr/DotSyntaxCallExpr_getArgument.expected index 64bdae371b7..85102771b48 100644 --- a/swift/ql/test/extractor-tests/generated/expr/DotSyntaxCallExpr/DotSyntaxCallExpr_getArgument.expected +++ b/swift/ql/test/extractor-tests/generated/expr/DotSyntaxCallExpr/DotSyntaxCallExpr_getArgument.expected @@ -1,2 +1,2 @@ -| dot_syntax_call.swift:6:1:6:3 | call to foo(_:_:) | 0 | : X.Type | -| dot_syntax_call.swift:7:1:7:3 | call to bar() | 0 | : X.Type | +| dot_syntax_call.swift:6:1:6:3 | call to foo(_:_:) | 0 | dot_syntax_call.swift:6:1:6:1 | : X.Type | +| dot_syntax_call.swift:7:1:7:3 | call to bar() | 0 | dot_syntax_call.swift:7:1:7:1 | : X.Type | From 4c53c341f6c2fb9022b18c69c6ed6655b0a8669f Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Thu, 14 Jul 2022 05:51:45 +0200 Subject: [PATCH 3/7] Swift: make `TargetFile::good()` a class invariant Fallible initialization has been moved to a factory function, and `commit` has been moved to the destructor. --- swift/extractor/SwiftExtractor.cpp | 35 +++++++++---------- swift/extractor/infra/TargetFile.cpp | 50 ++++++++++++++++++++-------- swift/extractor/infra/TargetFile.h | 25 ++++++++------ 3 files changed, 69 insertions(+), 41 deletions(-) diff --git a/swift/extractor/SwiftExtractor.cpp b/swift/extractor/SwiftExtractor.cpp index 88ad2152473..b36db1ba23b 100644 --- a/swift/extractor/SwiftExtractor.cpp +++ b/swift/extractor/SwiftExtractor.cpp @@ -77,6 +77,20 @@ static llvm::SmallVector getTopLevelDecls(swift::ModuleDecl& modul return ret; } +static void dumpArgs(TargetFile& out, const SwiftExtractorConfiguration& config) { + out << "/* extractor-args:\n"; + for (const auto& opt : config.frontendOptions) { + out << " " << std::quoted(opt) << " \\\n"; + } + out << "\n*/\n"; + + out << "/* swift-frontend-args:\n"; + for (const auto& opt : config.patchedFrontendOptions) { + out << " " << std::quoted(opt) << " \\\n"; + } + out << "\n*/\n"; +} + static void extractDeclarations(const SwiftExtractorConfiguration& config, swift::CompilerInstance& compiler, swift::ModuleDecl& module, @@ -86,25 +100,13 @@ static void extractDeclarations(const SwiftExtractorConfiguration& config, // The extractor can be called several times from different processes with // the same input file(s). Using `TargetFile` the first process will win, and the following // will just skip the work - TargetFile trapStream{filename + ".trap", config.trapDir, config.getTempTrapDir()}; - if (!trapStream.good()) { + auto trapTarget = TargetFile::create(filename + ".trap", config.trapDir, config.getTempTrapDir()); + if (!trapTarget) { // another process arrived first, nothing to do for us return; } - - trapStream << "/* extractor-args:\n"; - for (const auto& opt : config.frontendOptions) { - trapStream << " " << std::quoted(opt) << " \\\n"; - } - trapStream << "\n*/\n"; - - trapStream << "/* swift-frontend-args:\n"; - for (const auto& opt : config.patchedFrontendOptions) { - trapStream << " " << std::quoted(opt) << " \\\n"; - } - trapStream << "\n*/\n"; - - TrapDomain trap{trapStream}; + dumpArgs(*trapTarget, config); + TrapDomain trap{*trapTarget}; // TODO: move default location emission elsewhere, possibly in a separate global trap file // the following cannot conflict with actual files as those have an absolute path starting with / @@ -129,7 +131,6 @@ static void extractDeclarations(const SwiftExtractorConfiguration& config, auto fileLabel = trap.createLabel(name.str().str()); trap.emit(FilesTrap{fileLabel, name.str().str()}); } - trapStream.commit(); } static std::unordered_set collectInputFilenames(swift::CompilerInstance& compiler) { diff --git a/swift/extractor/infra/TargetFile.cpp b/swift/extractor/infra/TargetFile.cpp index 5051c0c191f..d6c4df74318 100644 --- a/swift/extractor/infra/TargetFile.cpp +++ b/swift/extractor/infra/TargetFile.cpp @@ -1,5 +1,10 @@ #include "swift/extractor/infra/TargetFile.h" +#include +#include +#include +#include + #include #include @@ -36,31 +41,49 @@ std::string initPath(std::string_view target, std::string_view dir) { TargetFile::TargetFile(std::string_view target, std::string_view targetDir, std::string_view workingDir) - : workingPath{initPath(target, workingDir)}, targetPath{initPath(target, targetDir)} { + : workingPath{initPath(target, workingDir)}, targetPath{initPath(target, targetDir)} {} + +bool TargetFile::init() { errno = 0; // since C++17 "x" mode opens with O_EXCL (fails if file already exists) if (auto f = std::fopen(targetPath.c_str(), "wx")) { std::fclose(f); out.open(workingPath); checkOutput("open file for writing"); - } else { - if (errno != EEXIST) { - error("open file for writing", targetPath); - } - // else we just lost the race, do nothing (good() will return false to signal this) + return true; } + if (errno != EEXIST) { + error("open file for writing", targetPath); + } + // else we just lost the race + return false; } -bool TargetFile::good() const { - return out && out.is_open(); +std::optional TargetFile::create(std::string_view target, + std::string_view targetDir, + std::string_view workingDir) { + TargetFile ret{target, targetDir, workingDir}; + if (ret.init()) return {std::move(ret)}; + return std::nullopt; +} + +TargetFile& TargetFile::operator=(TargetFile&& other) { + if (this != &other) { + commit(); + workingPath = std::move(other.workingPath); + targetPath = std::move(other.targetPath); + out = std::move(other.out); + } + return *this; } void TargetFile::commit() { - assert(good()); - out.close(); - errno = 0; - if (std::rename(workingPath.c_str(), targetPath.c_str()) != 0) { - error("rename file", targetPath); + if (out.is_open()) { + out.close(); + errno = 0; + if (std::rename(workingPath.c_str(), targetPath.c_str()) != 0) { + error("rename file", targetPath); + } } } @@ -69,5 +92,4 @@ void TargetFile::checkOutput(const char* action) { error(action, workingPath); } } - } // namespace codeql diff --git a/swift/extractor/infra/TargetFile.h b/swift/extractor/infra/TargetFile.h index 91a6f1651f1..551abe0bb76 100644 --- a/swift/extractor/infra/TargetFile.h +++ b/swift/extractor/infra/TargetFile.h @@ -2,28 +2,29 @@ #include #include -#include -#include +#include #include -#include -#include namespace codeql { -// Only the first process trying to open an `TargetFile` for a given `target` is allowed to do -// so, all others will have an instance with `good() == false` and failing on any other operation. +// Only the first process trying to create a `TargetFile` for a given `target` is allowed to do +// so, all others will have `create` return `std::nullopt`. // The content streamed to the `TargetFile` is written to `workingDir/target`, and is moved onto -// `targetDir/target` only when `commit()` is called +// `targetDir/target` on destruction. class TargetFile { std::string workingPath; std::string targetPath; std::ofstream out; public: - TargetFile(std::string_view target, std::string_view targetDir, std::string_view workingDir); + static std::optional create(std::string_view target, + std::string_view targetDir, + std::string_view workingDir); - bool good() const; - void commit(); + ~TargetFile() { commit(); } + + TargetFile(TargetFile&& other) = default; + TargetFile& operator=(TargetFile&& other); template TargetFile& operator<<(T&& value) { @@ -34,7 +35,11 @@ class TargetFile { } private: + TargetFile(std::string_view target, std::string_view targetDir, std::string_view workingDir); + + bool init(); void checkOutput(const char* action); + void commit(); }; } // namespace codeql From d748cb483d1ea03d6786d62eaf5a2292696bdd4b Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Thu, 14 Jul 2022 06:10:12 +0200 Subject: [PATCH 4/7] Swift: include cleanup Fix a problem with `sstream` not being transitively included on macOS. --- swift/extractor/SwiftExtractor.cpp | 4 ---- swift/extractor/trap/TrapDomain.h | 2 ++ 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/swift/extractor/SwiftExtractor.cpp b/swift/extractor/SwiftExtractor.cpp index b36db1ba23b..60ac3436fa8 100644 --- a/swift/extractor/SwiftExtractor.cpp +++ b/swift/extractor/SwiftExtractor.cpp @@ -1,11 +1,7 @@ #include "SwiftExtractor.h" -#include -#include #include -#include #include -#include #include #include diff --git a/swift/extractor/trap/TrapDomain.h b/swift/extractor/trap/TrapDomain.h index 42aed53d499..c84a3910134 100644 --- a/swift/extractor/trap/TrapDomain.h +++ b/swift/extractor/trap/TrapDomain.h @@ -1,6 +1,8 @@ #pragma once #include +#include + #include "swift/extractor/trap/TrapLabel.h" #include "swift/extractor/infra/TargetFile.h" From 3e06455ac1e11ceedbb5ae2b18d54ceb6bacc921 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Thu, 14 Jul 2022 15:39:36 +0200 Subject: [PATCH 5/7] Swift: delete `TargetFile`'s move assignment --- swift/extractor/infra/TargetFile.cpp | 10 ---------- swift/extractor/infra/TargetFile.h | 3 ++- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/swift/extractor/infra/TargetFile.cpp b/swift/extractor/infra/TargetFile.cpp index d6c4df74318..c2639e81bd8 100644 --- a/swift/extractor/infra/TargetFile.cpp +++ b/swift/extractor/infra/TargetFile.cpp @@ -67,16 +67,6 @@ std::optional TargetFile::create(std::string_view target, return std::nullopt; } -TargetFile& TargetFile::operator=(TargetFile&& other) { - if (this != &other) { - commit(); - workingPath = std::move(other.workingPath); - targetPath = std::move(other.targetPath); - out = std::move(other.out); - } - return *this; -} - void TargetFile::commit() { if (out.is_open()) { out.close(); diff --git a/swift/extractor/infra/TargetFile.h b/swift/extractor/infra/TargetFile.h index 551abe0bb76..5f5ca7d823c 100644 --- a/swift/extractor/infra/TargetFile.h +++ b/swift/extractor/infra/TargetFile.h @@ -24,7 +24,8 @@ class TargetFile { ~TargetFile() { commit(); } TargetFile(TargetFile&& other) = default; - TargetFile& operator=(TargetFile&& other); + // move assignment deleted as non-trivial and not needed + TargetFile& operator=(TargetFile&& other) = delete; template TargetFile& operator<<(T&& value) { From 22ff8c2c7e33e2f6bc0533a2b99ea00044fb3cc5 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Thu, 14 Jul 2022 15:40:48 +0200 Subject: [PATCH 6/7] Swift: remove redundant braces --- swift/extractor/infra/TargetFile.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/swift/extractor/infra/TargetFile.cpp b/swift/extractor/infra/TargetFile.cpp index c2639e81bd8..d9706205c90 100644 --- a/swift/extractor/infra/TargetFile.cpp +++ b/swift/extractor/infra/TargetFile.cpp @@ -63,7 +63,7 @@ std::optional TargetFile::create(std::string_view target, std::string_view targetDir, std::string_view workingDir) { TargetFile ret{target, targetDir, workingDir}; - if (ret.init()) return {std::move(ret)}; + if (ret.init()) return ret; return std::nullopt; } From d13f9d5d71e6de2e672a041f14460d5e8dd3184f Mon Sep 17 00:00:00 2001 From: Aditya Sharad <6874315+adityasharad@users.noreply.github.com> Date: Thu, 14 Jul 2022 07:29:29 -0700 Subject: [PATCH 7/7] Update docs/codeql/query-help/javascript.rst Co-authored-by: Felicity Chapman --- docs/codeql/query-help/javascript.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/codeql/query-help/javascript.rst b/docs/codeql/query-help/javascript.rst index ae85de99f7b..58fe97eb3b0 100644 --- a/docs/codeql/query-help/javascript.rst +++ b/docs/codeql/query-help/javascript.rst @@ -3,7 +3,7 @@ CodeQL query help for JavaScript .. include:: ../reusables/query-help-overview.rst -These queries are published in the CodeQL query pack ``codeql/javascript-queries`` (`changelog `__, `source `__). +These queries are published in the CodeQL query pack ``codeql/javascript-queries`` (`changelog `__, `source `__). For shorter queries that you can use as building blocks when writing your own queries, see the `example queries in the CodeQL repository `__.