Skip to content
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.

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

clangd has trouble resolving GCC include_next for C++ system headers for headers with a ".h" extension #70

Closed
NomiChirps opened this issue Jul 25, 2022 · 29 comments

Comments

@NomiChirps
Copy link
Contributor

NomiChirps commented Jul 25, 2022

In my C++ project the file naming convention is foo.cc for source files and foo.h for headers, which I think is not too unusual. I was puzzled about why clangd seemed to be unable to find any system headers (e.g. #include <cassert>) only when looking at my header files -- the source files worked just fine. One exciting journey through the clangd source code later, I discovered that it extracts a different set of system include paths for C- and C++-language files (naturally), but, in the absence of a -x gcc flag specifying it explicitly, will guess the language based on the file's extension.

I was able to work around this issue without renaming my header files by adding --cxxopt=-xc++ to the refresh_compile_commands() rule in my BUILD file, like so:

refresh_compile_commands(
    name = "refresh_compile_commands",
    targets = {
        "//myproject/...": "--cxxopt=-xc++",
    },
)

This was a big headache to track down, though. Would it make sense for bazel-compile-commands-extractor to always add this flag when running bazel aquery? If not, maybe a note in the FAQ :)

@cpsauer
Copy link
Contributor

cpsauer commented Jul 25, 2022

Hey, @NomiChirps! Good to hear from you again, and thanks for your contributions.

Could I ask you to check that this is happening even with the latest version of clangd? (14.0.3 at the time of writing.)

For context: We used to automatically add such a language flag for headers with ambiguous extensions, based on the extension of the corresponding source. Clangd then fixed the underlying issue in 14.0.0, so we removed the workaround. (This is consistent with your reading of the code, because the source file, not the header, is being supplied in the command.) The full historical context on this issue is in #12. But if this issue is back somehow, I definitely want to put the language flag logic back in.

Cheers,
Chris

@NomiChirps
Copy link
Contributor Author

NomiChirps commented Jul 25, 2022

Aha, I think I see. Thanks for those pointers to the clangd issue; I wasn't aware of the transferCompileCommand behavior added in https://reviews.llvm.org/D116167.

It looks like the root issue might actually be with my toolchain definition; I'm using https://github.com/dfr/rules_pico with --platforms=@rules_pico//platforms:pico.

So far I've found that, with a minimal cc_library() test target and a standard Bazel gcc-on-windows config (set BAZEL_SH to the msys2 bash.exe and put --compiler=mingw-gcc in .bazelrc), the resulting compile_commands.json contains only a single entry, for my "test.cc", and everything works as it should.

But with the rules_pico toolchain, it contains entries not only for "test.cc", but also "test.h" and for the entire transitive dependency set of system headers (g:\\code\\tools\\gcc-arm-11.2-2022.02-mingw-w64-i686-arm-none-eabi\\arm-none-eabi\\include\\machine\\ieeefp.h and so on).

No doubt what's happening is that clangd skips the flag-transfer logic in https://reviews.llvm.org/D116167 because a command line for "test.h" is provided explicitly in compile_commands.json.

I'm trying to figure out what exactly it is about the rules_pico toolchain definition that causes this difference in bazel-compile-command-extractor's behavior. Will let you know if I find anything.

@cpsauer
Copy link
Contributor

cpsauer commented Jul 26, 2022

Thanks for diving in, @NomiChirps!

To confirm, if you run something like bazel aquery "mnemonic('(Objc|Cpp)Compile',deps(//...))", you're seeing entries for headers with this rules_pico toolchain, but not with a standard toolchain!? If so, very strange indeed that they'd be creating compile actions for the headers. We could, I suppose, add a filter for malformed compile actions like that, but it seems offhand like the toolchain should be fixed to not create them.
[I'm assuming you're passing exclude_headers = "all" to this tool, so we'd expect no headers in compile_commands.json. I'm also assuming that by "entries for test.h", you mean that the command arguments list contains "test.h", not just that the file is "test.h", so as to not invoke that flag-transfer logic.]

@NomiChirps
Copy link
Contributor Author

NomiChirps commented Jul 26, 2022

Sorry, I should have been more clear! No, the compile_commands.json looks like this: (full gist)

[
  {
    "file": "test.cc",
    "arguments": [
      "G:/code/tools/gcc-arm-11.2-2022.02-mingw-w64-i686-arm-none-eabi/bin/arm-none-eabi-gcc",
      "-U_FORTIFY_SOURCE",
      "-D_FORTIFY_SOURCE=1",
      "-DFREESTANDING",
      "-fstack-protector",
      "-Wall",
      "-fno-omit-frame-pointer",
      "-mcpu=cortex-m0plus",
      "-mthumb",
      "-MD",
      "-MF",
      "bazel-out/x64_windows-fastbuild/bin/_objs/test/test.d",
      "-frandom-seed=bazel-out/x64_windows-fastbuild/bin/_objs/test/test.o",
      "-iquote",
      ".",
      "-iquote",
      "bazel-out/x64_windows-fastbuild/bin",
      "-no-canonical-prefixes",
      "-Wno-builtin-macro-redefined",
      "-D__DATE__=\"redacted\"",
      "-D__TIMESTAMP__=\"redacted\"",
      "-D__TIME__=\"redacted\"",
      "-c",
      "test.cc",
      "-o",
      "bazel-out/x64_windows-fastbuild/bin/_objs/test/test.o"
    ],
    "directory": "G:/code/compile-commands-issue-repro"
  },
  {
    "file": "g:\\code\\tools\\gcc-arm-11.2-2022.02-mingw-w64-i686-arm-none-eabi\\arm-none-eabi\\include\\machine\\endian.h",
    "arguments": [
      "G:/code/tools/gcc-arm-11.2-2022.02-mingw-w64-i686-arm-none-eabi/bin/arm-none-eabi-gcc",
      [... same flags ...]
      "-c",
      "test.cc",
      "-o",
      "bazel-out/x64_windows-fastbuild/bin/_objs/test/test.o"
    ],
    "directory": "G:/code/compile-commands-issue-repro"
  },
  [... many more system headers ...]
  [eventually there is also a 'file: "test.h"' with the same command line]

bazel aquery "mnemonic('(Objc|Cpp)Compile',deps(//...))" only produces one command, for test.cc, as expected.

I haven't been setting exclude_headers at all! I skimmed over that part of the README, oops :). I've been assuming headers should have been excluded by default. With exclude_headers = "all", it behaves as expected: just one item in compile_commands.json, for test.cc, and clangd guesses the header language correctly. Testing now, I can't seem to reproduce the behavior I mentioned before with the mingw-gcc toolchain, of the headers being excluded even without exclude_headers being specified. Not sure what happened there; probably a mistake. Sorry for the red herring!

Okay, so, observations:

Toolchain compile_commands.json contains headers? clangd guesses header language correctly? Reproduction compile_commands.json gist
rules_pico Yes No https://github.com/NomiChirps/compile-commands-issue-repro https://gist.github.com/NomiChirps/834c67898314a4d4a53ddebbb481fce8
mingw-gcc Yes/No ??? Yes https://github.com/NomiChirps/compile-commands-issue-repro/tree/mingw-gcc https://gist.github.com/NomiChirps/7a2228daca8a6a00b6c9414076c9b65b
MSVC (Bazel default) Yes Yes https://github.com/NomiChirps/compile-commands-issue-repro/tree/msvc https://gist.github.com/NomiChirps/0c5edf05bca533c5e131100295638704

The relevant difference seems to be that the mingw-gcc toolchain specifies --std=gnu++0x, but rules_pico specifies neither --std nor -x. However, MSVC doesn't specify /TP or anything either, so now I'm extra confused about clangd's logic here.

Thank you for working with me to figure this out!

edit: Okay, weird. Now the mingw-gcc toolchain DOES exclude headers again, even without exclude_headers=all. Random, inconsistent behavior? There must be something I'm not controlling for in my tests.

edit2: I should mention that I've been testing mostly with clangd-14.0.3, but my original issue also occurred with 14.0.0 and with clangd_snapshot_20220717.

@NomiChirps
Copy link
Contributor Author

Clangd verbose log for rules_pico: https://gist.github.com/NomiChirps/85afddb252bb9699b2d0d59a2a3f43f3

For mingw-gcc; however, I can no longer reproduce the compile_commands.json in the table above. This log is with this compile_commands.json, which excludes all header files for some unknown reason. https://gist.github.com/NomiChirps/7421c7029fada1b0634caf9e887e73f8

For MSVC: https://gist.github.com/NomiChirps/d8d662f5cd8a2209cc7837d561326542

Curiously, clangd's system include extraction fails for both mingw-gcc and MSVC, but clangd uses the system MSVC headers in both cases. Apparently a fallback.

@cpsauer
Copy link
Contributor

cpsauer commented Jul 28, 2022

Thanks for scoping things some more, @NomiChirps.

Since I'm traveling--and a few thousand miles from my Windows machines--it's going to be several days before I can get back to this properly. Sorry to not have my usual speed, but I wanted to at least give you a heads up.

A couple of things in the meantime, if you want:

  1. If you notice anything more or come up with any more hypotheses, please let me know.
  2. It might be worth trying to run via the command line, the command from the arguments list for test.h in compile_commands.json. The goal would be making sure it is well formed and runs cleanly. If so, the underlying issue is probably a clangd bug, which we should also file over at clangd/clangd. If so it also might be worth temporarily trying ccls to see if it can avoid the problem.

Cheers! Sorry to be out of town--and have this not be smooth sailing for you.

Chris

@js8544
Copy link

js8544 commented Jul 29, 2022

I am having the same issue when compiling Tensorflow on a Mac M1 pro. Clangd treated the header files as C files until I added the "--cxxopt=-xc++" flag to refresh_compile_commands. With the "--cxxopt=-xc++" flag, it works correctly, treating header files as C++.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

Hey guys. Sorry it's taken me so much longer than usual to get to this. But I'm back and working on it.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

My first attempts to reproduce were on my mac, but I'm not managing to hit the error case yet, even without -x or -std.

For example, I tried grabbing the sample repo, stripped down to be toolchain-independent (since I didn't have the pico compiler installed), but removed all references to -std in the generated compile_commands.json. (There were no -x, language entries.) I was seeing similar results on my other test repos, so clearly I haven't figured out what's causing the issue. That's probably why it's there :)

When I looked at the clangd output (In VSCode, open the terminal pane -> output tab -> clangd from dropdown, while test.h is open) I'm seeing that clangd is doing the correct thing, auto inserting -x c++-header to prevent the issue, consistent with https://reviews.llvm.org/D116167.


I'll switch to my windows desktop shortly, but I'm hoping I can get a bit more of your help tracking things down and getting us to a minimal reproduction. The goal is to get a crisp understanding of what's wrong so that we can both patch around it and worth with the clangd/clangd folks to resolve the underlying issue

@NomiChirps, could I ask you to try manually adding -std=c++11 to the test.h compile_commands.json in the error case, and confirm that that fixes the error case? Knowing that we can intentionally trip the issue will go a long way towards being sure we know the cause.

You may have to manually restart clangd for things to take effect. (VSCode command palate: CTRL+shift+P -> clangd:restart language server) If adding -std doesn't work, could you try with -x c++-header? And then could I ask you to look at the clangd output to see what command it was choosing, with and without the addition? Search for the line after "test.h version 1 with command" in the logs.

And @js8544, any further ideas on how to reduce your tensorflow case into a more simple, minimal example? I was trying on my mac, but again, things seemed to work with clangd 14, even without any -x or -std flags.

Thanks for your help and patience tracking this down!
Chris

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

Or to try to track things down from the other end:
Does the following, super minimal example, modified from @NomiChirps', reproduce the issue for you, @js8544, or you, @NomiChirps, if you use pico or gcc and play with the flags? I'm try to track down whether its -x and/or -std flags that trigger this on your systems, since that isn't triggering it on mine, or whether it's something about a specific toolchain.

clangd_system_headers.zip

(Just download and bazel run :refresh_compile_commands)

I'm seeing the working behavior above, with or without -std. My compile_command.json looks like the following:

Click to expand
[
  {
    "file": "test.cc",
    "arguments": [
      "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang",
      "-D_FORTIFY_SOURCE=1",
      "-fstack-protector",
      "-fcolor-diagnostics",
      "-Wall",
      "-Wthread-safety",
      "-Wself-assign",
      "-fno-omit-frame-pointer",
      "-O0",
      "-DDEBUG",
      "-std=c++11",
      "-iquote",
      ".",
      "-iquote",
      "bazel-out/darwin-fastbuild/bin",
      "-MD",
      "-MF",
      "bazel-out/darwin-fastbuild/bin/_objs/test/test.d",
      "-frandom-seed=bazel-out/darwin-fastbuild/bin/_objs/test/test.o",
      "-isysroot",
      "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk",
      "-F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks",
      "-F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks",
      "-no-canonical-prefixes",
      "-pthread",
      "-no-canonical-prefixes",
      "-Wno-builtin-macro-redefined",
      "-D__DATE__=\"redacted\"",
      "-D__TIMESTAMP__=\"redacted\"",
      "-D__TIME__=\"redacted\"",
      "-target",
      "x86_64-apple-macosx12.3",
      "-c",
      "test.cc",
      "-o",
      "bazel-out/darwin-fastbuild/bin/_objs/test/test.o"
    ],
    "directory": "/Users/cpsauer/Downloads/compile-commands-issue-repro-main"
  },
  {
    "file": "test.h",
    "arguments": [
      "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang",
      "-D_FORTIFY_SOURCE=1",
      "-fstack-protector",
      "-fcolor-diagnostics",
      "-Wall",
      "-Wthread-safety",
      "-Wself-assign",
      "-fno-omit-frame-pointer",
      "-O0",
      "-DDEBUG",
      "-std=c++11",
      "-iquote",
      ".",
      "-iquote",
      "bazel-out/darwin-fastbuild/bin",
      "-MD",
      "-MF",
      "bazel-out/darwin-fastbuild/bin/_objs/test/test.d",
      "-frandom-seed=bazel-out/darwin-fastbuild/bin/_objs/test/test.o",
      "-isysroot",
      "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk",
      "-F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks",
      "-F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks",
      "-no-canonical-prefixes",
      "-pthread",
      "-no-canonical-prefixes",
      "-Wno-builtin-macro-redefined",
      "-D__DATE__=\"redacted\"",
      "-D__TIMESTAMP__=\"redacted\"",
      "-D__TIME__=\"redacted\"",
      "-target",
      "x86_64-apple-macosx12.3",
      "-c",
      "test.cc",
      "-o",
      "bazel-out/darwin-fastbuild/bin/_objs/test/test.o"
    ],
    "directory": "/Users/cpsauer/Downloads/compile-commands-issue-repro-main"
  }
]

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

Also, @NomiChirps, anything I should know to efficiently install that compiler for pico?

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

And @NomiChirps, I was going through your clangd log for the problem case, and I'm seeing it auto adding the -x c++-header in the command for test.h, meaning that it does think it's a c++ header, right? Any other theories about what's going on here?

@js8544
Copy link

js8544 commented Aug 20, 2022

Or to try to track things down from the other end: Does the following, super minimal example, modified from @NomiChirps', reproduce the issue for you, @js8544, or you, @NomiChirps, if you use pico or gcc and play with the flags? I'm try to track down whether its -x and/or -std flags that trigger this on your systems, since that isn't triggering it on mine, or whether it's something about a specific toolchain.

clangd_system_headers.zip

(Just download and bazel run :refresh_compile_commands)

I'm seeing the working behavior above, with or without -std. My compile_command.json looks like the following:

Click to expand

It does reproduce for me. Clangd gives Invalid argument '-std=c++0x' not allowed with 'C' and 'cstdint' file not found on test.h but no error on test.cc

@js8544
Copy link

js8544 commented Aug 20, 2022

Oh I see. My system has clang 13.0.1, which doesn't include the fix @cpsauer mentioned above. So recognizing headers as c files is expected.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

Just to be clear, clangd 13, right? If so, good news, since clangd 14 should resolve that issue. (Seems likely that we're dealing with two different root causes here.)

@js8544
Copy link

js8544 commented Aug 20, 2022

Just to be clear, clang_d_ 13, right? If so, good news, since clangd 14 should resolve that issue. (Seems likely that we're dealing with two different root causes here.)

Yep, clangd 13. So my case is closed :)

@cpsauer
Copy link
Contributor

cpsauer commented Aug 20, 2022

Yay! Great. Delighted to hear we've got things working for you. One down, one to go.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 24, 2022

@NomiChirps, thinking about your clangd log:

Do those detected default include paths (e.g. g:\code\tools\gcc-arm-11.2-2022.02-mingw-w64-i686-arm-none-eabi\bin\../lib/gcc/arm-none-eabi/11.2.1/include) look generally right? Like if you resolve them on your machine via CMD, do they point somewhere valid?

[Looks like cstdint (the missing header in test.h) is at g:\\code\\tools\\gcc-arm-11.2-2022.02-mingw-w64-i686-arm-none-eabi\\arm-none-eabi\\include\\c++\\11.2.1\\cstdint. That differs by a bit, but is close enough that maybe there's junctioning. Interestingly, the c++-specific paths don't always appear as auto-detected in my (working) macOS setup either, so that particular part doesn't seem to be a cause for alarm.]

It's seeming more and more likely (from the log) that clangd is recognizing the right language, since it's explicitly inferring -x c++-header for cl. But I'm not sure how to reconcile that with --cxxopt=-xc++ fixing things for you.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 24, 2022

@HighCommander4, sorry to pull you in, but any idea what might be going on from the clangd side here?

@NomiChirps
Copy link
Contributor Author

Okey dokey, here's as clean and complete of a reproduction as I think I can manage: https://github.com/NomiChirps/compile-commands-testing

There's no rules_pico or alternate toolchain involved, and I'm now on Linux instead of Windows. Just one cc_binary and one cc_library, 3 files in total. Nevertheless clangd/vscode is having trouble with the include in "foo.h" unless I add the --cxxopt=-xc++ workaround. I've also included a copy of all the logs I can think of. I'll try to dig into it when I have time and see if I can find anything obviously wrong.

I should also note that another functioning workaround seems to be setting exclude_headers="all", which I'm now using in my main project instead of "--cxxopt=-xc++", because of the positive effect it has on VSCode's performance. exclude_headers="external" without "--cxxopt=xc++" does NOT work.

Please let me know if this reproduces the issue on your system too!

@cpsauer
Copy link
Contributor

cpsauer commented Aug 24, 2022

Hey @NomiChirps! Thanks so much

...but, and you're not going to like this, I'm earnestly trying and haven't yet managed to reproduce. All works fine so far on my machine, with the logs showing it's (re)loaded the database, and inferred that it's a C++ header, found iostream. All like your bad log, except that it doesn't then choke on the transitive include. Obviously substantial system differences--like that mine uses clang under the hood instead of gcc.

[The great irony here, of course, is that all this header entry emitting is to work around, basically, the exact issue you initially suspected this was: inferring the wrong language for .h files. exclude_headers="all" will definitely cause that problem in some cases, for example, if you directly open a C++ header-only library that has files in another language (C or Objc) right closest to it.]

I'll dig in more tm (downloading and spinning up a new VM overnight), but I'm guessing this is going to end up being an underlying clangd bug--perhaps with how it navigates gcc's default includes. If the compile_commands.json looks right, and the clangd output wrong, then we've got to go a layer down into clangd. Hence my trying to call in @HighCommander4 for his clangd expertise. In my experience, he's been magically good at tracing these. And then we can get it over to clangd/clangd, understanding what's going wrong.

-CS

@cpsauer
Copy link
Contributor

cpsauer commented Aug 25, 2022

Yay! I can reproduce the issue with GCC in this new Ubuntu VM.
(GCC 11.2.0, so it reproduces with a different gcc version than yours.)

That facilitated my distilling the error case down further to something independent of bazel and this project, so the clangd folks can easily play with it. Thanks @NomiChirps for your patience in helping me reproduce the issue. Sorry about my clang-based-blindness, the issue, and how long this has taken.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 25, 2022

@NomiChirps, it's unrelated to this problem, but I happened to notice that your logs indicate your clangd configuration is missing the compile-commands-dir flag from the README. I figured I should give you a friendly heads, since it's useful to have, e.g., so that clangd correctly finds commands for external or generated code. Again, no impact on this particular issue--just noticed and figured you'd want the heads up.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 25, 2022

Being able to play around with the problem let me significantly narrow down the issue. Here's what I found:

  • The problem had to do with include_next; changing the problematic, transitive #include_next <cstdlib>s to #include <cstdlib> makes the problem go away. Similarly, directly including cstdlib from foo.h works fine.
  • As you'd said, things resolve if you add -xc++ to the header. But no problems appear if you add -xc++-header to the source.
  • Removing the entry for the header causes the error to go away, even though that means clangd is inferring the same command from the other file.
  • Changing to a hpp extension causes the error to go away.
  • Disabling --query-driver causes the error to go away.

I'll submit the issue to the clangd folks in a sec, but please do say something if you were seeing something more general. I know that in the very first post you were saying that all stdlib includes were showing erroneous red squiggles, which is different from what I was seeing.

@cpsauer cpsauer changed the title clangd does not include C++ system headers in the search path for files with a ".h" extension clangd has trouble resolving GCC include_next for C++ system headers for headers with a ".h" extension Aug 25, 2022
@cpsauer
Copy link
Contributor

cpsauer commented Aug 25, 2022

Okay, hopefully that's report thoroughly captures what we know on the simplest of cases, so they can solve this problem a layer down.

In the meantime, let's get an interim fix. I think the cleanest is to restore the -x inserting workaround, since I know of another clangd bug that could benefit from our re-inserting that.

@cpsauer
Copy link
Contributor

cpsauer commented Aug 25, 2022

Restored that workaround with improvements!

Should fix things until the clangd folks do, so I'm going to optimistically close this for now.

But please check that it fixes things for you! And if it doesn't, holler, and I'll open this right back up.

@cpsauer cpsauer closed this as completed Aug 25, 2022
@NomiChirps
Copy link
Contributor Author

That fixes it well enough for me! Thank you for spending so much of your time chasing this down :)

@cpsauer
Copy link
Contributor

cpsauer commented Aug 26, 2022

You're very welcome. Sorry it took so long.

You say 'well enough'. Is there something still broken? If so, I'd like to know!

Chris

@NomiChirps
Copy link
Contributor Author

NomiChirps commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants