Open CookiePLMonster opened 8 months ago
At the most basic level, that doc is oversimplified. We should update it. The strategy for resolving the system includes is documented here: https://code.visualstudio.com/docs/cpp/customize-default-settings-cpp#_system-include-pathdefines-resolution-strategies
For compile_commands.json we allow for users to specify compilerPath
to pick a different compiler to query in the event that the compiler listed in compile_commands.json is not queryable.
However, if you are specifying a compiler in compilerPath
and it's throwing away important options from the compiler command line that are listed in compile_commands.json, we treat that as a bug in the extension.
However, if you are specifying a compiler in
compilerPath
and it's throwing away important options from the compiler command line that are listed in compile_commands.json, we treat that as a bug in the extension.
That matches my observations, yes. I included c_cpp_properties.json
but I forgot to include my current compile_commands.json
:
Based on this setup, I expect "compilerPath"
to be used for files not present in compiler_commands.json
, otherwise that JSON should take precedence. Instead, I am seeing that the JSON is completely ignored, as per the logs included above, and the result is that IntelliSense is lacking all the -mcpu=
-inferred defines.
It seems like we could add a setting to control the precedence...but the "pending" IntelliSense configuration changes would make this a non-issue.
It seems like we could add a setting to control the precedence...
Is the current behaviour not a defect then? If I understand the docs correctly, compile_commands.json
should have took precedence over "compilerPath"
in this instance.
@sean-mcmanus If this is intended behaviour, how do you suggest to move forward? Update project's c_cpp_properties.json
to have an empty compiler path and live with IntelliSense errors until the first compilation builds the commands JSON? I would consider this an acceptable tradeoff.
The current behavior of the compilerPath overriding the compiler in compile_commands.json
is expected and not a defect. We would need to add a setting to disable it (as @sean-mcmanus mentioned).
We also do not currently handle the -mcpu=
option. Are you getting incorrect results without it? We don't do anything special for different cpu types. We're currently limited to x86/x64/arm/arm64 support. If cortex "mostly" aligns with one of those, you might be able to set the intelliSenseMode
property in your configuration to force a specific architecture that gives you the best results.
We also do not currently handle the
-mcpu=
option. Are you getting incorrect results without it? We don't do anything special for different cpu types. We're currently limited to x86/x64/arm/arm64 support. If cortex "mostly" aligns with one of those, you might be able to set theintelliSenseMode
property in your configuration to force a specific architecture that gives you the best results.
I believe gcc handles it for you - with "compilerPath"
set to ""
, vscode invokes gcc with the parameters given, including -mcpu
- please see the bits of the language server logs I included in the initial post.
The current behavior of the compilerPath overriding the compiler in compile_commands.json is expected and not a defect. We would need to add a setting to disable it (as @sean-mcmanus mentioned).
Therefore, to sum my issue up:
compile_commands.json
is parsed as expected and I get the correct results for my specific ARM CPU."compilerPath"'
needs to be set to ""
in c_cpp_properties.json
for this feature to work correctly.With those in mind, I would say it's 50/50 on the c_cpp_properties.json
configuration not realizing "compilerPath"
overrides compile_commands.json
, but also in my defense the docs really don't make that clear:
If there is a matching entry in compile_commands.json for a file open in the editor, that command line will be used to configure IntelliSense for that file, instead of the other fields of c_cpp_properties.json.
By "the other fields", I understood that this includes 'compilerPath"
. Since it doesn't, in my opinion the docs should clarify this.
As for this issue - I agree that overriding the compiler can be considered not a defect, but it is also overriding compiler commands. I believe this part is wrong.
EDIT:
Just to make it entirely clear, let me visualise that -mcpu
truly is used:
"compilerPath"
set explicitly - parameters from compile_commands.json
are ignored, therefore IntelliSense doesn't know what CPU I am targetting:
"compilerPath"
set to ""
- compile_commands.json
is parsed by IntelliSense that queries GCC for supported defines, therefore IntelliSense knows exactly what CPU I am targetting:
Setting compilerPath
to ""
disables the compiler querying when used with includePath
and defines
(i.e. for source files not existing in compile_commands.json
). So it's often better to just delete compilerPath
from your configuration if you are not using it to override the compiler in compile_commands.json
.
I was just talking to @Colengms and he said that we shouldn't be filtering out the -mcpu
option from the query because it's not in our list of options to remove (because they mess up the query). So I'm not sure yet why it's not showing up in your query.
So it's often better to just delete
compilerPath
from your configuration if you are not using it to override the compiler incompile_commands.json
.
This results in IntelliSense trying to use cl.exe
:
[1/24/2024, 11:40:34 PM] For C source files, the cStandard was changed from "gnu17" to "c17" based on compiler args and querying compilerPath: "cl.exe"
even though intelliSenseMode
is set to gcc-arm
, as I have showed in the first post.
I was just talking to @Colengms and he said that we shouldn't be filtering out the
-mcpu
option from the query because it's not in our list of options to remove (because they mess up the query). So I'm not sure yet why it's not showing up in your query.
Do note that with compilerPath
set to a non-empty path, my language server logs don't mention compile_commands.json
at all - they do, however, mention it when that path is ""
.
Hi @CookiePLMonster .
Could you enable "C_Cpp.loggingLevel": "Debug"
and give us more of the output from the C/C++ output panel leading up to the issue? Could you try this with the latest (insiders) version of the extension? I believe what you provided shows compiler queries for the base/default compiler, but additional invocations of the compiler should be occurring for compile command lines in your compile_commands.json
. Unless there is an issue matching the filename in compile_commands.json
, one of those logged command lines should refer to your -mcpu=
argument and should indicate a reason why that compiler query has failed. When a compiler query fails for a compile_commands.json
entry, the base/default configuration is used as a fallback. A failed compiler query could explain why you're seeing the base configuration used instead of the compiler and argument from the compile_commands.json
entry.
Based on this setup, I expect "compilerPath" to be used for files not present in compiler_commands.json, otherwise that JSON should take precedence
I believe the following documentation is missing one detail:
If there is a matching entry in compile_commands.json for a file open in the editor, that command line will be used to configure IntelliSense for that file, instead of the other fields of c_cpp_properties.json.
Currently, if there is a "compilerPath" explicitly specified in your configuration (in c_cpp_properties.json
, or C_Cpp.default.compilerPath
), it will be used instead of the compiler specified in compile_commands.json
, _when querying command lines from compile_commands.json
_. The args from compile_commands.json
should still be used, so you should see still an attempted compiler query in your log that refers to -mcpu=
. This 'feature' was intended as a work-around for users working with generated compile_commands.json
files which leverage a script or unsupported compiler, allowing them to arbitrary redirect the compiler query. This behavior isn't ideal, as it breaks 'fallback' from compile_commands.json
to the base configuration, if that fallback configuration should use a different compiler path. That appears to be the core of the issue you're reporting. I'd like to use this issue to track moving this 'special' value for overring compile_commands.json
compiler paths, to a new setting instead the current behavior, which seems to break base configuration fallback.
Currently, if there is a "compilerPath" explicitly specified in your configuration (in
c_cpp_properties.json
, orC_Cpp.default.compilerPath
), it will be used instead of the compiler specified incompile_commands.json
, _when querying command lines fromcompile_commands.json
_. The args fromcompile_commands.json
should still be used, so you should see still an attempted compiler query in your log that refers to-mcpu=
.
Please do note that the compile_commands.json
I attached contains only the "command"
parameter, the toolchain does not output "arguments"
. Therefore, if "compilerPath"
overrides the entire command and not just the first part of it, that'll be an issue. However, I have been told that other tools (incl. alternative C/C++ extensions) handle such cases fine, and it is also documented as supported by this extension.
@Colengms As requested, I ran the test on the latest pre-release version of the extension (v1.19.2
) and with the Debug logging level, C/C++ output panel (not C/C++ Diagnostics):
Based on this behaviour, I believe "compilerPath"
overrides the entire "command"
parameter instead of just the first part of it, or maybe it ignores compile_commands.json
entirely. The approach used by the toolchain I use is not recommended over "arguments"
, but it is allowed:
command: The compile command as a single shell-escaped string. Arguments may be shell quoted and escaped following platform conventions, with ‘"’ and ‘\’ being the only special characters. Shell expansion is not supported.
Either arguments or command is required. arguments is preferred, as shell (un)escaping is a possible source of errors.
I had a quick look at the toolchain and modifying it to emit "arguments"
instead of "command"
wouldn't be trivial - it already operates on an escaped and joined list of arguments so splitting them again would not be worth it. It would be better for the extension to correctly handle this (valid) use case instead.
Hi @CookiePLMonster
I do not see any failure to query a compiler in your logs, so we can rule that (and fallback scenarios) out.
Putting together a simple example of what you've described, I'm unable to reproduce the behavior we're seeing in your logs. There would seem to be something about your environment, workspace, or configuration that we may need to know to understand what's going on.
Are you using a single-root or multi-root workspace? I'm seeing a multitude of "didChangeCppProperties" messages in the logs you provided, which I don't see in my local repro. If you're using multiple roots, the configuration(s) of the other root(s) might be relevant.
Can you provide us with a stand-alone example, as a github repo or a zip file? I suspect this would not be difficult for us to address if we could repro locally.
Hey @Colengms, considering I was not even aware multi-root workspaces are a thing, I use a single-root one. Always opening the directory as a workspace.
Can you provide us with a stand-alone example, as a github repo or a zip file? I suspect this would not be difficult for us to address if we could repro locally.
The project I encountered this issue with is open source, standalone and self-contained - as in, it keeps the toolchain and all intermediates in its own repo directory only, not polluting the home directory or PATH. I suspect for this reason it should serve you well as a test case:
./fbt vscode_dist
. This sets up the VSCode workspace and downloads the toolchain for compilation. Worry not, it downloads those to a toolchain/
directory in the repo directory, not impacting your environment otherwise.[Debug] Build Firmware
task.With this set-up done, you may now observe the issue:
build/latest/compile_commands.json
has been created by the toolchain. Do note the command contains options like -mcpu=cortex-m4
that are supposed to impact the system defines IntelliSense sees..c
file mentioned in the JSON file. In my case, I went for furi/flipper.c
.__ARM_ARCH_7EM__
macro anywhere in the file. Notice IntelliSense claims it is undefined, yet (if you attempt to build) the compiler sees it as defined to 1
. This is because -mcpu=cortex-m4
implies an ARMv7E instruction set, which this system define signifies..vscode/c_cpp_properties.json
and set "compilerPath"
to ""
. Save the changes.furi/flipper.c
. Notice that IntelliSense immediately picks up on the existence on __ARM_ARCH_7EM__
and says Expands to: 1
. This is the intended behaviour.I hope those repro steps are workable for you.
Hi @CookiePLMonster
With those repro steps, if I remove the compilerPath
entry in the generated c_cpp_properties.json
, everything appears to work as expected. The issue would seem to be that, with the compilerPath
specified, it overrides the compile_commands.json
entry entirely, using both the compilerPath
and compilerArgs
from the base configuration (i.e. in c_cpp_properties.json
) instead of anything from compile_commands.json
.
You may be able to work around the issue by removing the compilePath
entry entirely. Or, if you need to be able to fall back to some base configuration for files not in c_cpp_properties.json
, you could copy a set of arguments from the compile_commands.json
into the compilerArgs
field of c_cpp_properties.json
. (Assuming the same args are sufficient to configure all files, that may eliminate the need to refer to the compile_commands.json
file at all. Also, the compilerArgs
field, as an array of args, should not contain any shell command-line escaping. If that's an issue, providing a complete compile command-line in the compilerPath
field, is also supported.)
I think the original intention was for compilerPath
to override only the compiler and not the arguments in compile_commands.json
. Though, instead of adjusting that behavior, I'd like to use this issue to track no longer allowing compilerPath
to override command lines in compile_commands.json
, in favor of using a separate settings value for that.
Also, I see this invalid entry present: "configurationProvider": "ms-vscode.cpptools"
Which you may want to remove also. The C/C++ Extension is not a provider of custom configurations to itself. This field would be for custom configuraiton providers such as CMake Tools, or Makefile Tools.
Hey @Colengms, your insights make sense to me, thank you! Just one note:
You may be able to work around the issue by removing the compilePath entry entirely.
Given that removing the line makes the extension fall back to cl.exe
(as I mentioned HERE), would setting the path to ""
not be preferable? This is what I proposed in the drafted PR to the linked repository and my tests have been successful.
would setting the path to "" not be preferable
That would result in no IntelliSense for files not found in compile_commands.json
. Assuming that all of the compile_commands.json
entries are indicating essentially the same arguments, you might try just pasting one of those command lines into the compilerPath
field. That may successfully configure all files appropriately, without even requiring use of the compile_commands.json
.
Assuming that all of the
compile_commands.json
entries are indicating essentially the same arguments, you might try just pasting one of those command lines into thecompilerPath
field. That may successfully configure all files appropriately, without even requiring use of thecompile_commands.json
.
I don't think that's a safe assumption as I've seen a different set of arguments for .c and .cpp files. I also don't have enough knowledge of this project's toolchain.
That would result in no IntelliSense for files not found in
compile_commands.json
.
No Intellisense at all or just not querying the compiler for defines? If it's the latter, I don't know how useful cl.exe's query be for this codebase.
Hi @CookiePLMonster .
No Intellisense at all or just not querying the compiler for defines
You're correct in that the IntelliSense engine would still provide results, but system headers are unlikely to be parsed correctly when predefined macros are not set appropriately. That usually results in IntelliSense being effectively broken.
I'm going to go ahead and correct the previously intended behavior of the compilerPath
override, to continue to use the arguments provided in compile_commands.json
. That should at least allow the original(?) approach, of specifying the same compiler in the compilePath
field, to enable fallback for files not in compile_commands.json
to use the (same) compiler in the base configuration. We expect to release 1.19.3 within days, and it should include this fix.
I'm going to go ahead and correct the previously intended behavior of the
compilerPath
override, to continue to use the arguments provided incompile_commands.json
.
This sounds perfect @Colengms, and effectively the exact change I was hoping to see when I opened this issue 👍
With this in mind, I am going to shelve my PR to the Flipper Zero repository (https://github.com/flipperdevices/flipperzero-firmware/pull/3394) and wait for 1.19.3 to release. That said, I think it is still going to be worth removing the "configurationProvider": "ms-vscode.cpptools"
entry you highlighted earlier as invalid.
Hey @Colengms, did the bug fix make it to 1.19.3? While the issue is not linked, this change log entry sounds related:
Fix an issue where use of an explicit
compilerPath
to override the compiler in acompile_commands.json
with also throw out the compiler arguments.
EDIT:
It's been fixed! I verified on a clear project and a regenerated vscode environment that IntelliSense now respects -mcpu
with the compilerPath
still set:
Yes, it was fixed in 1.19.3 (as your EDIT confirms). I have updated the project marking to reflect which release it went in. It was accidentally marked for 1.20.
Should this issue be closed then? Or do you wish to use it to track that "additional setting" that's mentioned in a revised issue title? Or keep it open until the fix hits a mainline version and not just a pre-release?
We usually keep issues open while the changes are only available in pre-release.
I think the original intention was for compilerPath to override only the compiler and not the arguments in compile_commands.json. Though, instead of adjusting that behavior, I'd like to use this issue to track no longer allowing compilerPath to override command lines in compile_commands.json, in favor of using a separate settings value for that.
@CookiePLMonster @bobbrow I had updated the title of this issue to track a better solution. Restoring the previous behavior is a temporary solution. My intent was not to consider this issue resolved until the solution in the title has been implemented. Please leave this issue open to track that better solution. (Or I'll need to create another one).
That's fine by me - the current behaviour is optimal already, and I'm fine with keeping this issue open.
Environment
Bug Summary and Steps to Reproduce
Bug Summary:
If both
"compilerPath"
and"compileCommands"
are present in"c_cpp_properties.json"
, the former seems to take precedence during IntelliSense analysis. This results in the command line of the file present incompile_commands.json
(e.g.-mcpu
) to be ignored, leading to missing defines. Removing"compilerPath"
results in the command line fromcompile_commands.json
being taken into the account.This behaviour goes directly against what c_cpp_properties.json reference documents:
Additionally, the intended behaviour (
compile_commands.json
overriding other options) was mentioned yesterday in another issue: https://github.com/microsoft/vscode-cpptools/issues/11880#issuecomment-1901003596.Defining
"compilerPath": "",
works as a workaround.Observed behaviour:
"compilerPath"
takes precedence over"compileCommands"
for populating system/architecture specific defines.Expected behaviour:
"compileCommands"
takes precedence over"compilerPath"
for populating system/architecture specific defines.Configuration and Logs
c_cpp_properties.json
``` { "configurations": [ { "name": "main", "intelliSenseMode": "gcc-arm", "compilerPath": "H:/dev/flipper-zero/.ufbt/toolchain/x86_64-windows/bin/arm-none-eabi-gcc.EXE", // This causes issues "compileCommands": "${workspaceFolder}/.vscode/compile_commands.json", "configurationProvider": "ms-vscode.cpptools", "cStandard": "gnu17", "cppStandard": "c++17" } ], "version": 4 } ```Language server logs ("compilerPath" absent)
``` Querying compiler's default target using command line: "H:\dev\flipper-zero\.ufbt\toolchain\x86_64-windows\bin\arm-none-eabi-gcc.EXE" -dumpmachine Compiler returned default target value: arm-none-eabi Compiler query command line: H:\dev\flipper-zero\.ufbt\toolchain\x86_64-windows\bin\arm-none-eabi-gcc.EXE -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16 -mthumb -Wall -Wextra -Wno-address-of-packed-member -Wredundant-decls -Wdouble-promotion -fdata-sections -ffunction-sections -fsingle-precision-constant -fno-math-errno -g -Os -mword-relocations -mlong-calls -fno-common -nostdlib -std=gnu17 -Wp,-v -E -dM -x c nul Attempting to get defaults from C compiler in compile_commands.json file: 'H:\dev\flipper-zero\.ufbt\toolchain\x86_64-windows\bin\arm-none-eabi-gcc.EXE' Querying compiler for default C++ language standard using command line: H:\dev\flipper-zero\.ufbt\toolchain\x86_64-windows\bin\arm-none-eabi-g++.EXE -x c++ -E -dM nul ```Language server logs ("compilerPath" present)
``` Querying compiler's default target using command line: "H:/dev/flipper-zero/.ufbt/toolchain/x86_64-windows/bin/arm-none-eabi-gcc.EXE" -dumpmachine Compiler returned default target value: arm-none-eabi Compiler query command line: H:/dev/flipper-zero/.ufbt/toolchain/x86_64-windows/bin/arm-none-eabi-gcc.EXE -std=c++17 -Wp,-v -E -dM -x c++ nul Attempting to get defaults from C++ compiler in "compilerPath" property: 'H:/dev/flipper-zero/.ufbt/toolchain/x86_64-windows/bin/arm-none-eabi-gcc.EXE' Compiler query command line: H:/dev/flipper-zero/.ufbt/toolchain/x86_64-windows/bin/arm-none-eabi-gcc.EXE -std=gnu17 -Wp,-v -E -dM -x c nul LSP: Message ignored due to no registered handler: $/setTrace ```Other Extensions
No response
Additional context
No response