[clangd][query-driver] Extract GCC version from the driver output

[ArcsinX]

Summary

Clang uses 4.2.1 as a default GCC version.
For projects that rely on the GCC version, this can be a problem, e.g.

#if __GNUC__ < 5
#error "Incompatible compiler"
#endif

This patch extracts the GCC version from the driver output and adds -fgnuc-version=<extracted version> compile option to ensure that the values of GCC macros are correct.

[joerg]

What do you mean with "GCC macros are correct"? Clang is *not* GCC 5 and not 1:1 compatible with GCC 5. Project that have checks like that should arrive in 2010...

[ArcsinX]

In D107304#2920960, @joerg wrote:

What do you mean with "GCC macros are correct"?

Imagine that we have a project and we build it with GCC, we have compile_commands.json file which contains all compile command used to build this project.
Now we are trying to use clangd for this project, but as it turns, __GNUC__ is equal to 4, but we expect it to be equal to compiler version which was used during the build.

Clang is *not* GCC

We are talking about clangd -- tool, which provides IDE features, I expect it to be compatible (as much as possible) with a project which was built with GCC

[kadircet]

I am not sure this is a good idea. Surely we can go with this approach and pretend to have the same GNUC version as the toolchain, but we are still using clang underneath as @joerg pointed out and we might not be able to parse language/compiler extensions supported by GNU compilers, which will lead to confusion.

I think in such cases people should just have a .clangd file in their repo that adds -fgnuc-version=x.y.z to compile flags, i.e:

CompileFlags:
  Add: -fgnuc-version=x.y.z

This will be a more explicit step taken by the user, hence they'll be more likely to know why clangd is not behaving the way they want it to.

[ArcsinX]

In D107304#2921588, @kadircet wrote:

I am not sure this is a good idea. Surely we can go with this approach and pretend to have the same GNUC version as the toolchain, but we are still using clang underneath as @joerg pointed out and we might not be able to parse language/compiler extensions supported by GNU compilers, which will lead to confusion.

The most confusing thing for me that we already have this implicitly set -fgnuc-version=4.2.1. So, we already have implicitly set __GNUC__ and other GCC macros as a default behavior.

I think in such cases people should just have a .clangd file in their repo that adds -fgnuc-version=x.y.z to compile flags, i.e:

CompileFlags:
  Add: -fgnuc-version=x.y.z

This will be a more explicit step taken by the user, hence they'll be more likely to know why clangd is not behaving the way they want it to.

Sometimes it's not an easy task to understand that errors in diagnostics returned by clangd are the result of incorrect __GNUC__ value (in many projects it's not just #error ...). Also it's not convenient to use .clangd config in this case:

• change compiler version => update .clangd
• several build directories (e.g. native and cross with different GCC versions).
• query-driver supports different compilers in the same compile_commands.json
• idea was to make it in more automatic way.

[kadircet]

The most confusing thing for me that we already have this implicitly set -fgnuc-version=4.2.1. So, we already have implicitly set GNUC and other GCC macros as a default behavior.

It might be because clang actually supports all the GNU extensions up until 4.2.1, I don't know the history here, but if clang can compile with that GNUC macro by default, clangd should be good as well. But there are no promises for anything higher than that.

Sometimes it's not an easy task to understand that errors in diagnostics returned by clangd are the result of incorrect GNUC value (in many projects it's not just #error ...). Also it's not convenient to use .clangd config in this case:
change compiler version => update .clangd
several build directories (e.g. native and cross with different GCC versions).
query-driver supports different compilers in the same compile_commands.json
idea was to make it in more automatic way.

I think these are the reasons why this needs to be set explicitly by the user. Because even though it can make things work, it might as well break things implicitly (e.g. you have a compiler version 4.2.1 everything works as expected, but once you update your compiler to gcc-5 clangd breaks all of a sudden)

[ArcsinX]

In D107304#2921694, @kadircet wrote:

The most confusing thing for me that we already have this implicitly set -fgnuc-version=4.2.1. So, we already have implicitly set GNUC and other GCC macros as a default behavior.

It might be because clang actually supports all the GNU extensions up until 4.2.1, I don't know the history here, but if clang can compile with that GNUC macro by default, clangd should be good as well. But there are no promises for anything higher than that.

Seem the background is that it was unable to build chromium with clang: https://reviews.llvm.org/D68055
And as I can see there: this flag does not enable or disable any GCC extensions implemented in Clang. Thus, it just defines some GCC macros. So, without this patch we guaranty to user that these macros values will be incompatible with his GCC version if it is not 4.2.1, which is not an expected behavior for --query-driver for me: I expect that it can extract as much information from the driver output as possible and help clang with files processing.

I think these are the reasons why this needs to be set explicitly by the user. Because even though it can make things work, it might as well break things implicitly (e.g. you have a compiler version 4.2.1 everything works as expected, but once you update your compiler to gcc-5 clangd breaks all of a sudden)

Can't agree that we have equal probabilities to break something in a project which was build with GCC x.y.z when specifying the correct version (x.y.z) and incorrect (4.2.1).

[joerg]

Yes, there are quite a number of small differences in builtin support and even certain macros that just invite even more trouble than this. This is IMO begging for hard to trace down errors.

[kadircet]

Sorry I think I've failed to explain my point. My concern is not about changing the way clang is parsing the code.

I am saying that people might make use of compiler extensions by guarding their code with GNUC macros. Today we know that having 4.2.1 for GNUC version in clang works for those use cases (one way or the other clang is able to compile majority of the code out there people put behind a guard of GNUC <= 4.2.1). But people might make use of different compiler extensions while guarding for higher GNUC versions and they are probably not tested as extensively today. Hence I am saying that those will get broken in weird ways (and implicitly), while they are working today.

[ArcsinX]

@kadircet Thanks for clarification. Now your position is clear for me.

But clang was designed as a drop-in replacement for GCC:

• https://clang.llvm.org/docs/LanguageExtensions.html#introduction
• https://clang.llvm.org/features.html#gcccompat

I do not have any proves that clang is 100% compatible with any GCC version, but also I can't find any evidence that the most compatible version of GCC is 4.2.1

I really faced the problem with that default 4.2.1 on a project: strange errors in clangd and it was really hard to understand that the reason was __GNUC__ == 4. -fgnuc-version=<real GCC version here> addition helped me to solve this. That's why I think that it can be added automatically (and .clangd can be used to override this with 4.2.1 (or any other) if user doesn't want this automatic GCC version detection =) )

[kadircet]

In D107304#2922806, @ArcsinX wrote:

@kadircet Thanks for clarification. Now your position is clear for me.

But clang was designed as a drop-in replacement for GCC:

• https://clang.llvm.org/docs/LanguageExtensions.html#introduction
• https://clang.llvm.org/features.html#gcccompat

I do not have any proves that clang is 100% compatible with any GCC version, but also I can't find any evidence that the most compatible version of GCC is 4.2.1

The existence of query-driver mechanism is proof that clang is not 100% GCC compatible (anymore). Instead of going with the query-driver approach we wanted to update custom toolchain support to mimic the GCC behaviour, but we gave up because it would get out-of-date again.

I really faced the problem with that default 4.2.1 on a project: strange errors in clangd and it was really hard to understand that the reason was __GNUC__ == 4. -fgnuc-version=<real GCC version here> addition helped me to solve this. That's why I think that it can be added automatically (and .clangd can be used to override this with 4.2.1 (or any other) if user doesn't want this automatic GCC version detection =) )

I definitely get your point too. But I am not convinced that changing the default behaviour here is going to be beneficial for majority of the users.
In other words, if we keep things as-is today, people that want a specific GNUC version can set clangd to parse their code with that specific version they know about and face the consequences if clang doesn't provide the same extensions GCC does. Or we can change the behaviour to what you propose and make people that has a broken parse to change their GNUC version to something they have no idea about (they won't know that they should set it to 4.2.1 or any other specific version).

[ArcsinX]

In D107304#2922975, @kadircet wrote:

But I am not convinced that changing the default behaviour here is going to be beneficial for majority of the users.

I think that majority of users doesn't use --query-driver at all.

In other words, if we keep things as-is today, people that want a specific GNUC version can set clangd to parse their code with that specific version they know about and face the consequences if clang doesn't provide the same extensions GCC does. Or we can change the behaviour to what you propose and make people that has a broken parse to change their GNUC version to something they have no idea about (they won't know that they should set it to 4.2.1 or any other specific version).

I think that setting correct version of GCC wont' break anything for the most of users (but I do not know how to prove this), but can fix (I saw this by myself), that's why I can't agree with you =)
I bet that most of users which face the problem I described, can't solve this by their own, because of knowledge lack (nothing can be found in the documentation that can help):
Level 1: I see errors in my code with --query-driver specified. Why this happened?
Level 2: Ok, I know that this is because of __GNUC__ macro definition, what should I do to make it works?

[kadircet]

Ok if you think that setting GCC version to higher values by default won't break anything, I think we should update this for all of clang instead. Apparently that was something done in the past already in https://github.com/llvm/llvm-project/commit/b5fe494a2cdd33a0b069939795043f538e9a492c. It is currently being set in https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L6127.

If we bumped the version number there for all of clang by default, clangd wouldn't be lying about the version and this will have a much wider visibility. That way we'll be sure that users won't see breakages specific to clangd and we'll also be able to get opinions from other stakeholders in clang that know about possible side effects of this bump. WDYT?

[xbolva00]

I think we should update this for all of clang instead.

But first you need to prove that clang matches gcc x.y.z.

[joerg]

This discussion is becoming pointless. Clang is not a 1:1 replacement for later GCC versions. Easiest example is the support for __builtin_apply and friends.

[ArcsinX]

In D107304#2924886, @kadircet wrote:

Ok if you think that setting GCC version to higher values by default won't break anything,

I think that GCC version should be the same, which was used to build the project: it can be higher or lower than 4.2.1

In D107304#2925220, @joerg wrote:

This discussion is becoming pointless.

Yes, seems we can't get agreement here, so I am planning to abandon this.

Clang is not a 1:1 replacement for later GCC versions. Easiest example is the support for __builtin_apply and friends.

Still we have no example when GCC x.y.z can compile something, but clang with -fgnuc-version=x.y.z can't process this project with some critical errors (but works ok with -fgnuc-version=4.2.1). But it's easy to create an example when -fgnuc-version=4.2.1 doesn't work (example in the description).

[sammccall]

I disagree that this is a pointless discussion, FWIW.

The __GNUC__ macro is used for compiler feature (or bug?) detection.
Clang's current answer is "yes we are GCC >=4, but no we are not GCC >=5".
This causes the codebase to reject the compiler, because it relies on features/absence of bugs from GCC 5. @ArcsinX any idea what these are, concretely?
If clang has those features/lacks those bugs, there's some value in making it pass the feature check.

Problem is, there are significant extensions in GCC 4.3 that clang doesn't implement. Per user's manual:

clang does not support __builtin_va_arg_pack/__builtin_va_arg_pack_len. This is used rarely, but in some potentially interesting places, like the glibc headers, so it may be implemented pending user demand. Note that because clang pretends to be like GCC 4.2, and this extension was introduced in 4.3, the glibc headers will not try to use this extension with clang at the moment.

See also this thread and D51011. So we can't simply bump clang's compatibilty claims.

---

For clangd, I think this illustrates why implicitly increasing __GNUC__ (e.g. via query-driver) isn't safe in general.
For files that optionally use e.g. __builtin_va_arg_pack (like glibc headers), we'll direct the preprocessor to take that path and then reject the code.

With codebases that refuse to build at all without large __GNUC__ but actually mostly parse fine, this is unfortunate, but the sort of thing that users probably should opt into.
The symptom #error You need GCC X.Y to build this code is much easier to understand than the results of trying to use unsupported extensions.
I think the best we can do here is document the workaround.

[ArcsinX]

In D107304#2926310, @sammccall wrote:

Clang's current answer is "yes we are GCC >=4, but no we are not GCC >=5".
This causes the codebase to reject the compiler, because it relies on features/absence of bugs from GCC 5. @ArcsinX any idea what these are, concretely?

The checks about GCC version that I saw were related with some features from C++ standard that GCC < 4.X.Y doesn't support and there was a try to implement these features for GCC < 4.X.Y, but clang (or system headers got from query driver?) has these features and some kind of conflict appears.

[ArcsinX]

As I can see, no chances for this to get approved. So. closing.

clang-tools-extra/clangd/QueryDriverDatabase.cpp

 
 namespace clang {
 namespace clangd {
 namespace {
 
 struct DriverInfo {
   std::vector<std::string> SystemIncludes;
   std::string Target;
+  std::string GccVersion;
 };
 
 bool isValidTarget(llvm::StringRef Triple) {
   std::shared_ptr<TargetOptions> TargetOpts(new TargetOptions);
   TargetOpts->Triple = Triple.str();
   DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
                           new IgnoringDiagConsumer);
   IntrusiveRefCntPtr<TargetInfo> Target =
       TargetInfo::CreateTargetInfo(Diags, TargetOpts);
   return bool(Target);
 }
 
 llvm::Optional<DriverInfo> parseDriverOutput(llvm::StringRef Output) {
   DriverInfo Info;
   const char SIS[] = "#include <...> search starts here:";
   const char SIE[] = "End of search list.";
   const char TS[] = "Target: ";
+  const char VS[] = "gcc version ";
   llvm::SmallVector<llvm::StringRef> Lines;
   Output.split(Lines, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
 
   enum {
     Initial,            // Initial state: searching for target or includes list.
     IncludesExtracting, // Includes extracting.
     Done                // Includes and target extraction done.
   } State = Initial;
   bool SeenIncludes = false;
   bool SeenTarget = false;
+  bool SeenVersion = false;
   for (auto *It = Lines.begin(); State != Done && It != Lines.end(); ++It) {
     auto Line = *It;
     switch (State) {
     case Initial:
       if (!SeenIncludes && Line.trim() == SIS) {
         SeenIncludes = true;
         State = IncludesExtracting;
       } else if (!SeenTarget && Line.trim().startswith(TS)) {
         SeenTarget = true;
         llvm::StringRef TargetLine = Line.trim();
         TargetLine.consume_front(TS);
         // Only detect targets that clang understands
         if (!isValidTarget(TargetLine)) {
           elog("System include extraction: invalid target \"{0}\", ignoring",
                TargetLine);
         } else {
           Info.Target = TargetLine.str();
           vlog("System include extraction: target extracted: \"{0}\"",
                TargetLine);
         }
+      } else if (!SeenVersion && Line.trim().startswith(VS)) {
+        SeenVersion = true;
+        llvm::StringRef GccVersionLine = Line.trim();
+        GccVersionLine.consume_front(VS);
+        Info.GccVersion = GccVersionLine
+                              .take_while([](unsigned char C) {
+                                return C == '.' || std::isdigit(C);
+                              })
+                              .str();
+        vlog("System include extraction: GCC version extracted: \"{0}\"",
+             Info.GccVersion);
       }
       break;
     case IncludesExtracting:
       if (Line.trim() == SIE) {
         State = SeenTarget ? Done : Initial;
       } else {
         Info.SystemIncludes.push_back(Line.trim().str());
         vlog("System include extraction: adding {0}", Line);
if (!BufOrError) {
     return llvm::None;
   }
 
   llvm::Optional<DriverInfo> Info =
       parseDriverOutput(BufOrError->get()->getBuffer());
   if (!Info)
     return llvm::None;
   log("System includes extractor: successfully executed {0}\n\tgot includes: "
-      "\"{1}\"\n\tgot target: \"{2}\"",
-      Driver, llvm::join(Info->SystemIncludes, ", "), Info->Target);
+      "\"{1}\"\n\tgot target: \"{2}\"{3}",
+      Driver, llvm::join(Info->SystemIncludes, ", "), Info->Target,
+      Info->GccVersion.empty()
+          ? std::string()
+          : llvm::formatv("\n\tgot GCC version: \"{0}\"", Info->GccVersion));
   return Info;
 }
 
 tooling::CompileCommand &
 addSystemIncludes(tooling::CompileCommand &Cmd,
                   llvm::ArrayRef<std::string> SystemIncludes) {
   for (llvm::StringRef Include : SystemIncludes) {
     // FIXME(kadircet): This doesn't work when we have "--driver-mode=cl"
for (llvm::StringRef Arg : Cmd.CommandLine) {
       if (Arg == "-target" || Arg.startswith("--target="))
         return Cmd;
     }
     Cmd.CommandLine.push_back("--target=" + Target);
   }
   return Cmd;
 }
 
+tooling::CompileCommand &setGccVersion(tooling::CompileCommand &Cmd,
+                                       const std::string &GccVersion) {
+  if (!GccVersion.empty()) {
+    // We do not want to override existing GCC version with extracted one.
+    for (llvm::StringRef Arg : Cmd.CommandLine) {
+      if (Arg.startswith("-fgnuc-version"))
+        return Cmd;
+    }
+    Cmd.CommandLine.push_back("-fgnuc-version=" + GccVersion);
+  }
+  return Cmd;
+}
+
 /// Converts a glob containing only ** or * into a regex.
 std::string convertGlobToRegex(llvm::StringRef Glob) {
   std::string RegText;
   llvm::raw_string_ostream RegStream(RegText);
   RegStream << '^';
   for (size_t I = 0, E = Glob.size(); I < E; ++I) {
     if (Glob[I] == '*') {
       if (I + 1 < E && Glob[I + 1] == '*') {
if (llvm::any_of(Driver,
       // relative or absolute).
       llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
 
     if (auto Info =
             QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
               return extractSystemIncludesAndTarget(
                   Driver, Lang, Cmd->CommandLine, QueryDriverRegex);
             })) {
-      setTarget(addSystemIncludes(*Cmd, Info->SystemIncludes), Info->Target);
+      setGccVersion(setTarget(addSystemIncludes(*Cmd, Info->SystemIncludes),
+                              Info->Target),
+                    Info->GccVersion);
     }
     return Cmd;
   }
 
 private:
   // Caches includes extracted from a driver. Key is driver:lang.
   Memoize<llvm::StringMap<llvm::Optional<DriverInfo>>> QueriedDrivers;
   llvm::Regex QueryDriverRegex;

clang-tools-extra/clangd/test/system-include-extractor.test

 # RUN: echo '#!/bin/sh' >> %t.dir/bin/my_driver.sh
 # RUN: echo '[ "$0" = "%t.dir/bin/my_driver.sh" ] || exit' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'args="$*"' >> %t.dir/bin/my_driver.sh
 # RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/bin/my_driver.sh
 # RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'echo line to ignore >&2' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "gcc version 8.3.0 (Rev2)\r\n" >&2' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/bin/my_driver.sh
 # RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/bin/my_driver.sh
 # RUN: chmod +x %t.dir/bin/my_driver.sh
 
 # Create header files my/dir/a.h and my/dir2/b.h
 # RUN: mkdir -p %t.dir/my/dir

 {
   "jsonrpc":"2.0",
   "method":"textDocument/didOpen",
   "params": {
     "textDocument": {
       "uri": "file://INPUT_DIR/the-file.cpp",
       "languageId":"cpp",
       "version":1,
-      "text":"#include <a.h>\n#include <b.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif"
+      "text":"#include <a.h>\n#include <b.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif\n#if !defined(__GNUC__) || __GNUC__ != 8\n#error \"Incorrect GCC version\"\n#endif"
     }
   }
 }
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [],
 ---
 {"jsonrpc":"2.0","id":10000,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}