Skip to content

fix: reproducible standard library for clang-cl #997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

fix: reproducible standard library for clang-cl #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50eae73
Run tests using clang-cl on windows
K-ballo Sep 8, 2025
b301c6e
Use clang-cl system includes syntax
K-ballo Sep 8, 2025
4ac0c3f
Use clang-cl nostdlibinc option
K-ballo Sep 8, 2025
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
13 changes: 6 additions & 7 deletions src/lib/MrDocsCompilationDatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,8 @@ adjustCommandLine(
it != implicitIncludeDirectories.end()) {
for (auto const& inc : it->second)
{
new_cmdline.emplace_back(std::format("-isystem{}", inc));
}
new_cmdline.emplace_back(is_clang_cl ? std::format("-external:I{}", inc) : std::format("-isystem{}", inc));
}
}
}

Expand All @@ -445,11 +445,10 @@ adjustCommandLine(
// implicit include paths and add the standard library
// and system includes manually. That gives MrDocs
// access to libc++ in a portable way.
new_cmdline.emplace_back("-nostdinc++");
new_cmdline.emplace_back("-nostdlib++");
new_cmdline.emplace_back(is_clang_cl ? "/X" : "-nostdinc++");
for (auto const& inc : (*config)->stdlibIncludes)
{
new_cmdline.emplace_back(std::format("-isystem{}", inc));
new_cmdline.emplace_back(is_clang_cl ? std::format("-external:I{}", inc) : std::format("-isystem{}", inc));
}
}

Expand All @@ -458,7 +457,7 @@ adjustCommandLine(
new_cmdline.emplace_back("-nostdinc");
for (auto const& inc : (*config)->libcIncludes)
{
new_cmdline.emplace_back(std::format("-isystem{}", inc));
new_cmdline.emplace_back(is_clang_cl ? std::format("-external:I{}", inc) : std::format("-isystem{}", inc));
}
}

Expand All @@ -467,7 +466,7 @@ adjustCommandLine(
// ------------------------------------------------------
for (auto const& inc : (*config)->systemIncludes)
{
new_cmdline.emplace_back(std::format("-isystem{}", inc));
new_cmdline.emplace_back(is_clang_cl ? std::format("-external:I{}", inc) : std::format("-isystem{}", inc));
}
for (auto const& inc : (*config)->includes)
{
Expand Down
18 changes: 13 additions & 5 deletions src/lib/SingleFileDB.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,25 @@ class SingleFileDB
public:
explicit
SingleFileDB(
llvm::StringRef pathName)
llvm::StringRef pathName, bool is_clang_cl = false)
{
auto fileName = files::getFileName(pathName);
auto parentDir = files::getParentDir(pathName);

std::vector<std::string> cmds;
cmds.emplace_back("clang");
if (is_clang_cl) {
cmds.emplace_back("clang-cl");
Copy link
Collaborator

@alandefreitas alandefreitas Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This path is unnecessary. is_clang_cl would be generating behavior that's externally functionally equivalent for both true and false.

I know the point here is to make different values arrive at MrDocsCompilationDatabase, but that's not the role of SingleFileDB (which we might want to use independently later and not have to maintain both paths).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an attempt to represent a compilation database generated by cmake on windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that. Besides the obvious problem that clang-cl is not the only compiler on Windows, that's not the role of this class, and everything else I said above. This is not a test class, and you're including a change the class doesn't need. I left another comment explaining how you can test the change you actually want to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow, clang is not the only compiler on Windows/Linux either.

I agree that none of these hardcoded values belong in a class that represents a single file compilation database.

cmds.emplace_back("/std:c++latest");
cmds.emplace_back("/permissive-");
cmds.emplace_back("/WX");
}
else {
cmds.emplace_back("clang");
cmds.emplace_back("-std=c++23");
cmds.emplace_back("-pedantic-errors");
cmds.emplace_back("-Werror");
}
cmds.emplace_back("-fsyntax-only");
cmds.emplace_back("-std=c++23");
cmds.emplace_back("-pedantic-errors");
cmds.emplace_back("-Werror");
cmds.emplace_back(fileName);
cc_.emplace_back(
parentDir,
Expand Down
8 changes: 7 additions & 1 deletion src/test/TestRunner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,15 @@ handleFile(

// Create an adjusted MrDocsDatabase
auto parentDir = files::getParentDir(filePath);
bool const is_clang_cl =
#if defined(WIN32)
true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an arbitrary compile value for a runtime condition. We just need to make sure the include paths we want are getting there. One easy way to test this is to define some macro or something in our libc stubs and check (maybe just static assert) if that's what we're getting in the libcxx.cpp test or some similar new test for libc stubs, or just for that.

Testing the same code with different is_clang_cl in the MrDocsCompilationDatabase is a bit harder, and we'd have to think about it. But that looks more like a unit test than a golden test. It should go into the unit tests, and we check if the commands have the properties we expect from them after MrDocsCompilationDatabase normalizes them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this change is to more closely match the actual user experience. It's a good idea to make the tests more robust.

While we own and can easily modify the libc stubs, there is no test for the system-libc option.

There is a test for the system-stdlib option, but those headers come directly from llvm-project and modifying them in place is questionable. We can add an extra file somewhere in build/share, include this location as an additional system-stdlib location, and check for it with __has_include.

The existing libcxx test check attempts to include <experimental/iterator> which is not currently provided by msvc, but there's no guarantee that it will never be provided.

#else
false;
#endif
std::unordered_map<std::string, std::vector<std::string>> defaultIncludePaths;
MrDocsCompilationDatabase compilations(
llvm::StringRef(parentDir), SingleFileDB(filePath), config, defaultIncludePaths);
llvm::StringRef(parentDir), SingleFileDB(filePath, is_clang_cl), config, defaultIncludePaths);

report::debug("Building Corpus", filePath);
auto corpus = CorpusImpl::build(config, compilations);
Expand Down
Loading