[clangd] Avoid passing -xobjective-c++-header to the system include extractor

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/1568

[nridge]

Note: I did not rebase this on top of D146941, because I'd like to suggest that this fix be backported to the 16.x branch (and presumably that's not the case for D146941); however, I'm happy to rebase D146941 on top of this if that would be helpful.

I'd like to add test coverage for this to system-include-extractor.test, just wanted to post the patch for feedback in the meantime.

[kadircet]

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

473

this feels like too much of a layering violation and might (will?) go wrong in cases where language was explicitly set to objective-c++-header.

if the user is relying on fallback commands with an overwrite of Compiler: in the config && --query-driver globs, would it be too much of a hassle to expect them to have a CompileFlags: Add: ... block too?

[nridge]

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

473

this feels like too much of a layering violation and might (will?) go wrong in cases where language was explicitly set to objective-c++-header.

This has occurred to me, and my first idea for a fix was to limit this change to cases where the -xobjective-c++-header originates from the fallback command.

However, as mentioned here, when I tested this I found that -xobjective-c++-header did not make any difference (compared to -xc++-header or -xc++) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories. This made me think this simpler fix would be appropriate.

if the user is relying on fallback commands with an overwrite of Compiler: in the config && --query-driver globs, would it be too much of a hassle to expect them to have a CompileFlags: Add: ... block too?

You're right, adding a section like this to the config does seem to be a viable workaround:

---

If:
  PathMatch: *\.h

CompileFlags:
  Add: [-xc++-header]

But I think it would still be nice to fix this in clangd, as being foiled by objective-c support not being installed is a very unexpected failure mode for a user whose project does not involve objective-c at all.

For what it's worth, I don't think this kind of setup is uncommon. A common scenario seems to be a casual user playing around with a small project (hence, doesn't have a build system or compile_commands.json), on a platform where --query-driver is needed to find the standard library headers (most commonly, MinGW on Windows).

[kadircet]

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

473

However, as mentioned here, when I tested this I found that -xobjective-c++-header did not make any difference (compared to -xc++-header or -xc++) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories.

Well, that's definitely re-assuring, but I am not sure if it's enough to say it'll work that way with all gcc's or when there are other/certain "system" libraries installed. As in theory objc compilation should at least add some framework search paths and what not by default, no?

But I think it would still be nice to fix this in clangd, as being foiled by objective-c support not being installed is a very unexpected failure mode for a user whose project does not involve objective-c at all.

Completely agree, but we're only showing that to people that already fiddled with clangd internals. So I don't think that as unacceptable.

For what it's worth, I don't think this kind of setup is uncommon. A common scenario seems to be a casual user playing around with a small project (hence, doesn't have a build system or compile_commands.json), on a platform where --query-driver is needed to find the standard library headers (most commonly, MinGW on Windows).

I think instead of trying to make things work with query-driver in such setups, we should try to make sure things work out-of-the-box in mingw (and other toolchain) setups. I believe people not using query-driver in such vanilla installation is way more common than people using query-driver and CompileFlags.Compiler override. Also this will probably make sure other clang-tools can work with those setups too.
We have mingw toolchain detection here.

[nridge]

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

473

However, as mentioned here, when I tested this I found that -xobjective-c++-header did not make any difference (compared to -xc++-header or -xc++) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories.

Well, that's definitely re-assuring, but I am not sure if it's enough to say it'll work that way with all gcc's or when there are other/certain "system" libraries installed. As in theory objc compilation should at least add some framework search paths and what not by default, no?

To be honest, I don't know enough about objective-c to say either way.

Perhaps @dgoldman can help us answer this question: would you expect the -x objective-c++ flag to cause the compiler to use any additional / objective-c specific built-in include directories (compared to -x c++), for any compiler you're aware of that has a gcc-compatible driver syntax?

[nridge]

A user on Discord just ran into this again; I'd like to try and find a way forward with this mitigation.

Since we haven't heard from @dgoldman, I'd like to explore an alternative strategy: plumb in information about whether the -xobjective-c++-header came from the fallback flags, and alter it to -xc++-header only in that case.

@kadircet, would you be open to accepting this with the above change?

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

473

However, as mentioned here, when I tested this I found that -xobjective-c++-header did not make any difference (compared to -xc++-header or -xc++) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories.

Well, that's definitely re-assuring, but I am not sure if it's enough to say it'll work that way with all gcc's or when there are other/certain "system" libraries installed. As in theory objc compilation should at least add some framework search paths and what not by default, no?

To be honest, I don't know enough about objective-c to say either way.

Perhaps @dgoldman can help us answer this question: would you expect the -x objective-c++ flag to cause the compiler to use any additional / objective-c specific built-in include directories (compared to -x c++), for any compiler you're aware of that has a gcc-compatible driver syntax?

473

Circling back to reply to some of the other points made here:

But I think it would still be nice to fix this in clangd, as being foiled by objective-c support not being installed is a very unexpected failure mode for a user whose project does not involve objective-c at all.

Completely agree, but we're only showing that to people that already fiddled with clangd internals. So I don't think that as unacceptable.

FWIW, having fielded many user questions in the Discord channel and on StackOverflow, my impression is that having to use --query-driver is a very frequent need. Often users just starting out with clangd trying to get their standard library includes need to use it. So, I don't think we should view it as something advanced, or as "clangd internals", and if we can make the experience smoother by addressing issues like this, we should do so.

For what it's worth, I don't think this kind of setup is uncommon. A common scenario seems to be a casual user playing around with a small project (hence, doesn't have a build system or compile_commands.json), on a platform where --query-driver is needed to find the standard library headers (most commonly, MinGW on Windows).

I think instead of trying to make things work with query-driver in such setups, we should try to make sure things work out-of-the-box in mingw (and other toolchain) setups. I believe people not using query-driver in such vanilla installation is way more common than people using query-driver and CompileFlags.Compiler override. Also this will probably make sure other clang-tools can work with those setups too.
We have mingw toolchain detection here.

Definitely, if we can reduce the set of scenarios in which users need --query-driver at all, that's a bigger win!

But it's also a more involved effort, and in the case of MinGW probably better undertaken by someone who uses Windows and can test things out there.

Meanwhile, I'm hoping we can achieve a smaller and easier win with this proposed change.

[nridge]

Adding Sam as well in case he has any thoughts on the discussion (another user ran into this recently and filed https://github.com/clangd/clangd/issues/1694)

[sammccall]

Agree this is a mess: the reason we think objective-c++-header is safe is that we have built-in support, but in the presence of query-driver that's not enough.

In terms of testing this: I think ~nobody really cares about objc + gcc these days, the thing we'd expect to break by downgrading objective-c++-header to c++-header is querying the clang driver on a mac.
I checked that, and at least on my machine it makes no difference.

So while this is ugly and layering-violationy, it's also short and simple, and solves a practical problem that we've created ourselves.

(Also discussed with @kadircet offline - you're good to land this)

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

472

Maybe add comment:

// In practice, we don't see different include paths for the two on clang+mac,
// which is the most common objective-c compiler.

[nridge]

Rebase and address review comment

clang-tools-extra/clangd/SystemIncludeExtractor.cpp

for (size_t I = 0, E = Cmd.CommandLine.size(); I < E; ++I) {
       } else if (Arg.consume_front("-isysroot")) {
         if (Arg.empty() && I + 1 < E)
           ISysroot = Cmd.CommandLine[I + 1];
         else
           ISysroot = Arg.str();
       }
     }
 
+    // Downgrade objective-c++-header (used in clangd's fallback flags for .h
+    // files) to c++-header, as some drivers may fail to run the extraction
+    // command if it contains `-xobjective-c++-header` and objective-c++ support
+    // is not installed.
+    // In practice, we don't see different include paths for the two on
+    // clang+mac, which is the most common objectve-c compiler.
+    if (Lang == "objective-c++-header") {
+      Lang = "c++-header";
+    }
+
     // If language is not explicit in the flags, infer from the file.
     // This is important as we want to cache each language separately.
     if (Lang.empty()) {
       llvm::StringRef Ext = llvm::sys::path::extension(File).trim('.');
       auto Type = driver::types::lookupTypeForExtension(Ext);
       if (Type == driver::types::TY_INVALID) {
         elog("System include extraction: invalid file type for {0}", Ext);
       } else {
public:
 
   void operator()(tooling::CompileCommand &Cmd, llvm::StringRef File) const {
     if (Cmd.CommandLine.empty())
       return;
 
     DriverArgs Args(Cmd, File);
     if (Args.Lang.empty())
       return;
     if (auto Info = QueriedDrivers.get(Args, [&] {
           return extractSystemIncludesAndTarget(Args, QueryDriverRegex);
         })) {
       setTarget(addSystemIncludes(Cmd, Info->SystemIncludes), Info->Target);
     }
   }
 
 private:
   // Caches includes extracted from a driver. Key is driver:lang.
   Memoize<llvm::DenseMap<DriverArgs, std::optional<DriverInfo>>> QueriedDrivers;