microsoft / vscode-cmake-tools

CMake integration in Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=vector-of-bool.cmake-tools
MIT License
1.46k stars 450 forks source link

In custom configuration data, indicate which sources are associated with the active build target #1875

Open HardCoreCodin opened 3 years ago

HardCoreCodin commented 3 years ago

Bug type: Language Service

Describe the bug

A single C codebase that has 2 CMake targets/proejcts defined for it: One using CUDA and another not using it (plain C). The same codebase is compiled to either of these depending on which target is current/active. CUDA-related portions are within #ifdef __CUDACC__ blocks. Compilation succeeds on both targets, producing working executables. But the editing experience is not adjusting properly. Switchin back and forth between targets doesn't inform the syntax highlighting and code anlysis of a CUDA target being enabled/disabled. It always just debahves as if the projects aren't using CUDA. There is a .cu file who's #include is under an #ifdef __CUDACC__ block, and when opening it even while the CUDA target is active, none of the CUDA-specific syntax is considered properly, so is full of errors being reported visually - even though it compiles and runs just fine.

Steps to reproduce

  1. Start a new CUDA CMake project'
  2. Create a code.h file with a #ifdef __CUDACC__ block declaring a __constant__ variable.
  3. Create a cuda_code.cu file and include it into code.h under that #ifdef __CUDACC__, and add some CUDA-specific code in it.
  4. Include code.h in main.cu (Add #include code.h).
  5. Duplicate main.cu and rename it's file extension to just .c
  6. Duplicate the project definition in the CMakeLists.txt file, renaming it to something else and making it a non-CUDA target/project (remove the CUDA term in the project() directive, as well as any target properties such as CUDA_SEPARABLE_COMPILATION), then change it's target executable to be main.c insteaf of main.cu.
  7. After CMake reconfigures, select the CUDA-enabled target as the active one, and open cuda_code.cu - see how the syntax highlighting isn't working. Also open code.h and see how the #ifdef __CUDACC__ block is greyed out.

Expected behavior The code analysis and syntax highlighting should get enabled and disabled appropriately when switchin between the CUDA vs. the non-CUDA targets. Note: See attached animated-gif

Code sample and logs

#ifdef __CUDACC__
    #ifndef NDEBUG
        #define gpuErrchk(ans) { gpuAssert((ans), __FILE__, __LINE__); }
        inline void gpuAssert(cudaError_t code, const char *file, int line, bool abort=true) {
            if (code != cudaSuccess) {
                fprintf(stderr,"GPUassert: %s %s %d\n", cudaGetErrorString(code), file, line);
                if (abort) exit(code);
            }
        }
    #else
        #define gpuErrchk(ans) ans
    #endif
#endif
-------- Diagnostics - 5/12/2021, 7:11:28 PM
Version: 1.4.0-insiders
Current Configuration:
{
    "name": "Win32",
    "includePath": [
        "${workspaceFolder}/**"
    ],
    "defines": [
        "_DEBUG",
        "UNICODE",
        "_UNICODE"
    ],
    "windowsSdkVersion": "10.0.19041.0",
    "compilerPath": "C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.28.29910/bin/Hostx64/x64/cl.exe",
    "cStandard": "c17",
    "cppStandard": "c++17",
    "intelliSenseMode": "windows-msvc-x64",
    "configurationProvider": "ms-vscode.cmake-tools",
    "compilerArgs": [],
    "intelliSenseModeIsExplicit": true,
    "cStandardIsExplicit": true,
    "cppStandardIsExplicit": true,
    "compilerPathIsExplicit": true,
    "browse": {
        "path": [
            "${workspaceFolder}/**"
        ],
        "limitSymbolsToIncludedHeaders": true
    }
}
Custom browse configuration: 
{
    "browsePath": [
        "d:/code/c/raytracerxpu"
    ],
    "standard": "c++14",
    "compilerPath": "c:/program files (x86)/microsoft visual studio/2019/community/vc/tools/llvm/x64/bin/clang-cl.exe",
    "compilerArgs": [
        "-D_WINDOWS",
        "-Xcompiler=\" /GR /EHsc\"",
        "-Xcompiler=\"-O2 -Ob2\"",
        "-DNDEBUG",
        "--generate-code=arch=compute_52,code=[compute_52,sm_52]",
        "-Xcompiler=-MD",
        "-std=c++14"
    ]
}
Custom configurations:
[ D:\Code\C\RayTracerXPU\main.c ]
{
    "defines": [
        "WIN32",
        "_WINDOWS",
        "NDEBUG"
    ],
    "includePath": [],
    "compilerPath": "c:/program files (x86)/microsoft visual studio/2019/community/vc/tools/llvm/x64/bin/clang-cl.exe",
    "compilerArgs": [
        "/DWIN32",
        "/D_WINDOWS",
        "/O2",
        "/Ob2",
        "/DNDEBUG",
        "-MD"
    ]
}
Translation Unit Mappings:
[ D:\Code\C\RayTracerXPU\main.c ]:
    D:\CODE\C\RAYTRACERXPU\LIB\RENDER\RAYTRACER.H
    D:\CODE\C\RAYTRACERXPU\LIB\CORE\TYPES.H
Translation Unit Configurations:
[ D:\Code\C\RayTracerXPU\main.c ]:
    Process ID: 4504
    Memory Usage: 64 MB
    Compiler Path: c:/program files (x86)/microsoft visual studio/2019/community/vc/tools/llvm/x64/bin/clang-cl.exe
    Includes:
        C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\COMMUNITY\VC\TOOLS\LLVM\X64\LIB\CLANG\11.0.0\INCLUDE
        C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\COMMUNITY\VC\TOOLS\MSVC\14.28.29910\INCLUDE
        C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\COMMUNITY\VC\TOOLS\MSVC\14.28.29910\ATLMFC\INCLUDE
        C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.19041.0\UCRT
        C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.19041.0\SHARED
        C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.19041.0\UM
        C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.19041.0\WINRT
    Defines:
        WIN32
        _WINDOWS
        NDEBUG
        WIN32
        _WINDOWS
        NDEBUG
    Standard Version: c17
    IntelliSense Mode: windows-clang-x64
    Other Flags:
        --ms_compatibility
        --microsoft_version=1929
        --ms_extensions
        --clang
        --clang_version=110000
Total Memory Usage: 64 MB

------- Workspace parsing diagnostics -------
Number of files discovered (not excluded): 4161

Screenshots VSCodeCUDA

Additional context I've also tried different settings (without success) such as:

    "C_Cpp.intelliSenseEngine": "Tag Parser",
    "C_Cpp.default.intelliSenseMode": "windows-clang-x64",
    "C_Cpp.default.configurationProvider": "ms-vscode.cmake-tools",

Setting to "Tag Parser" has the opposite effect: CUDA syntax seems to always be on, even when the non-CUDA project is active.

sean-mcmanus commented 3 years ago

Your logging indicates IntelliSense is running on main.c and not main.cu. Does the bug repro if you don't open main.c and instead just open main.cu?

I don't think our code that determines which TU a header belongs in is able to change its TU based on conditional compilation, i.e. cuda_code.cu and code.h are probably getting associated with the main.c. I can check later...

Using "Tag Parser" bypasses IntelliSense, i.e. no regions are inactive.

HardCoreCodin commented 3 years ago

Yes, I saw that, and was confused by it as well - what exactly do you mean by "don't open main.c"? What do you mean by "open"? Do you mean to say that intellisense makes descisions on how to interpret code based on what files are being viewed (as in, have tab open for editing them)? That sounds super dodgy..

There are no TUs - this is technically a "unity build" - so each CMake target has a only single TU - a single source code file is being compiled for that target - the rest of the files are all header files technically (though they contain actual code, not declerations).

To illustrate, this is what a CMakeLists.txt would look like for the repro-example:

project(maincu CUDA)
add_executable(maincu main.cu)

project(mainc )
add_executable(mainc main.c)

Each target has a single TU.

Switching between targets is a supported feature of CMake Tools (also made by Microsoft). Saying that the C/C++ extension doesn't support switching CMake targets is essentially saying it doesn't support CMake Tools.

sean-mcmanus commented 3 years ago

I mean don't open the "main.c" file. CMake Tools sends us configuration info based on the file that is open, so if you open main.c then it will send the configuration info for that file, but I don't remember if the configuration provider will send us info on which source file an open header file belongs to (I could look it up later or @Colengms might know), so if you have main.c open it could be associating the header file with that instead of the main.cu.

But it's possible there's a CMake Tools bug in that maybe it shouldn't be sending configuration info for main.c when the maincu target is used.

I'm not too familiar with how target switching should work (maybe @andreeis from the CMake Tools knows) or I could investigate later...

HardCoreCodin commented 3 years ago

The build configuration (CMake or snything else) defines __CUDACC__ (as well as tons of other CUDA-related stuff). The "active" build configuration determines it's current state, so the syntax highlighting and code analysis should reflect that. If that's not how it works, this should be considered a bug (in any sane way of looking at it). Regardless of what configures the build (CMake or anything else).

The CMake Tools documentation, seems to suggest that this is how it should be working: https://vector-of-bool.github.io/docs/vscode-cmake-tools/how_do_i.html#set-up-include-paths-for-c-intellisense

Colengms commented 3 years ago

Hi @HardCoreCodin . Could you enable the following:

    "C_Cpp.loggingLevel": "Debug",

... and provide the output of the C/C++ channel leading up to the issue?

A source file for which a custom configuration has already been provided by a config provider (i.e. a source file that was previously opened, or associated with another header that was previously opened) receives a higher priority when choosing a source file to associate a header with. Since it looks like a custom configuration was received for main.c, that appears to the C/C++ extension as a good choice to associate with the header (though will not be CUDA, as CUDA is not supported in C).

If CMake Tools is providing a custom configuration for a main.c while it is not associated with the active target (or otherwise shouldn't be associated with the current configuration), that would seem to be an issue with CMake Tools. Or, if you are changing the current target and the C/C++ extension is not discarding the custom configuration previously associated with the file (so, using a stale configuration), that could be an issue with the C/C++ extension, or perhaps could be due to not receiving a notification from CMake Tools to discard old configurations when the target is changed.

The output of the C/C++ output channel with debug logging enabled would help determine if either of the above issues could be the cause.

If a file is successfully configured for CUDA, you should see --cuda in the Other Flags section of the Log Diagnostics output.

HardCoreCodin commented 3 years ago

@Colengms Note that I'm using a "unity build" kind of setup: It's a single TU with a single source-file being compiled per-target - and it just #includes-in code from header files. So the semantics of header vs. source are actually kind of reversed here: My "source file" (.c/.cu) only contains #include directives for header files, and no actual code by itself. My "header files" (.h) contain actual code, not just declerations, as well as further #include directives for more code.

So this whole story there aboud associating between them doesn't make much sense in this setup. If anything, the header files should be choosing which source file to be associated with (not the other way around), based on which source file #includeed them.

And there's ever only going to be a single source file at any given time - the one that the current CMake target is compiling. The other one should be considered outside the project, as it's not referenced at all by the current target (configuration). Switching between targets using CMake Tools switches which source file is being compiled, and so in this setup it should make all header files query again which source file (ultimately) they are now being #includeed from.

Does this make sense?

That's how I expected it to work, and that's how it seems to work in CLion. It all works just fine there, with this exact same setup - with the syntax-highlighting/code-analysis switching on/off back and forth as I switch between targets. The UX experience I'm getting there is exactly as shown in the animated gif I've attached here.

When setting the non-cuda target as "active", all the code in all the header files, is being #includeed ultimately from the .c source file, and should thus be interpreted by the extension as being just plain C code (as that's how the compiler would see them in this compilation configuration). So __CUDACC__ should be unset, which means that all my CUDA-specific code there should appear greyed-out and shouldn't be attempted to be interpreted by the extension.

When setting the cuda target as "active", all the code in all the header files, is now being #includeed ultimately from the .cu source file, and should thus be interpreted by the extension as being "CUDA supportive" (as that's how the compiler would see them in this compilation configuration). So __CUDACC__ should be set, which means that all my CUDA-specific code there should be active and should be interpreted properly by the extension.

On a side note: I shoudn't be expected to answer questions here based on deep knowledge of the design of the extension. You're getting me into the weeds of the design, using terms I'm not suppose to be familiar with: What's an "output channel"? What's a "config provider"? What's a "custom configutation"? (why is it "custom"? As opposed to what?) I'm sure these term's semantics seem obvious to you, but they're not.

I'm just reporting my use case and perceived-vs.-expected experiences - the rest is details I shouldn't be expected to dive into.

sean-mcmanus commented 3 years ago

Output channel refers to: https://code.visualstudio.com/docs/cpp/enable-logging-cpp Config provider and custom configuration both refer to the CMake Tools extension, set by the configurationProvider setting (well, the configuration info that CMake Tools sends us).

i.e. our C/C++ logging shows stuff like:

image

Colengms commented 3 years ago

@HardCoreCodin Custom config providers such as CMake Tools, as well as compile_commands.json, provide build information (effectively) in the form of compile command lines. CMake Tools would not be tracking configuration information for header files, as it's likely not aware of them, as they are included later by source files only when compiled. The C/C++ extension tries to choose an appropriate source file for a header, as sources are what build systems such as CMake and compile_commands.json are aware of. CMake Tools can only provide a configuration (compiler args) for a source file CMake would pass directly to a compile command line.

The primary issue you appear to be seeing is that main.c is being chosen as the source file for that header. I suspect that if main.cu were chosen, it would get successfully configured for CUDA. The C/C++ output log might help us understand what is going on here.

Whether or not __CUDA__ is defined by the build system does not control whether the C/C++ extension considers a file to be CUDA. If using a custom configuration provider, or compile_commands.json, (scenarios in which entire compile command lines are present), the file is determined to be CUDA if passed to a compile command line that invokes the CUDA compiler nvcc. The C/C++ extension actually needs to invoke nvcc to determine what configuration information (compiler args) it will pass to the 'host' compiler.

The issue doesn't appear to have to do with CUDA, per se. It has to do with which source file is getting associated with your header. "main.c" will not be considered CUDA unless it's been configured to be passed to nvcc by your CMake configuration.

Some additional information about how a source file is chosen for a header is here: https://github.com/microsoft/vscode-cpptools/issues/7396#issuecomment-823745633

HardCoreCodin commented 3 years ago

@sean-mcmanus @Colengms I've now set the settings to this:

    "cmake.configureOnOpen": true,
    "C_Cpp.autocompleteAddParentheses": true,
    "C_Cpp.updateChannel": "Insiders",
    "C_Cpp.loggingLevel": "Debug",
    "C_Cpp.default.intelliSenseMode": "windows-clang-x64",
    "C_Cpp.default.configurationProvider": "ms-vscode.cmake-tools"

Then, restarting VS code and looking at the C++ output (channel?) shows this: log_when_opening_VSCode.txt

Then, opening my types.h header file, this is being added to the output: log_when_opening_type_header_file.txt

It's already displaying wrongly, as the current/active CMake target is the CUDA one, but the __CUDACC__-guarded block is displayed as greyed out.

Then, when I switch to the non-CUDA target, this gets added to the output: log_when_type_header_file_is_open_and_switching_to_non_CUDA_target.txt

Lastly, switching back from non-cuda target to the cuda target, this gets added: log_when_type_header_file_is_open_and_switching_from_non_CUDA_target_to_CUDA_target.txt

Does this help?

Colengms commented 3 years ago

Thanks @HardCoreCodin . That helped rule out a few things.

If a file is valid for any project, CMake Tools appears to return a custom configuration for it, regardless of what the current build target is set to with CMake: Set Build Target. It's not clear to me what the correct behavior should be here. Some users may expect to be able to work with files other than those associated with the currently selected build target. However, if custom configurations are provided based on all projects/targets, header/source associations from all projects would appear to be equally suitable to the C/C++ Extension.

Perhaps we could add something to the configuration provider API to help facilitate header source selection, which would give the config provider an opportunity to identify which candidate source file should be preferred. CMake Tools could indicate that sources associated with the current build target are preferred.

As a temporary work around, if you open main.cu before opening the header, that should result in main.cu being a considered a better match for the header (since it's already set up). If that does not work as expected, please let me know.

HardCoreCodin commented 3 years ago

@Colengms I just tried it now - still same behavior:

log_when_CUDA_target_is_active_and_main_cu_is_open_then_opening_types_header.txt

I'm not sure why you frame it as though there's some ambiguity here, there isn't - CLion gets it right every time. The development experience is expected to reflect the currently active build configuration. That's just simple sanity. Anyone expecting any other behaviour, is obviously not thinking about it clrearly.

Some users may expect to be able to work with files other than those associated with the currently selected build target.

Who are these hypothetical users you're conceptualizing? If they want to develop for a different target, they can just switch to it - it's literally 2 mouse clicks...

sean-mcmanus commented 3 years ago

Your logging looks like IntelliSense should be working:

  define: __CUDACC__
  define: __NVCC__
  define: _WINDOWS
  define: NDEBUG
  define: __CUDACC_VER_MAJOR__=11
  define: __CUDACC_VER_MINOR__=3
  define: __CUDACC_VER_BUILD__=58
  define: __CUDA_API_VER_MAJOR__=11
  define: __CUDA_API_VER_MINOR__=3
  preinclude: C:\PROGRAM FILES\NVIDIA GPU COMPUTING TOOLKIT\CUDA\V11.3\INCLUDE\CUDA_RUNTIME.H
  other: --rtti
  other: --cuda
  stdver: ms_c++14
  intelliSenseMode: windows-msvc-x64
Queueing IntelliSense update for files in translation unit of: D:\CODE\C\RAYTRACERXPU\MAIN.CU

What are you seeing not working? __CUDACC__ isn't defined? We have a bug where macros can get undefined if duplicate includes are used -- it should be fixed in our 1.4.0-insiders2 release next week (https://github.com/microsoft/vscode-cpptools/issues/7270).

Colengms commented 3 years ago

@HardCoreCodin Your log indicates that CUDA support is enabled for the file. At the bottom right of the window, do you should see a field indicating the file type?

image

It "CUDA C++" is not selected, perhaps it was changed from default. Does it work if you click there and switch it back? This language ID is used to enable syntax colorization, which actually comes from VS Code itself.

Otherwise, if that is correct, what specific functionality are you using to determine that CUDA support is not working for you? If you're getting a squiggle with a file that Log Diagnostics indicates is using --cuda, it's possible there is an issue with IntelliSense parser that could be addressed.

Who are these hypothetical users you're conceptualizing?

Myself, my team, and users of other projects I have worked on as well. Very often, multiple projects are definied within a directory structure, and all of those projects are intended to be built and linked together.

Consider opening an issue in the CMake Tools repro suggesting that it decline to provide custom configurations for all but the currently active target. Since that would be a change to CMake Tools and not the C/C++ Extension, I'd suggest discussing the merits there.

Colengms commented 3 years ago

I do see an issue here with language ID association of header files. When a header file is associated with a CUDA source file, the language ID of the header remains "C++". It should be changed to reflect the source file it gets associated with. I'll include a fix for that as part of https://github.com/microsoft/vscode-cpptools/issues/7359 . As a work-around, you can change the language ID to CUDA C++ by clicking on the field and selecting it. This only impacts syntax highlighting, and wouldn't cause squiggles.

HardCoreCodin commented 3 years ago

Actually, I miss-spoke - the CUDA section is not greyed out now, just the debug section: VSCode_CUDA_CPP

Though it does still show C++ down there at the corner, not CUDA C++. Also it's now stuck in CUDA-mode even when I switch back to the non-CUDA target

Very often, multiple projects are definied within a directory structure, and all of those projects are intended to be built and linked together.

@Colengms Are you refering to a DAG of library projects? If only a few of them are CUDA-enabled, wouldn't having more non-CUDA ones actually make it more likely that your development experience would appear broken? I can't see how that would conflict with current-target-based syntax-highlighting/code-analysis (CUDA vs. non-CUDA). If anything, I can see it as being an even stronger case in favour of it.

HardCoreCodin commented 3 years ago

Consider opening an issue in the CMake Tools repro suggesting that it decline to provide custom configurations for all but the currently active target.

@Colengms @sean-mcmanus If that's a coordination issue, given how it involvs 2 extensions owned by the same company, the teams working on those should be collaborating on the specifics of that coordination - it shouldn't be driven by users.

sean-mcmanus commented 3 years ago

@andreeis Summary is: decline to provide custom configurations for all but the currently active target.

Colengms commented 3 years ago

I'd actually prefer to address this with https://github.com/microsoft/vscode-cpptools/issues/7526 . That would be a change to the configuration provider API to a) provide all configurations up-front (this is needed for Unification, ultimately), and b) indicate which source files are in the currently active project - so they can be preferred when searching for a header for a source file. I do not believe that declining to provide configurations for inactive targets would be a good idea.

HardCoreCodin commented 3 years ago

I do not believe that declining to provide configurations for inactive targets would be a good idea. @Colengms I was about to say the same thing - there should probably be a way to just choose the active one out of the given set. But that's a detail of the coordination, so I removed my saying that (a 'not my problem' kind of a thing).

sean-mcmanus commented 3 years ago

@Colengms Should the title be changed?

github-actions[bot] commented 11 months ago

This issue is now marked as 'stale-old' due to there being no activity on it for the past 720 days. Unless the 'stale-old' label is removed or the issue is commented on, this will be remain open for at least 14 days and then it may be closed. If you would like to make this issue exempt from getting stale, please add the 'stale-exempt' label.