[PATCH] D154903: clangd: Provide the resource dir via environment variable

Sat Jul 22 14:52:39 PDT 2023

madscientist added a comment.

Sorry for the delay in replying.  I'll try to respond to the various points you made:

- I don't quite understand the first bullet, "more ways to initialize the resource dir".  As far as I can see there's only one place it's set.  If my change is using a "non-official" alternative to obtaining the resource dir then certainly I would be happy to change my code to fix that: really the goal of my patch is to ensure that there is one single way to find this directory and this is //preserved// by telling the compiler driver explicitly where it is, rather than having multiple places attempting to figure it out for themselves.
- If the resource-dir were to become built in then I would imagine that this variable would be changed to either be empty or (preferable) contain a special token such as "builtin" so that the compiler driver could know this.  How this would or could work would depend greatly on exactly how the built-in facilities were implemented, but if for example all that's needed is to avoid adding GCC's intrinsics headers to the list, then the compiler driver that saw a `CLANGD_RESOURCE_DIR` value of `builtin` (for example) would remove the GCC intrinsics directory completely, rather than replacing it with the clangd resource directory.

If using the built-in headers from one compiler with a different compiler is fated to break in mysterious ways, does that mean you feel it's not useful to try to use clangd in any project which doesn't use clang as its compiler?  I hope that this is not the case since (although the idea has been raised on their mailing lists) there is not much work going on in GCC to create a GCC-based LSP server.  Although I do agree that there are no guarantees, in reality clangd works very well in my GCC-based project if I replace the GCC intrinsics directory with the clangd resource directory when the compiler driver generates its list of built-in locations.

Regarding alternative solutions:

- My environment is cross-platform between GNU/Linux (with various distributions and on various platforms such as x86_64, arm64, and power9 and power10), MacOS on both x86_64 and m1/m2, and Windows (10+) on x86_64.  So relying on things like /proc/$PPID/exe (or even bash!) is not ideal.
- Your solution requires me to know implicitly where (compared to the clangd binary) the resource directory lives; that seems less reliable than having clangd tell me.  I don't quite understand the reassurance that these implementation details are stable enough to assume to be "fact", compared with the concern in the first bullet above about how they might change.

Your solution also assumes there is only one version of clang installed on the system; in fact my system has multiple versions so `.../lib/clang/*/include` expands to multiple paths which parses incorrectly (clangd assumes one directory per line).  Of course I could have my compiler driver run `clangd -version` and parse the output to find the right version; my resource directory is `/usr/lib/clang/16` but the version is `16.0.0`... it is do-able but it just seems increasingly hairy.

At the moment I don't have a different compiler driver for clangd than the standard compiler wrapper script that is provided via `compile_commands.json` although of course I could create one and change my `.clangd` to point to it.  I was going to simply modify my compiler wrapper to DTRT if it discovered it was being invoked with `-v` and `CLANGD_RESOURCE_DIR` was set.

Also, on one of my systems clangd is apparently configured to use posix_spawn() to invoke the compiler driver, which does not work with shell scripts.  But that just seems like a bug somewhere.  I get:

  E[17:23:24.164] System include extraction: driver execution failed with return code: -1 - 'posix_spawn failed: Exec format error'. Args: [/home/pds/src/clangd/bin/qdriver.sh -E -x c++ - -v]

although running that command from the shell prompt works fine.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154903/new/

https://reviews.llvm.org/D154903