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

Need a way to provide system defines and system includes when compiler querying is disabled (with compilerPath of ""). #6909

Open
Colengms opened this issue Feb 5, 2021 · 19 comments · Fixed by #8174
Labels
Feature: Configuration An issue related to configuring the extension or IntelliSense Feature Request Language Service
Milestone

Comments

@Colengms
Copy link
Collaborator

Colengms commented Feb 5, 2021

It's possible to disable compiler querying (and disable fallback to a different compiler on failure) by specifying a compilerPath of "". This also works in combination with a custom configuration provider or compile_commands.json, to prevent use of the compiler they indicate (in case it's not cl.exe, gcc, or a gcc variant we can query). However, the defines and includes from c_cpp_properties.json are not currently getting merged into the configuration in those cases. Since no compiler will be queried, it seems necessary to support a way for the user to provide the missing system includes and systems defines.

This issue tracks merging in the includes and defines from c_cpp_properties.json when a compilerPath of "" is used in combination with a custom configuration provider or compile_commands.json.

@Colengms Colengms self-assigned this Feb 5, 2021
@Colengms Colengms added Language Service Feature: Configuration An issue related to configuring the extension or IntelliSense labels Feb 5, 2021
@Colengms Colengms added this to To do in 1.3.0 via automation Feb 5, 2021
@Colengms Colengms added this to the 1.3.0 milestone Feb 5, 2021
@michelleangela
Copy link
Contributor

@Colengms
Copy link
Collaborator Author

Colengms commented Feb 5, 2021

I think the step there that needs to change is 1.4: If compilerPath is "", use an empty array for system include path and defines.

If the compilerPath is "", we won't get system includes or defines from a compiler, so we should use system include paths and defines provided in c_cpp_properties.json instead of using 'an empty array'. (This is needed when using compile_commands.json or a custom config provider. Otherwise, includes and defines from c_cpp_properties.json are being used).

@Colengms Colengms modified the milestones: 1.3.0, Triage Feb 5, 2021
@Colengms Colengms removed this from To do in 1.3.0 Feb 5, 2021
@github-actions
Copy link

github-actions bot commented Apr 7, 2021

This feature request is being closed due to insufficient upvotes. When enough upvotes are received, this issue will be eligible for our backlog.

@github-actions github-actions bot added the more votes needed Issues that have been postponed until more community members upvote it label Apr 7, 2021
@github-actions github-actions bot closed this as completed Apr 7, 2021
@gerhardol
Copy link

Can this be resurrected, it seem to be relative simple way to support other compilers than gcc/clang?
#6931 is maybe more general, but requires further format specifications while this is something I expect to be simple.

@sean-mcmanus sean-mcmanus reopened this Apr 8, 2021
@bobbrow
Copy link
Member

bobbrow commented Apr 8, 2021

I would have expected 1.4 to be the same behavior as 2.3 (from the documentation), though it is not written the same. Do we really not use the includePath and defines from c_cpp_properties.json for 1.4? I think the documentation and the behavior should be updated. This sounds more like a bug (which won't be auto-closed by the bot) than a feature request to me.

@Colengms
Copy link
Collaborator Author

Colengms commented Apr 8, 2021

@bobbrow I think the doc is accurate to the current behavior (and I assume was written later to articulate the current behavior, to make sense of it, as it's a rather complex matrix). Include path and defines from the base config are not merged in when a custom config (for a file or browse config) or entry from compile_commands.json are used. Rather, the base config is a fallback if the file is not present in compile_commands.json or the custom config provider does not provide a file config or any browse config. If you're saying this is a bug, are you saying the original intent was to merge includes and defines from the base config into these other configurations?

The fact that an empty compilerPath in the base config modifies the behavior of compiler_commands.json or custom configs, is actually unusual, as it impacts use of the base config for falling back to. So, it seems we have 2 conflicting design decisions here. If the intent is to use the base config for fallback, it's not clear to me that it's a good idea to always merge from it (or allow a blank compilerPath in it to modify the other configs).

@Colengms
Copy link
Collaborator Author

Colengms commented Apr 8, 2021

It actually wasn't until version 1.2.1 that an empty compilerPath in c_cpp_properties.json would override the compiler specified by a custom config provider. Previously, that behavior was isolated to use of compile_commands.json.

@github-actions github-actions bot modified the milestones: Triage, Backlog Apr 13, 2021
@github-actions github-actions bot removed the more votes needed Issues that have been postponed until more community members upvote it label Apr 13, 2021
@github-actions
Copy link

This feature request has received enough votes to be added to our backlog.

@gerhardol
Copy link

The documentation currently states that C_Cpp.default.systemIncludePath is used if set, but I did not get that working with cpptools 1.2.0 (so that may be a bug). Also system defines cannot be set(?), so this is not working regardless.
https://code.visualstudio.com/docs/cpp/customize-default-settings-cpp#_system-include-pathdefines-resolution-strategies
I am not clear how this should work also when reading the configuration also after we got a working setup...

To summarize what I would like to see - this was discussed some in the issues.
Support some way to support compilers without built-in support (not gcc or clang) requires some configuring:

  • Supply includes, flags for files. provider, compile_commands.json is possible or base config fallback
  • Supply system include paths and defines
  • To not try the compiler (no harm but uses CPU/licenses and adds error logs)

This is working except that if compile_commands.json is set, base config including system defines/paths are ignored.
If base config is something to fallback on if not supplied by the provider or compile_commands.json, this would work I believe.

The current workaround is to generate a superset of includes and defines in the base config.
If system defines/includes could be set, we could separate the configuration in c_cpp_properties.json (with system), compile_commands.json (generated from CMake) and a few settings in .code-workspace (that no longer need to be generated).

@gerhardol
Copy link

(I am still interested in seeing this in an upcoming release, not forgotten.)

@bobbrow
Copy link
Member

bobbrow commented Sep 20, 2021

I would have expected 1.4 to be the same behavior as 2.3 (from the documentation), though it is not written the same. Do we really not use the includePath and defines from c_cpp_properties.json for 1.4? I think the documentation and the behavior should be updated. This sounds more like a bug (which won't be auto-closed by the bot) than a feature request to me.

I may have been missing some context or read the doc wrong when I wrote that. I did not intend to say that the includePath and defines should be merged in when compileCommands or configurationProvider are in use.

With the PR that's out now though, I can see the value of allowing this for unsupported compilers. C_Cpp.default.systemIncludePath was supposed to address this but we never added the companion setting: C_Cpp.default.systemDefines. There may even be a json file format our code would support now due to the work that was done to support Linux in Visual Studio.

For the record, I think allowing compilerPath to be set to "" to turn off the probing was a mistake (my mistake). I think it would have been better to add a separate property to make it explicit. If we ever wanted to bump the c_cpp_properties.json version, that would be a worthy change to include in the "upgrade" logic.

@willson556
Copy link
Contributor

C_Cpp.default.systemIncludePath was supposed to address this but we never added the companion setting: C_Cpp.default.systemDefines.

Would it maybe make more sense to follow that pattern then and add systemDefines, systemIncludePath, and systemForcedInclude to c_cpp_properties.json? That might make more sense than the mergeConfigurations property I proposed.

I can also look at handling compile_commands.json — we’re using CMake Tools so that wasn’t a requirement for us.

@willson556
Copy link
Contributor

Digging into the code a little more this morning, it looks to me like the compile_commands.json file is accessed by the closed-source language server directly. I therefore can't extend this to compile_commands.json without Microsoft support.

Assuming this still isn't a priority for the Microsoft team, I think the current approach in #8174 may be the best approach; it at least provides a path where CMake Tools + IAR/Keil/etc can be used in VSCode with accurate intellisense.

@bobbrow
Copy link
Member

bobbrow commented Sep 21, 2021

I didn't think systemForcedInclude was a thing. Is it? I might have seen something like that with Arduino though it's been a long while. One of the reasons I think we held back on systemDefines is because this list can be huge and we didn't expect folks to get this 100% correct -- which can cause other problems. Ideally, we would add extensibility into the open source part of the extension to support the unrecognized compilers, but we haven't started work on that yet. Not all of these compilers are programmatically queryable, and some also support non-standard language extensions that we don't (e.g. Keil).

And yes, compile_commands.json is currently processed internally. We haven't started a discussion as to whether we could port that to TypeScript and make it open source.

@willson556
Copy link
Contributor

willson556 commented Sep 23, 2021

One of the reasons I think we held back on systemDefines is because this list can be huge and we didn't expect folks to get this 100% correct -- which can cause other problems

At least for IAR, generating some files suitable for a systemForcedInclude is straightforward. The first includes all the system defines and can be generated by running the following command:

iccarm.exe --IDE3 --NCG <a dummy input file> --predef_macros iar_predefined_macros.h <compile flags>

This generates a header file with all the system defines- see https://gist.github.com/willson556/3d36c21100a473f75f2e87d0398188ff for a sample.

The other trick is to use the contents of <IAR_INSTALL>/arm/config/syntax_icc.cfg, which has contents like:

__arm
__thumb
__interwork
__swi
__irq
__fiq
__ramfunc
__no_init
__root
__nested
__task
__noreturn
__absolute 
__big_endian 
__little_endian 
__packed 
__stackless 
__weak 
__nounwind

and generating a header file like:

#define __arm
#define __thumb
#define __interwork
#define __swi
#define __irq
#define __fiq
#define  __ramfunc
#define __no_init
#define __root
#define __nested
#define __task
#define __noreturn
#define __absolute
#define __big_endian
#define __little_endian
#define __packed
#define __stackless
#define __weak
#define __WEAK
#define __nounwind
#define __intrinsic

This lets intellisense ignore the IAR language extensions.

Not sure what the situation looks like for Keil and other unsupported compilers, but at least for IAR there's a reasonable path for the user (or in the future cpptools) to generate a reasonable list of defines.

@willson556
Copy link
Contributor

To answer the other questions:

I didn't think systemForcedInclude was a thing. Is it?

It's not, just a hypothetical that could be useful for the above use case.

Ideally, we would add extensibility into the open source part of the extension to support the unrecognized compilers, but we haven't started work on that yet.
...
And yes, compile_commands.json is currently processed internally. We haven't started a discussion as to whether we could port that to TypeScript and make it open source.

Performance issues or other potential blockers notwithstanding, it seems like having as much of the "configuration" logic in the open-source part of the extension as possible would be advantageous. If there was a good structure in place for adding compilers, I'd expect the community would be able to step in and support the various niche/proprietary compilers that are used in embedded development rather than Microsoft needing to procure licenses, etc for all of them.

@bobbrow
Copy link
Member

bobbrow commented Sep 23, 2021

Performance issues or other potential blockers notwithstanding, it seems like having as much of the "configuration" logic in the open-source part of the extension as possible would be advantageous. If there was a good structure in place for adding compilers, I'd expect the community would be able to step in and support the various niche/proprietary compilers that are used in embedded development rather than Microsoft needing to procure licenses, etc for all of them.

This is the goal of #6931

For your example, I would think we'd implement it without forced includes, but the end result would be the same: support as many compilers as we can, and let the community help where possible.

@Colengms
Copy link
Collaborator Author

Colengms commented Oct 5, 2021

This issue is still tracking support for providing system includes and defines when using compile_commands.json, which is not addressed by #8174 .

@tolezk
Copy link

tolezk commented Mar 26, 2022

Just upvoting this as a vital feature. This would let take a number workarounds in game.

@sean-mcmanus sean-mcmanus modified the milestones: Backlog, On Deck Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Configuration An issue related to configuring the extension or IntelliSense Feature Request Language Service
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants