Swift: replace assertions and prints in the file library

Author: redsun82

swift/extractor/infra/file/BUILD.bazel

@@ -2,9 +2,10 @@ load("//swift:rules.bzl", "swift_cc_library")
 
 swift_cc_library(
     name = "file",
-    srcs = glob(["*.cpp"]),
-    hdrs = glob(["*.h"]) + [":path_hash_workaround"],
+    srcs = glob(["*.cpp", "FsLogger.h"]),
+    hdrs = glob(["*.h"], exclude=["FsLogger.h"]) + [":path_hash_workaround"],
     visibility = ["//swift:__subpackages__"],
+    deps = ["//swift/extractor/infra/log"],
 )
 
 genrule(

swift/extractor/infra/file/FsLogger.h

@@ -0,0 +1,10 @@
+#include "swift/extractor/infra/log/SwiftLogging.h"
+
+namespace codeql {
+namespace fs_logger {
+inline Logger& logger() {
+  static Logger ret{"fs"};
+  return ret;
+}
+}  // namespace fs_logger
+}  // namespace codeql

swift/extractor/infra/file/Path.cpp

@@ -1,17 +1,21 @@
 #include "swift/extractor/infra/file/Path.h"
+
 #include <iostream>
 #include <unistd.h>
 
+#include "swift/extractor/infra/file/FsLogger.h"
+
+using namespace std::string_view_literals;
+
 namespace codeql {
 
+using namespace fs_logger;
+
 static bool shouldCanonicalize() {
-  auto preserve = getenv("CODEQL_PRESERVE_SYMLINKS");
-  if (preserve && std::string(preserve) == "true") {
-    return false;
-  }
-  preserve = getenv("SEMMLE_PRESERVE_SYMLINKS");
-  if (preserve && std::string(preserve) == "true") {
-    return false;
+  for (auto var : {"CODEQL_PRESERVE_SYMLINKS", "SEMMLE_PRESERVE_SYMLINKS"}) {
+    if (auto preserve = getenv(var); preserve && preserve == "true"sv) {
+      return false;
+    }
   }
   return true;
 }
@@ -26,8 +30,13 @@ std::filesystem::path resolvePath(const std::filesystem::path& path) {
     ret = std::filesystem::absolute(path, ec);
   }
   if (ec) {
-    std::cerr << "Cannot get " << (canonicalize ? "canonical" : "absolute") << " path: " << path
-              << ": " << ec.message() << "\n";
+    if (ec.value() == ENOENT) {
+      // this is pretty normal, nothing to spam about
+      LOG_DEBUG("resolving non-existing {}", path);
+    } else {
+      LOG_WARNING("cannot get {} path for {} ({})", canonicalize ? "canonical" : "absolute", path,
+                  ec);
+    }
     return path;
   }
   return ret;

swift/extractor/infra/file/TargetFile.cpp

@@ -1,6 +1,8 @@
 #include "swift/extractor/infra/file/TargetFile.h"
+#include "swift/extractor/infra/file/FsLogger.h"
+#include "swift/extractor/infra/log/SwiftLogging.h"
+#include "swift/extractor/infra/log/SwiftAssert.h"
 
-#include <iostream>
 #include <cassert>
 #include <cstdio>
 #include <cerrno>
@@ -10,32 +12,24 @@
 namespace fs = std::filesystem;
 
 namespace codeql {
-namespace {
-[[noreturn]] void error(const char* action, const fs::path& arg, std::error_code ec) {
-  std::cerr << "Unable to " << action << ": " << arg << " (" << ec.message() << ")\n";
-  std::abort();
-}
 
-[[noreturn]] void error(const char* action, const fs::path& arg) {
-  error(action, arg, {errno, std::system_category()});
-}
+using namespace fs_logger;
 
-void check(const char* action, const fs::path& arg, std::error_code ec) {
-  if (ec) {
-    error(action, arg, ec);
-  }
+namespace {
+std::error_code currentErrorCode() {
+  return {errno, std::system_category()};
 }
 
 void ensureParentDir(const fs::path& path) {
   auto parent = path.parent_path();
   std::error_code ec;
   fs::create_directories(parent, ec);
-  check("create directory", parent, ec);
+  CODEQL_ASSERT(!ec, "Unable to create directory {} ({})", parent, ec);
 }
 
 fs::path initPath(const std::filesystem::path& target, const std::filesystem::path& dir) {
   fs::path ret{dir};
-  assert(!target.empty() && "target must be a non-empty path");
+  CODEQL_ASSERT(!target.empty());
   ret /= target.relative_path();
   ensureParentDir(ret);
   return ret;
@@ -53,13 +47,12 @@ bool TargetFile::init() {
   if (auto f = std::fopen(targetPath.c_str(), "wx")) {
     std::fclose(f);
     out.open(workingPath);
-    checkOutput("open file for writing");
+    checkOutput("open");
     return true;
   }
-  if (errno != EEXIST) {
-    error("open file for writing", targetPath);
-  }
-  // else we just lost the race
+  CODEQL_ASSERT(errno == EEXIST, "Unable to open {} for writing ({})", targetPath,
+                currentErrorCode());
+  // else the file already exists and we just lost the race
   return false;
 }
 
@@ -76,13 +69,11 @@ void TargetFile::commit() {
     out.close();
     std::error_code ec;
     fs::rename(workingPath, targetPath, ec);
-    check("rename file", targetPath, ec);
+    CODEQL_ASSERT(!ec, "Unable to rename {} -> {} ({})", workingPath, targetPath, ec);
   }
 }
 
 void TargetFile::checkOutput(const char* action) {
-  if (!out) {
-    error(action, workingPath);
-  }
+  CODEQL_ASSERT(out, "Unable to {} {} ({})", action, workingPath, currentErrorCode());
 }
 }  // namespace codeql

swift/extractor/infra/file/TargetFile.h

@@ -32,7 +32,7 @@ class TargetFile {
   TargetFile& operator<<(T&& value) {
     errno = 0;
     out << value;
-    checkOutput("write to file");
+    checkOutput("write to");
     return *this;
   }
 

swift/extractor/infra/log/SwiftLogging.h

@@ -9,6 +9,7 @@
 #include <binlog/TextOutputStream.hpp>
 #include <binlog/EventFilter.hpp>
 #include <binlog/adapt_stdfilesystem.hpp>
+#include <binlog/adapt_stderrorcode.hpp>
 #include <binlog/adapt_stdoptional.hpp>
 #include <binlog/adapt_stdvariant.hpp>
 

swift/extractor/remapping/SwiftFileInterception.cpp

@@ -14,6 +14,7 @@
 
 #include "swift/extractor/infra/file/PathHash.h"
 #include "swift/extractor/infra/file/Path.h"
+#include "swift/extractor/infra/log/SwiftAssert.h"
 
 #ifdef __APPLE__
 // path is hardcoded as otherwise redirection could break when setting DYLD_FALLBACK_LIBRARY_PATH
@@ -27,14 +28,22 @@
 namespace fs = std::filesystem;
 
 namespace {
+
+namespace {
+codeql::Logger& logger() {
+  static codeql::Logger ret{"open_interception"};
+  return ret;
+}
+}  // namespace
+
 namespace original {
 
 void* openLibC() {
   if (auto ret = dlopen(SHARED_LIBC, RTLD_LAZY)) {
     return ret;
   }
-  std::cerr << "Unable to dlopen " SHARED_LIBC "!\n";
-  std::abort();
+  LOG_CRITICAL("Unable to dlopen " SHARED_LIBC "!");
+  abort();
 }
 
 void* libc() {
@@ -71,8 +80,12 @@ bool mayBeRedirected(const char* path, int flags = O_RDONLY) {
 std::optional<std::string> hashFile(const fs::path& path) {
   auto fd = original::open(path.c_str(), O_RDONLY | O_CLOEXEC);
   if (fd < 0) {
-    auto ec = std::make_error_code(static_cast<std::errc>(errno));
-    std::cerr << "unable to open " << path << " for reading (" << ec.message() << ")\n";
+    if (errno == ENOENT) {
+      LOG_DEBUG("ignoring non-existing module {}", path);
+    } else {
+      LOG_ERROR("unable to open {} for hashing ({})", path,
+                std::make_error_code(static_cast<std::errc>(errno)));
+    }
     return std::nullopt;
   }
   auto hasher = picosha2::hash256_one_by_one();
@@ -102,7 +115,7 @@ class FileInterceptor {
 
   int open(const char* path, int flags, mode_t mode = 0) const {
     fs::path fsPath{path};
-    assert((flags & O_ACCMODE) == O_RDONLY);
+    CODEQL_ASSERT((flags & O_ACCMODE) == O_RDONLY, "We should only be intercepting file reads");
     // try to use the hash map first
     errno = 0;
     if (auto hashed = hashPath(path)) {
@@ -114,15 +127,15 @@ class FileInterceptor {
   }
 
   fs::path redirect(const fs::path& target) const {
-    assert(mayBeRedirected(target.c_str()));
+    CODEQL_ASSERT(mayBeRedirected(target.c_str()), "Trying to redirect {} which is unsupported",
+                  target);
     auto redirected = redirectedPath(target);
     fs::create_directories(redirected.parent_path());
     if (auto hashed = hashPath(target)) {
       std::error_code ec;
       fs::create_symlink(*hashed, redirected, ec);
-      if (ec) {
-        std::cerr << "Cannot remap file " << *hashed << " -> " << redirected << ": " << ec.message()
-                  << "\n";
+      if (ec && ec.value() != ENOENT) {
+        LOG_WARNING("Cannot remap file {} -> {} ({})", *hashed, redirected, ec);
       }
       return *hashed;
     }