[clangd] Add BuiltinHeaders flag to Config file
#129459
Conversation
Usage: ``` CompileFlags: BuiltinHeaders: QueryDriver ``` The two supported options are `QueryDriver` and `Clangd`. This Controls whether Clangd should include its own built-in headers (like stddef.h), or use only QueryDriver's built-in headers. Use the option value 'Clangd' (default) to use Clangd's headers, and use 'QueryDriver' to indicate QueryDriver's headers.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Khalil Estell (kammce) ChangesUsage: The two supported options are Full diff: https://github.com/llvm/llvm-project/pull/129459.diff 6 Files Affected:
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index 586d031d58481..33718231d9b55 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -59,6 +59,7 @@ struct Config {
std::optional<std::string> FixedCDBPath;
};
+ enum class BuiltinHeaderPolicy { Clangd, QueryDriver };
/// Controls how the compile command for the current file is determined.
struct {
/// Edits to apply to the compile command, in sequence.
@@ -66,6 +67,7 @@ struct Config {
Edits;
/// Where to search for compilation databases for this file's flags.
CDBSearchSpec CDBSearch = {CDBSearchSpec::Ancestors, std::nullopt};
+ BuiltinHeaderPolicy BuiltinHeaders = BuiltinHeaderPolicy::Clangd;
} CompileFlags;
enum class BackgroundPolicy { Build, Skip };
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index aa2561e081047..31530c206acd7 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -290,6 +290,18 @@ struct FragmentCompiler {
});
}
+ if (F.BuiltinHeaders) {
+ if (auto Val =
+ compileEnum<Config::BuiltinHeaderPolicy>("BuiltinHeaders",
+ *F.BuiltinHeaders)
+ .map("Clangd", Config::BuiltinHeaderPolicy::Clangd)
+ .map("QueryDriver", Config::BuiltinHeaderPolicy::QueryDriver)
+ .value())
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.CompileFlags.BuiltinHeaders = *Val;
+ });
+ }
+
if (F.CompilationDatabase) {
std::optional<Config::CDBSearchSpec> Spec;
if (**F.CompilationDatabase == "Ancestors") {
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index 9535b20253b13..c6b89cde51039 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -170,6 +170,12 @@ struct Fragment {
/// - Ancestors: search all parent directories (the default)
/// - std::nullopt: do not use a compilation database, just default flags.
std::optional<Located<std::string>> CompilationDatabase;
+
+ /// Controls whether Clangd should include its own built-in headers (like
+ /// stddef.h), or use only the QueryDriver's built-in headers. Use the
+ /// option value 'Clangd' (default) to indicate Clangd's headers, and use
+ /// 'QueryDriver' to indicate QueryDriver's headers.
+ std::optional<Located<std::string>> BuiltinHeaders;
};
CompileFlagsBlock CompileFlags;
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index 95cc5c1f9f1cf..a46d45df0d319 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -104,6 +104,10 @@ class Parser {
if (auto Values = scalarValues(N))
F.Remove = std::move(*Values);
});
+ Dict.handle("BuiltinHeaders", [&](Node &N) {
+ if (auto BuiltinHeaders = scalarValue(N, "BuiltinHeaders"))
+ F.BuiltinHeaders = *BuiltinHeaders;
+ });
Dict.handle("CompilationDatabase", [&](Node &N) {
F.CompilationDatabase = scalarValue(N, "CompilationDatabase");
});
diff --git a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
index c1c2e9fab9664..e9f4814738afb 100644
--- a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -30,6 +30,7 @@
// in the paths that are explicitly included by the user.
#include "CompileCommands.h"
+#include "Config.h"
#include "GlobalCompilationDatabase.h"
#include "support/Logger.h"
#include "support/Threading.h"
@@ -401,22 +402,30 @@ extractSystemIncludesAndTarget(const DriverArgs &InputArgs,
if (!Info)
return std::nullopt;
- // The built-in headers are tightly coupled to parser builtins.
- // (These are clang's "resource dir", GCC's GCC_INCLUDE_DIR.)
- // We should keep using clangd's versions, so exclude the queried builtins.
- // They're not specially marked in the -v output, but we can get the path
- // with `$DRIVER -print-file-name=include`.
- if (auto BuiltinHeaders =
- run({Driver, "-print-file-name=include"}, /*OutputIsStderr=*/false)) {
- auto Path = llvm::StringRef(*BuiltinHeaders).trim();
- if (!Path.empty() && llvm::sys::path::is_absolute(Path)) {
- auto Size = Info->SystemIncludes.size();
- llvm::erase(Info->SystemIncludes, Path);
- vlog("System includes extractor: builtin headers {0} {1}", Path,
- (Info->SystemIncludes.size() != Size)
- ? "excluded"
- : "not found in driver's response");
+ switch (Config::current().CompileFlags.BuiltinHeaders) {
+ case Config::BuiltinHeaderPolicy::Clangd: {
+ // The built-in headers are tightly coupled to parser builtins.
+ // (These are clang's "resource dir", GCC's GCC_INCLUDE_DIR.)
+ // We should keep using clangd's versions, so exclude the queried
+ // builtins. They're not specially marked in the -v output, but we can
+ // get the path with `$DRIVER -print-file-name=include`.
+ if (auto BuiltinHeaders = run({Driver, "-print-file-name=include"},
+ /*OutputIsStderr=*/false)) {
+ auto Path = llvm::StringRef(*BuiltinHeaders).trim();
+ if (!Path.empty() && llvm::sys::path::is_absolute(Path)) {
+ auto Size = Info->SystemIncludes.size();
+ llvm::erase(Info->SystemIncludes, Path);
+ vlog("System includes extractor: builtin headers {0} {1}", Path,
+ (Info->SystemIncludes.size() != Size)
+ ? "excluded"
+ : "not found in driver's response");
+ }
}
+ break;
+ }
+ case Config::BuiltinHeaderPolicy::QueryDriver:
+ vlog("System includes extractor: builtin headers from query driver");
+ break;
}
log("System includes extractor: successfully executed {0}\n\tgot includes: "
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 71edb704b49d6..2d942c8f856d1 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -58,6 +58,9 @@ Semantic Highlighting
Compile flags
^^^^^^^^^^^^^
+- Added `BuiltinHeaders` which controls if system includes are extracted from
+ Clangd or solely from the QueryDriver.
+
Hover
^^^^^
@@ -112,7 +115,7 @@ Changes in existing checks
<clang-tidy/checks/bugprone/unchecked-optional-access>` fixing false
positives from smart pointer accessors repeated in checking ``has_value``
and accessing ``value``. The option `IgnoreSmartPointerDereference` should
- no longer be needed and will be removed. Also fixing false positive from
+ no longer be needed and will be removed. Also fixing false positive from
const reference accessors to objects containing optional member.
- Improved :doc:`bugprone-unsafe-functions
@@ -131,7 +134,7 @@ Changes in existing checks
- Improved :doc:`performance/unnecessary-value-param
<clang-tidy/checks/performance/unnecessary-value-param>` check performance by
- tolerating fix-it breaking compilation when functions is used as pointers
+ tolerating fix-it breaking compilation when functions is used as pointers
to avoid matching usage of functions within the current compilation unit.
- Improved :doc:`performance-move-const-arg
|
|
I probably won't have the bandwidth to pay attention here, but I'd like to mention (as it's also said in the comment in code); clangd is only guaranteed to work with builtin headers coming from the same revision of clang (i.e. it might even break with builtin headers from a different clang version, let alone using another compiler). It might be worthwhile to mention what you're trying to solve/improve with this change, so that reviewers can understand "why/if" this change is needed (sorry if that discussion already happened in another channel that isn't visible here, but this is the main context for the change, hence it should be mentioned here as well). |
|
Added myself as a reviewer. I will look at the implementation over the coming days as time permits, but I wanted to address the big-picture questions now:
The The default behaviour was changed to the Prior to that change, we've had users for whom the This is what makes me believe that a configuration toggle is appropriate here; we can't make everyone happy with one setting. |
|
Thank you @HighCommander4 for the added context! |
There was a problem hiding this comment.
Thanks, this patch looks great! I just have some minor wording nits and it should be good to go.
Regarding testing: in principle it would be nice to have an automated test exercising this config option, but unfortunately --query-driver behaviour is tricky to exercise in a test. The one test we have for it is, IMO, sufficiently hard to read or extend that I don't think it's worth doing for this change, which is very simple at the implementation level (applying a boolean config option to gate an existing piece of code). @kadircet please feel free to yell at me if you disagree 😆
I'd argue that the fix isn't as simple as using builtin headers from a different toolchain for those users. Because clang itself (and I'd be surprised if gcc didn't) makes lots of assumptions about what compiler builtins are available, what predefined macros will be set etc. when parsing these builtin headers. So my worry is, by giving "some support" we'll actually create the impression that this should "work" for the users and create more confusion, which will turn into more complexity/maintenance burden in the project. e.g. right now people are getting file not found errors for such headers, going forward we'll just have different errors like declarations not being found, constants having unexpected values etc. Because gcc/clang will rely on different builtins being available. but, i won't block the change from going in. in the end, i don't know too much about gcc intrinsics, and maybe things will just work. i am happy to wait and see the outcome, but if we start getting more requests, i think before digging a deeper hole here we should really consider if we should.
I completely agree about tests. It was solely meant as an "integration" test to make sure interaction works end to end. If we want to test more, I think we should do some restructring. At least move the output we're trying to "emulate" into a separate file so that it's clear what we're parsing. |
Thanks for the added thoughts. One thing we can do to help mitigate these concerns is add a disclaimer to the documentation of (The patch is of course keeping |
|
@kammce Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Resolves clangd/clangd#2074
Problem
If
clangdis used on a project that uses a GCC cross compiler, the system headers for GCC are removed and theclangdsystem headers are used instead.clangd's system headers are built for the host machine. This difference between the host and cross compile target system headers can lead toclangdreporting errors.In my case, I'm using the ARM GNU toolchain
arm-none-eabi-gccon an M1 Mac.The issue comes down to including
cstddefwhich includesstddef.hwhich gets resolved toclang's version ofstddef.hwhich includes the__configfile from clang. The thread API if/else macro chain will emit an#error "No Thread API"because the thread API macros are not defined.A workaround is to define the necessary macros in my clangd configuration file, to silence the errors coming from the mac system headers. This kind of fix seems brittle as a change to the macro names could occur which would break me down the line.
Solution
Add a flag named
BuiltinHeadersto theCompileFlagcollection. The option can have one of two valuesQueryDriverandClangd. The default isClangdwhich uses the builtin headers forClangd.QueryDriverwill use the system header from the compiler matched via the query driver.Usage