nridge added a comment.
A user on Discord just ran into this
<https://discord.com/channels/636084430946959380/649134148723802113/1103941124642586664>
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?
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:340
+ // is not installed.
+ if (Lang == "objective-c++-header") {
+ Lang = "c++-header";
----------------
nridge wrote:
> kadircet wrote:
> > nridge wrote:
> > > kadircet wrote:
> > > > 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?
> > > > 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](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > 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).
> > > However, as mentioned
> > > [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/MinGW.cpp).
> > > However, as mentioned
> > > [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > 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?
> > > However, as mentioned
> > > [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > 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?
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:340
+ // is not installed.
+ if (Lang == "objective-c++-header") {
+ Lang = "c++-header";
----------------
nridge wrote:
> nridge wrote:
> > kadircet wrote:
> > > nridge wrote:
> > > > kadircet wrote:
> > > > > 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?
> > > > > 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](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > > 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).
> > > > However, as mentioned
> > > > [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > > 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](https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/MinGW.cpp).
> > > > However, as mentioned
> > > > [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > > 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?
> > > > However, as mentioned
> > > > [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437),
> > > > 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?
>
>
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](https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/MinGW.cpp).
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147905/new/
https://reviews.llvm.org/D147905
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
