Skip to content

Commit 6c932bc

Browse files
committed
Swift: address logging review comments
1 parent abc0c7c commit 6c932bc

File tree

6 files changed

+125
-128
lines changed

6 files changed

+125
-128
lines changed

swift/README.md

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
The Swift CodeQL package is an experimental and unsupported work in progress.
66

7+
##
8+
79
## Building the Swift extractor
810

911
First ensure you have Bazel installed, for example with
@@ -28,7 +30,9 @@ set up the search path
2830
in [the per-user CodeQL configuration file](https://docs.github.com/en/code-security/codeql-cli/using-the-codeql-cli/specifying-command-options-in-a-codeql-configuration-file#using-a-codeql-configuration-file)
2931
.
3032

31-
## Code generation
33+
## Development
34+
35+
### Code generation
3236

3337
Run
3438

@@ -41,7 +45,26 @@ to update generated files. This can be shortened to
4145

4246
You can also run `../misc/codegen/codegen.py`, as long as you are beneath the `swift` directory.
4347

44-
## IDE setup
48+
### Logging configuration
49+
50+
A log file is produced for each run under `CODEQL_EXTRACTOR_SWIFT_LOG_DIR` (the usual DB log directory).
51+
52+
You can use the environment variable `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` to configure levels for
53+
loggers and outputs. This must have the form of a comma separated `spec:level` list, where
54+
`spec` is either a glob pattern (made up of alphanumeric, `/`, `*` and `.` characters) for
55+
matching logger names or one of `out:bin`, `out:text` or `out:console`, and `level` is one of `trace`, `debug`, `info`,
56+
`warning`, `error`, `critical` or `no_logs` to turn logs completely off.
57+
Current output default levels are no binary logs, `info` logs or higher in the text file and `warning` logs or higher on
58+
standard error. By default, all loggers are configured with the lowest output level. Logger names are visible in the
59+
textual logs between `[...]`. Examples are `extractor/dispatcher` or `extractor/<source filename>.trap`. An example
60+
of `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` usage is the following:
61+
62+
```bash
63+
export CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS=out:console:trace,out:text:no_logs,*:warning,*.trap:trace
64+
```
65+
66+
This will turn off generation of a text log file, redirecting all logs to standard error, but will make all loggers only
67+
write warnings or above, except for trap emission logs which will output all logs.
4568

4669
### CLion and the native bazel plugin
4770

@@ -84,3 +107,6 @@ In particular for breakpoints to work you might need to setup the following remo
84107
|-------------|--------------------------------------|
85108
| `swift` | `/absolute/path/to/codeql/swift` |
86109
| `bazel-out` | `/absolute/path/to/codeql/bazel-out` |
110+
111+
### Thread safety
112+
The extractor is single-threaded, and there was no effort to make anything in it thread-safe.

swift/extractor/infra/log/SwiftLogging.cpp

Lines changed: 28 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -96,55 +96,61 @@ void collectSeverityRules(const char* var, LevelConfiguration&& configuration) {
9696

9797
} // namespace
9898

99-
void Log::configure(std::string_view root) {
100-
auto& i = instance();
101-
102-
i.rootName = root;
99+
void Log::configureImpl(std::string_view root) {
100+
rootName = root;
103101
// as we are configuring logging right now, we collect problems and log them at the end
104102
std::vector<std::string> problems;
105103
collectSeverityRules("CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS",
106-
{i.sourceRules, i.binary.level, i.text.level, i.console.level, problems});
107-
if (i.text || i.binary) {
104+
{sourceRules, binary.level, text.level, console.level, problems});
105+
if (text || binary) {
108106
std::filesystem::path logFile = getEnvOr("CODEQL_EXTRACTOR_SWIFT_LOG_DIR", ".");
109107
logFile /= root;
110108
logFile /= std::to_string(std::chrono::system_clock::now().time_since_epoch().count());
111109
std::error_code ec;
112110
std::filesystem::create_directories(logFile.parent_path(), ec);
113111
if (!ec) {
114-
if (i.text) {
112+
if (text) {
115113
logFile.replace_extension(".log");
116-
i.textFile.open(logFile);
117-
if (!i.textFile) {
114+
textFile.open(logFile);
115+
if (!textFile) {
118116
problems.emplace_back("Unable to open text log file " + logFile.string());
119-
i.text.level = Level::no_logs;
117+
text.level = Level::no_logs;
120118
}
121119
}
122-
if (i.binary) {
120+
if (binary) {
123121
logFile.replace_extension(".blog");
124-
i.binary.output.open(logFile, std::fstream::out | std::fstream::binary);
125-
if (!i.binary.output) {
122+
binary.output.open(logFile, std::fstream::out | std::fstream::binary);
123+
if (!binary.output) {
126124
problems.emplace_back("Unable to open binary log file " + logFile.string());
127-
i.binary.level = Level::no_logs;
125+
binary.level = Level::no_logs;
128126
}
129127
}
130128
} else {
131129
problems.emplace_back("Unable to create log directory " + logFile.parent_path().string() +
132130
": " + ec.message());
133-
i.binary.level = Level::no_logs;
134-
i.text.level = Level::no_logs;
131+
binary.level = Level::no_logs;
132+
text.level = Level::no_logs;
135133
}
136134
}
137135
for (const auto& problem : problems) {
138136
LOG_ERROR("{}", problem);
139137
}
140-
LOG_INFO("Logging configured (binary: {}, text: {}, console: {})", i.binary.level, i.text.level,
141-
i.console.level);
142-
flush();
138+
LOG_INFO("Logging configured (binary: {}, text: {}, console: {})", binary.level, text.level,
139+
console.level);
140+
flushImpl();
141+
}
142+
143+
void Log::flushImpl() {
144+
session.consume(*this);
143145
}
144146

145-
void Log::flush() {
146-
auto& i = instance();
147-
i.session.consume(i);
147+
Log::LoggerConfiguration Log::getLoggerConfigurationImpl(std::string_view name) {
148+
LoggerConfiguration ret{session, rootName};
149+
ret.fullyQualifiedName += '/';
150+
ret.fullyQualifiedName += name;
151+
ret.level = std::min({binary.level, text.level, console.level});
152+
ret.level = getLevelFor(ret.fullyQualifiedName, sourceRules, ret.level);
153+
return ret;
148154
}
149155

150156
Log& Log::write(const char* buffer, std::streamsize size) {
@@ -154,28 +160,9 @@ Log& Log::write(const char* buffer, std::streamsize size) {
154160
return *this;
155161
}
156162

157-
Log::Level Log::getLevelForSource(std::string_view name) const {
158-
auto dflt = std::min({binary.level, text.level, console.level});
159-
auto level = getLevelFor(name, sourceRules, dflt);
160-
// avoid Log::logger() constructor loop
161-
if (name.size() > rootName.size() && name.substr(rootName.size() + 1) != "logging") {
162-
LOG_DEBUG("setting up logger \"{}\" with level {}", name, level);
163-
}
164-
return level;
165-
}
166-
167163
Logger& Log::logger() {
168164
static Logger ret{"logging"};
169165
return ret;
170166
}
171167

172-
void Logger::setName(std::string name) {
173-
level_ = Log::instance().getLevelForSource(name);
174-
w.setName(std::move(name));
175-
}
176-
177-
Logger& logger() {
178-
static Logger ret{};
179-
return ret;
180-
}
181168
} // namespace codeql

swift/extractor/infra/log/SwiftLogging.h

Lines changed: 64 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@
1010
#include <binlog/EventFilter.hpp>
1111

1212
// Logging macros. These will call `logger()` to get a Logger instance, picking up any `logger`
13-
// defined in the current scope. A default `codeql::logger()` is provided, otherwise domain-specific
14-
// loggers can be added as class fields called `logger` (as `Logger::operator()()` returns itself).
15-
// Domain specific loggers are set up with a name that appears in the log and can be used to filter
16-
// debug levels (see `Logger`). If working in the global namespace, the default logger can be used
17-
// by defining `auto& logger = codeql::logger();`
13+
// defined in the current scope. Domain-specific loggers can be added or used by either:
14+
// * providing a class field called `logger` (as `Logger::operator()()` returns itself)
15+
// * declaring a local `logger` variable (to be used for one-time execution like code in `main`)
16+
// * declaring a `Logger& logger()` function returning a reference to a static local variable
17+
// * passing a logger around using a `Logger& logger` function parameter
18+
// They are created with a name that appears in the logs and can be used to filter debug levels (see
19+
// `Logger`).
1820
#define LOG_CRITICAL(...) LOG_IMPL(codeql::Log::Level::critical, __VA_ARGS__)
1921
#define LOG_ERROR(...) LOG_IMPL(codeql::Log::Level::error, __VA_ARGS__)
2022
#define LOG_WARNING(...) LOG_IMPL(codeql::Log::Level::warning, __VA_ARGS__)
@@ -68,7 +70,52 @@ class Log {
6870
public:
6971
using Level = binlog::Severity;
7072

73+
// Internal data required to build `Logger` instances
74+
struct LoggerConfiguration {
75+
binlog::Session& session;
76+
std::string fullyQualifiedName;
77+
Level level;
78+
};
79+
80+
// Configure logging. This consists in
81+
// * using environment variable `CODEQL_EXTRACTOR_SWIFT_LOG_DIR` to choose where to dump the log
82+
// file(s). Log files will go to a subdirectory thereof named after `root`
83+
// * using environment variable `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` to configure levels for
84+
// loggers and outputs. This must have the form of a comma separated `spec:level` list, where
85+
// `spec` is either a glob pattern (made up of alphanumeric, `/`, `*` and `.` characters) for
86+
// matching logger names or one of `out:bin`, `out:text` or `out:console`.
87+
// Output default levels can be seen in the corresponding initializers below. By default, all
88+
// loggers are configured with the lowest output level
89+
static void configure(std::string_view root) { instance().configureImpl(root); }
90+
91+
// Flush logs to the designated outputs
92+
static void flush() { instance().flushImpl(); }
93+
94+
// create `Logger` configuration, used internally by `Logger`'s constructor
95+
static LoggerConfiguration getLoggerConfiguration(std::string_view name) {
96+
return instance().getLoggerConfigurationImpl(name);
97+
}
98+
7199
private:
100+
static constexpr const char* format = "%u %S [%n] %m (%G:%L)\n";
101+
102+
Log() = default;
103+
104+
static Log& instance() {
105+
static Log ret;
106+
return ret;
107+
}
108+
109+
static class Logger& logger();
110+
111+
void configureImpl(std::string_view root);
112+
void flushImpl();
113+
LoggerConfiguration getLoggerConfigurationImpl(std::string_view name);
114+
115+
// make `session.consume(*this)` work, which requires access to `write`
116+
friend binlog::Session;
117+
Log& write(const char* buffer, std::streamsize size);
118+
72119
// Output filtered according to a configured log level
73120
template <typename Output>
74121
struct FilteredOutput {
@@ -93,72 +140,39 @@ class Log {
93140
using LevelRule = std::pair<std::regex, Level>;
94141
using LevelRules = std::vector<LevelRule>;
95142

96-
static constexpr const char* format = "%u %S [%n] %m (%G:%L)\n";
97-
98143
binlog::Session session;
99144
std::ofstream textFile;
100145
FilteredOutput<std::ofstream> binary{Level::no_logs};
101146
FilteredOutput<binlog::TextOutputStream> text{Level::info, textFile, format};
102147
FilteredOutput<binlog::TextOutputStream> console{Level::warning, std::cerr, format};
103148
LevelRules sourceRules;
104149
std::string rootName;
105-
106-
Log() = default;
107-
108-
static Log& instance() {
109-
static Log ret;
110-
return ret;
111-
}
112-
113-
friend class Logger;
114-
friend binlog::Session;
115-
116-
Level getLevelForSource(std::string_view name) const;
117-
Log& write(const char* buffer, std::streamsize size);
118-
static class Logger& logger();
119-
120-
public:
121-
// Configure logging. This consists in
122-
// * setting up a default logger with `root` as name
123-
// * using environment variable `CODEQL_EXTRACTOR_SWIFT_LOG_DIR` to choose where to dump the log
124-
// file(s). Log files will go to a subdirectory thereof named after `root`
125-
// * using environment variable `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` to configure levels for
126-
// loggers and outputs. This must have the form of a comma separated `spec:level` list, where
127-
// `spec` is either a glob pattern (made up of alphanumeric, `/`, `*` and `.` characters) for
128-
// matching logger names or one of `out:bin`, `out:text` or `out:console`.
129-
// Output default levels can be seen in the corresponding initializers above. By default, all
130-
// loggers are configured with the lowest output level
131-
static void configure(std::string_view root);
132-
133-
// Flush logs to the designated outputs
134-
static void flush();
135150
};
136151

137152
// This class represent a named domain-specific logger, responsible for pushing logs using the
138153
// underlying `binlog::SessionWriter` class. This has a configured log level, so that logs on this
139154
// `Logger` with a level lower than the configured one are no-ops.
140155
class Logger {
141-
binlog::SessionWriter w{Log::instance().session};
142-
Log::Level level_{Log::Level::no_logs};
143-
144-
void setName(std::string name);
145-
146-
friend Logger& logger();
147-
// constructor for the default `Logger`
148-
explicit Logger() { setName(Log::instance().rootName); }
149-
150156
public:
151-
explicit Logger(const std::string& name) { setName(Log::instance().rootName + '/' + name); }
157+
explicit Logger(const std::string& name) : Logger(Log::getLoggerConfiguration(name)) {}
152158

153159
binlog::SessionWriter& writer() { return w; }
154160
Log::Level level() const { return level_; }
155161

156162
// make defining a `Logger logger` field be equivalent to providing a `Logger& logger()` function
157163
// in order to be picked up by logging macros
158164
Logger& operator()() { return *this; }
159-
};
160165

161-
// default logger
162-
Logger& logger();
166+
private:
167+
static constexpr size_t queueSize = 1 << 20; // default taken from binlog
168+
169+
explicit Logger(Log::LoggerConfiguration&& configuration)
170+
: w{configuration.session, queueSize, /* id */ 0,
171+
std::move(configuration.fullyQualifiedName)},
172+
level_{configuration.level} {}
173+
174+
binlog::SessionWriter w;
175+
Log::Level level_;
176+
};
163177

164178
} // namespace codeql

swift/extractor/main.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,11 @@ int main(int argc, char** argv, char** envp) {
215215

216216
const auto configuration = configure(argc, argv);
217217
codeql::Log::configure("extractor");
218-
auto& logger = codeql::logger();
219-
LOG_INFO("calling extractor with arguments \"{}\"", argDump(argc, argv));
220-
LOG_DEBUG("environment:\n{}\n", envDump(envp));
218+
{
219+
codeql::Logger logger{"main"};
220+
LOG_INFO("calling extractor with arguments \"{}\"", argDump(argc, argv));
221+
LOG_DEBUG("environment:\n{}\n", envDump(envp));
222+
}
221223

222224
auto openInterception = codeql::setupFileInterception(configuration);
223225

swift/third_party/binlog/patches/01-change-severity-printing.patch

Lines changed: 0 additions & 31 deletions
This file was deleted.

swift/third_party/load.bzl

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,5 +64,4 @@ def load_dependencies(workspace_name):
6464
repository = "morganstanley/binlog",
6565
commit = "3fef8846f5ef98e64211e7982c2ead67e0b185a6",
6666
sha256 = "f5c61d90a6eff341bf91771f2f465be391fd85397023e1b391c17214f9cbd045",
67-
patches = ["01-change-severity-printing"],
6867
)

0 commit comments

Comments
 (0)