Skip to content
Closed
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
2 changes: 2 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ add_custom_target(generate_flatbuffers_schema DEPENDS "${GENERATED_HEADER}")
# Temporary migration-only build graph.
add_library(clice-core STATIC
"${PROJECT_SOURCE_DIR}/src/command/command.cpp"
"${PROJECT_SOURCE_DIR}/src/command/search_config.cpp"
"${PROJECT_SOURCE_DIR}/src/command/toolchain.cpp"
"${PROJECT_SOURCE_DIR}/src/command/toolchain_provider.cpp"
"${PROJECT_SOURCE_DIR}/src/compile/compilation.cpp"
"${PROJECT_SOURCE_DIR}/src/compile/compilation_unit.cpp"
"${PROJECT_SOURCE_DIR}/src/compile/diagnostic.cpp"
Expand Down
6 changes: 2 additions & 4 deletions src/clice.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,8 @@ int main(int argc, const char** argv) {
return 1;
}

std::string self_path = llvm::sys::fs::getMainExecutable(argv[0], (void*)main);
if(!clice::fs::init_resource_dir(self_path)) {
LOG_ERROR("Cannot find the resource dir: {}", self_path);
}
static int anchor;
std::string self_path = llvm::sys::fs::getMainExecutable("", &anchor);

auto& mode = *opts.mode;

Expand Down
257 changes: 228 additions & 29 deletions src/command/command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,49 @@ struct CompilationDatabase::Impl {
/// All source files in the compilation database.
llvm::DenseMap<StringID, object_ptr<JSONItem>> files;

/// TODO: Cache of toolchain query driver results.
llvm::DenseMap<CompilationInfo*, int> toolchains;
/// Pluggable toolchain provider: manages toolchain queries and caching.
ToolchainProvider toolchain;

/// Cache of SearchConfig keyed by (CompilationInfo*, options_bits).
/// options_bits encodes the CommandOptions fields that affect the result,
/// so different option combinations don't pollute each other's cache entries.
using ConfigCacheKey = std::pair<const CompilationInfo*, std::uint8_t>;
llvm::DenseMap<ConfigCacheKey, SearchConfig> search_config_cache;

static std::uint8_t options_bits(const CommandOptions& options) {
return options.query_toolchain ? 1u : 0u;
}
Comment on lines +161 to +163

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

options_bits does not encode append/remove options that affect lookup() output.

The options_bits() function only encodes query_toolchain, but lookup() also uses options.append and options.remove (lines 487-569) to modify the final arguments. If the same CompilationInfo is looked up with different append/remove options, the cache will return a stale SearchConfig.

Consider either:

  1. Including a hash of append/remove in the cache key, or
  2. Documenting that lookup_search_config must only be called with consistent append/remove for a given context.
Option 1: Extend options_bits to include append/remove presence
     static std::uint8_t options_bits(const CommandOptions& options) {
-        return options.query_toolchain ? 1u : 0u;
+        std::uint8_t bits = 0;
+        if(options.query_toolchain) bits |= 1u;
+        if(!options.append.empty()) bits |= 2u;
+        if(!options.remove.empty()) bits |= 4u;
+        return bits;
     }

Note: This only distinguishes empty vs non-empty; a full solution would hash the actual content.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static std::uint8_t options_bits(const CommandOptions& options) {
return options.query_toolchain ? 1u : 0u;
}
static std::uint8_t options_bits(const CommandOptions& options) {
std::uint8_t bits = 0;
if(options.query_toolchain) bits |= 1u;
if(!options.append.empty()) bits |= 2u;
if(!options.remove.empty()) bits |= 4u;
return bits;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/command/command.cpp` around lines 161 - 163, The cache key built by
options_bits currently encodes only query_toolchain, but lookup() (and thus
lookup_search_config) also mutates results based on CommandOptions::append and
CommandOptions::remove, so different append/remove values can return stale
SearchConfig; update options_bits(const CommandOptions&) to incorporate
append/remove into the key (e.g., include presence or a simple hash of the
append/remove vectors/strings) so that lookup()/lookup_search_config and cached
SearchConfig entries are differentiated by those values, or alternatively
document in the lookup_search_config API that append/remove must be constant for
a given CompilationInfo; touch the functions options_bits, CommandOptions,
lookup(), and lookup_search_config when applying this change.


/// The clang options we want to filter in all cases, like -c and -o.
llvm::DenseSet<std::uint32_t> filtered_options;

ArgumentParser parser{&allocator};

/// Check if an argument matches the source file path, handling
/// Windows path separator differences (backslash vs forward slash).
static bool is_same_file(llvm::StringRef argument, llvm::StringRef file) {
if(argument == file) {
return true;
}

#ifdef _WIN32
// On Windows, cmake may use backslashes in `arguments` but forward
// slashes in `file`. Normalize and compare.
if(argument.size() == file.size()) {
for(std::size_t i = 0; i < argument.size(); i++) {
char a = argument[i] == '\\' ? '/' : argument[i];
char b = file[i] == '\\' ? '/' : file[i];
if(a != b) {
return false;
}
}
return true;
}
#endif

return false;
}
Comment on lines +170 to +193

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Windows path comparison may fail on case differences.

The is_same_file() function normalizes slash direction but doesn't handle case-insensitivity. On Windows, C:\Foo\Bar.cpp and c:\foo\bar.cpp refer to the same file, but this function would return false.

If CMake or build systems generate paths with inconsistent casing, the source file won't be filtered from arguments (line 208), potentially causing duplicate entries or unexpected behavior.

Suggested case-insensitive comparison for Windows
 `#ifdef` _WIN32
         // On Windows, cmake may use backslashes in `arguments` but forward
         // slashes in `file`. Normalize and compare.
         if(argument.size() == file.size()) {
             for(std::size_t i = 0; i < argument.size(); i++) {
                 char a = argument[i] == '\\' ? '/' : argument[i];
                 char b = file[i] == '\\' ? '/' : file[i];
+                // Case-insensitive comparison on Windows
+                a = std::tolower(static_cast<unsigned char>(a));
+                b = std::tolower(static_cast<unsigned char>(b));
                 if(a != b) {
                     return false;
                 }
             }
             return true;
         }
 `#endif`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/command/command.cpp` around lines 170 - 193, is_same_file currently
normalizes slashes on Windows but still does case-sensitive compares causing
paths like "C:\Foo\Bar.cpp" vs "c:\foo\bar.cpp" to be considered different;
update the Windows-only branch in is_same_file to perform a case-insensitive
comparison (e.g., convert characters to a common case or use a
locale-independent tolower on each character) when comparing normalized
characters (argument and file) so that both slash and case differences are
ignored; ensure you apply the case normalization in the same per-character loop
(or normalize whole strings before comparing) and use safe casts for
char-to-unsigned-char when calling tolower to avoid UB.


object_ptr<CompilationInfo> save_compilation_info(this Impl& self,
llvm::StringRef file,
llvm::StringRef directory,
Expand All @@ -170,8 +205,7 @@ struct CompilationDatabase::Impl {
for(unsigned it = 0; it != arguments.size(); it++) {
llvm::StringRef argument = arguments[it];

/// FIXME: Is it possible that file in command and field are different?
if(argument == file) {
if(is_same_file(argument, file)) {
continue;
}

Expand All @@ -182,6 +216,7 @@ struct CompilationDatabase::Impl {
"/o",
"/Fo",
"/Fe",
"/Fd",
};

/// FIXME: This is a heuristic approach that covers the vast majority of cases, but
Expand Down Expand Up @@ -722,39 +757,94 @@ CompilationContext CompilationDatabase::lookup(llvm::StringRef file,
arguments.emplace_back(self->strings.save(s).data());
};

if(options.resource_dir) {
append_arg("-resource-dir");
append_arg(fs::resource_dir);
}

if(info && options.query_toolchain) {
auto callback = [&](const char* s) {
return save_string(s).data();
};
toolchain::QueryParams params = {file, directory, arguments, callback};
// Save user-level include paths before replacing with cc1 args.
// The cached toolchain query uses minimal args (no -I/-D/-W etc.)
// for cache efficiency, so user include paths must be injected back.
auto user_args = std::move(arguments);

/// FIXME: querying is expensive, we want to cache this ...
arguments = toolchain::query_toolchain(params);
auto cached = self->toolchain.query_cached(file, directory, user_args);

/// FIXME: we need mangle the arguments again.
/// Work around ... the logic of this should be moved to query ...
bool next_main_file = false;
for(auto& arg: arguments) {
if(arg == llvm::StringRef("-main-file-name")) {
next_main_file = true;
continue;
if(cached.empty()) {
LOG_WARN("failed to query toolchain: {}", file);
arguments = std::move(user_args);
} else {
// Start with cc1 result (has system paths, driver flags, etc.).
arguments.assign(cached.begin(), cached.end());

// Remove the temp source file that was appended during query.
arguments.pop_back();
Comment on lines +773 to +776

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Unconditional pop_back() assumes cached result always has a trailing temp source file.

Line 776 calls arguments.pop_back() without checking if arguments has more than one element. While the code path only executes when cached is non-empty (checked on line 768), if the cached result somehow contains only the -cc1 flag or minimal elements, this could remove a required argument.

Consider adding a size check for safety:

Suggested defensive check
             // Start with cc1 result (has system paths, driver flags, etc.).
             arguments.assign(cached.begin(), cached.end());

-            // Remove the temp source file that was appended during query.
-            arguments.pop_back();
+            // Remove the temp source file that was appended during query.
+            if(!arguments.empty()) {
+                arguments.pop_back();
+            }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/command/command.cpp` around lines 773 - 776, The unconditional
arguments.pop_back() after arguments.assign(cached.begin(), cached.end()) is
unsafe; before removing the last element, check that arguments.size() > 0 (or >1
if you require at least one retained argument) and that the last element is
actually the temporary source filename (e.g., matches the temp file pattern or a
known marker). If the checks fail, skip pop_back() (or log a warning) to avoid
removing a required flag; update the block surrounding arguments, cached, and
pop_back() accordingly.


// The toolchain query derives the resource dir from the system
// compiler's executable path. If that compiler is a different clang
// version, its builtin headers may not match ours. Replace the
// queried resource dir with ours so the headers are consistent.
// (See clangd's CommandMangler for precedent.)
if(!resource_dir().empty()) {
llvm::StringRef old_resource_dir;
for(std::size_t i = 0; i + 1 < arguments.size(); ++i) {
if(arguments[i] == llvm::StringRef("-resource-dir")) {
old_resource_dir = arguments[i + 1];
break;
}
}
if(!old_resource_dir.empty() && old_resource_dir != resource_dir()) {
for(auto& arg: arguments) {
llvm::StringRef s(arg);
if(s.starts_with(old_resource_dir)) {
auto replaced =
resource_dir().str() + s.substr(old_resource_dir.size()).str();
arg = self->strings.save(replaced).data();
}
}
}
}

if(next_main_file) {
arg = self->strings.save(path::filename(file)).data();
next_main_file = false;
// Inject user include paths (-I, -isystem, -iquote) from the
// original mangled args into the cc1 result.
self->parser.parse(
llvm::ArrayRef(user_args).drop_front(),
[&](std::unique_ptr<llvm::opt::Arg> arg) {
auto id = arg->getOption().getID();
if(id == ID::OPT_I || id == ID::OPT_isystem || id == ID::OPT_iquote) {
append_arg(arg->getSpelling());
for(auto value: arg->getValues()) {
append_arg(value);
}
}
},
[](int, int) {});

// Fix -main-file-name to match the actual file.
bool next_main_file = false;
for(auto& arg: arguments) {
if(arg == llvm::StringRef("-main-file-name")) {
next_main_file = true;
continue;
}

if(next_main_file) {
arg = self->strings.save(path::filename(file)).data();
next_main_file = false;
}
}
}

if(arguments.empty()) {
LOG_WARN("failed to query toolchain: {}", file);
} else {
arguments.pop_back();
// Inject our resource dir if not already present in the arguments.
// On success, the cc1 output already has -resource-dir (possibly
// replaced above). On failure, the original user_args won't have it.
if(!resource_dir().empty()) {
bool has_resource_dir = false;
for(auto& arg: arguments) {
if(arg == llvm::StringRef("-resource-dir")) {
has_resource_dir = true;
break;
}
}
if(!has_resource_dir) {
append_arg("-resource-dir");
append_arg(resource_dir());
}
}
}

Expand All @@ -763,6 +853,63 @@ CompilationContext CompilationDatabase::lookup(llvm::StringRef file,
return CompilationContext(directory, std::move(arguments));
}

SearchConfig CompilationDatabase::lookup_search_config(llvm::StringRef file,
const CommandOptions& options,
const void* context) {
// Resolve to the internal CompilationInfo pointer for cache lookup.
auto path_id = self->strings.get(file);
auto it = self->files.find(path_id);
const CompilationInfo* info_ptr = nullptr;
if(it != self->files.end()) {
if(!context) {
info_ptr = it->second->info.ptr;
} else {
auto cur = it->second;
while(cur) {
if(cur->info.ptr == context) {
info_ptr = cur->info.ptr;
break;
}
cur = cur->next;
}
}
}

if(info_ptr) {
auto key = Impl::ConfigCacheKey{info_ptr, Impl::options_bits(options)};
auto cache_it = self->search_config_cache.find(key);
if(cache_it != self->search_config_cache.end()) {
return cache_it->second;
}
}

auto ctx = lookup(file, options, context);
auto config = extract_search_config(ctx.arguments, ctx.directory);

if(info_ptr) {
auto key = Impl::ConfigCacheKey{info_ptr, Impl::options_bits(options)};
self->search_config_cache.try_emplace(key, config);
}
return config;
}

bool CompilationDatabase::has_cached_configs() const {
return !self->search_config_cache.empty();
}

llvm::StringRef CompilationDatabase::resource_dir() {
static std::string dir = [] {
// Use address of this lambda to locate our binary via dladdr/proc.
static int anchor;
auto exe = llvm::sys::fs::getMainExecutable("", &anchor);
if(exe.empty()) {
return std::string{};
}
return clang::driver::Driver::GetResourcesPath(exe);
}();
return dir;
}

std::optional<std::uint32_t> CompilationDatabase::get_option_id(llvm::StringRef argument) {
auto& table = clang::driver::getDriverOptTable();

Expand All @@ -783,6 +930,58 @@ std::optional<std::uint32_t> CompilationDatabase::get_option_id(llvm::StringRef
}
}

ToolchainProvider& CompilationDatabase::toolchain() {
return self->toolchain;
}

std::vector<ToolchainProvider::PendingEntry> CompilationDatabase::resolve_toolchain_entries(
llvm::ArrayRef<std::pair<llvm::StringRef, const void*>> files) {
std::vector<ToolchainProvider::PendingEntry> entries;
entries.reserve(files.size());

for(auto& [file, context]: files) {
auto path_id = self->strings.get(file);
auto stored_file = self->strings.get(path_id);

object_ptr<CompilationInfo> info = nullptr;
auto it = self->files.find(path_id);
if(it != self->files.end()) {
if(!context) {
info = it->second->info;
} else {
auto cur = it->second;
while(cur) {
if(cur->info.ptr == context) {
info = cur->info;
break;
}
cur = cur->next;
}
}
}

if(!info || info->arguments.empty()) {
continue;
}

ToolchainProvider::PendingEntry entry;
entry.file = stored_file;
entry.directory = self->strings.get(info->directory);
entry.arguments.reserve(info->arguments.size());
for(auto arg_id: info->arguments) {
entry.arguments.push_back(self->strings.get(arg_id).data());
}

entries.push_back(std::move(entry));
}

return entries;
}

llvm::StringRef CompilationDatabase::resolve_path(std::uint32_t path_id) {
return self->strings.get(path_id);
}

std::vector<const char*> CompilationDatabase::files() {
std::vector<const char*> result;
for(auto& [file, _]: self->files) {
Expand Down
33 changes: 30 additions & 3 deletions src/command/command.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include <string>
#include <vector>

#include "command/search_config.h"
#include "command/toolchain_provider.h"
#include "support/format.h"

#include "llvm/ADT/ArrayRef.h"
Expand All @@ -18,10 +20,9 @@ struct CommandOptions {
/// Ignore unknown commands arguments.
bool ignore_unknown = true;

/// Inject resource directory to the command.
bool resource_dir = false;

/// Query the compiler driver for additional information, such as system includes and target.
/// When enabled, also replaces the queried resource dir with our own (clang tools must use
/// builtin headers matching their parser version — see clangd's CommandMangler for precedent).
bool query_toolchain = false;

/// Suppress the warning log if failed to query driver info.
Expand Down Expand Up @@ -95,9 +96,35 @@ class CompilationDatabase {
/// all contexts and let user choose one.
/// std::vector<CompilationContext> fetch_all(llvm::StringRef file);

/// Combined lookup + extract_search_config with internal caching.
/// Results are cached by CompilationInfo pointer, avoiding repeated
/// argument parsing across multiple calls with the same context.
SearchConfig lookup_search_config(llvm::StringRef file,
const CommandOptions& options = {},
const void* context = nullptr);

/// Check if SearchConfig cache is populated (non-empty).
bool has_cached_configs() const;

/// Get an the option for specific argument.
static std::optional<std::uint32_t> get_option_id(llvm::StringRef argument);

/// Get the resource directory for clang builtin headers. Computed once
/// from the current executable path using Driver::GetResourcesPath.
static llvm::StringRef resource_dir();

/// Resolve a path_id (from UpdateInfo) back to the file path string.
llvm::StringRef resolve_path(std::uint32_t path_id);

/// Access the toolchain provider for batch pre-warming and direct queries.
ToolchainProvider& toolchain();

/// Resolve (file, context) pairs to PendingEntry tuples for toolchain queries.
/// Converts CDB-internal context pointers to raw (file, directory, arguments)
/// that the ToolchainProvider can consume.
std::vector<ToolchainProvider::PendingEntry>
resolve_toolchain_entries(llvm::ArrayRef<std::pair<llvm::StringRef, const void*>> files);

/// FIXME: bad interface design ...
std::vector<const char*> files();

Expand Down
Loading
Loading