microsoft / vscode-cpptools

Official repository for the Microsoft C/C++ extension for VS Code.
Other
5.54k stars 1.56k forks source link

New IntelliSense parser doesn't resolve recursive include paths #897

Closed keton closed 6 years ago

keton commented 7 years ago

I've just updated vscode-cpptools to July edition and enabled new IntelliSense mode by adding "C_Cpp.intelliSenseEngine": "Default" to settings.json. This caused a lot of "include file xx.h not found" errors on a previously correctly configured project.

VSCode still can find the files because pressing F12 on function and file names works but it seems that if include path is defined as recursive new IntelliSense engine is not using it.

Manually adding all the paths as non recursive fixes those errors but is not usable in my case. Since almost all paths in my project rely on this behavior I had to revert to tag parser for now.

Please advice is there any way to use new IntelliSense engine with recursive paths.

sean-mcmanus commented 7 years ago

The "previously correctly configured" project was only correctly configured for the Tag Parser's recursive browse.path. Making the includePath recursive is not technically valid, because the path is used for compilation instead of just finding C/C++ files for our symbol database. A compiler that accepted recursive include paths would not be able to handle files with the same name at different path locations due to the recursion, and it leads to performance problems. We are working on alternative solutions, such as adding a code action to add automatically add one of the missing files found by the F12 Tag Parser and supporting compile_commands.json to automatically provide the paths. Would those solutions be good enough for you or do you have other suggestions? It may also be possible to attempt to use recursive paths and then give errors if multiple ambiguous files are found, and give errors if the recursion generates too many paths such that performance issues occur.

keton commented 7 years ago

Ok,

Maybe I'll provide more info about my use case so you can better understand my problem.

I'm creating software development kit for an embedded platform based on ARM Cortex M4F MCU. Our build system is Makefile based and modular in sense that each module exists in it's own directory.

This is in line with design specifications of embedded RTOS kernel we rely on. RTOS kernel alone consists of ~400 separate modules where each module is in it's own directory. We already add ~100 modules on top of that and this number is constantly growing as our work progresses.

All those modules share common top level paths so before new Intellisense our includePaths/browseFolders looked like this:

"${workspaceRoot}",
"${workspaceRoot}/../RTOS",
"${workspaceRoot}/../modules",
"${gccInstallRoot}"

In order to satisfy new Intellisense this list has to be replaced by folder listing ~500 lines long.

Since we intend to ship complete IDE experience based on VSCode and we plan to support Windows, Mac and Linux this has to be repeated 6 times (includePaths+browseFolders* 3 platforms) giving config file 3k lines long.

What's more concerning this list has to be manually updated every time new module is added (few times a day during active development, every release in normal update cycle).

Since VSCode doesn't have a way to provide single configuration file to multiple projects (correct me if I'm wrong) configuration update must be performed on each project separately.

This may work if only we as a SDK developer have to do it but such change has to be retroactively applied by out customers on all their existing projects every time there's an update.

A compiler that accepted recursive include paths would not be able to handle files with the same name at different path locations due to the recursion, and it leads to performance problems.

You write that no support for recursion helps with performance but even with this setup I don't experience any problems on machine as old as 2011 i5 iMac with 4gb of RAM not to mention my regular workstation. And as far is I observed recursive folder scanning is done once when project is opened.

As for not being to handle files with the same name - ok but my whole setup doesn't have this. Honestly I haven't seen a production project with name clashes since during compilation all .o files typically end up in the same location. Even if it's not the case VScode supports excludePaths feature to deal with it.

Could you please at least consider giving developer an option to opt-in for recursive inclusion similar to folderName(/*) feature of tagParser. This will help a lot with projects like mine which by spec consist of a lot of folders and there's no single include folder.

sean-mcmanus commented 7 years ago

Our compiler just needs the same paths that each module needs to build itself, so just like a compiler it automatically adds the path that the C/C++ file is located in. So you shouldn't need to add 500 paths, unless somehow your Makefile has 500 paths added to it, enabling any C/C++ file in a module to reference any header in any other module, which seems unlikely. We might consider this if there exists a commonly used build system that supports an option to recursively add include paths recursively. Do you expect users to create files that don't match any names in the 500 folders of your SDK?

keton commented 7 years ago

Our compiler just needs the same paths that each module needs to build itself, so just like a compiler it automatically adds the path that the C/C++ file is located in. So you shouldn't need to add 500 paths

That would be true if all those modules existed inside ${workspaceRoot}. As I've written before most of the code lives in 2 folders outside of ${workspaceRoot}. ${workspaceRoot} contains only code specific to user's application. The whole folder structure is as follows:

SDK root folder
-- RTOS (embedded kernel code we rely on in 400 subfolders divided into few top level folders)
-- modules (our drivers and RTOS customization code. ~100 subfolders divided into few top level folders)
-- examples (top level folder of examples)
-- -- example1 (this contains application specific code and .vscode folder)
-- -- example2 (this contains application specific code and .vscode folder)
.... (and so on)

--projects (user application code organized exactly the same as examples)

From the compiler standpoint not everything is included in every build. Each project has its top level makefile specifying list of modules (folders) to include. This is automatically assembled into include and object paths.

Our tests with users show that they expect the IDE to 'just work' so we try to configure VSCode code assists so they will recognize all modules in single universal settings template. This way no matter what user selects he'll get working code completion. Users don't want to configure IDE for each project separately. That's why I'm trying to add whole codebase to include path in IDE settings even if most of it isn't used on given project.

Do you expect users to create files that don't match any names in the 500 folders of your SDK?

All SDK files are prefixed in such way that makes accidental name clash unlikely. 400 of those folders belong to third party RTOS kernel where unique file and function names are part of design spec. It's tested and certified in this form and cannot be modified by us. Our code needs to fit into this standard.

I know that it may seem like very weird project configuration but the same pattern is widespread in embedded BSPs from may vendors including STMicro (STM32), Sillicon Labs (Gecko series) and NordicSemi (nRF5x series). Typical solution is to use Eclipse based IDE which is recursive by default. We're betting on VSCode because in my opinion it provides better editing experience.

kamenlitchev commented 6 years ago

I'd vote for recursive inclusion, too. Just tried setting up some ARM GCC projects and it is a nightmare chasing down all directories where a specific .H file is located. Granted, the projects in question come from Mbed Compiler Online and MediaTek SDKs with a Makefile. What those makefiles do with few lines of code took me 2 hours to sort out.

Given that (ARM) GCC does recurse included directories, it is normal that any code completion does the same.

As for the way to take care of name collisions, you can always do one (or both) of:

sean-mcmanus commented 6 years ago

@kamenlitchev Recursive search for headers can be achieved via using the code action suggestions. As long as the path can be found recursively via the browse.path, you should be able to use the code action to find the file and add the required path. Let us know if this isn't sufficient. We have considered an option to add include files repeatedly whenever there's only 1 possible path found via the recursive search, but that is on our backlog.

ihnorton commented 6 years ago

500 paths added to it

Sadly, yes. (in my case, this is a CMake project with dozens of dependencies, all using side-by-side nested headers 😢 ):

cat /tmp/one-clang-invocation | sed -e $'s/ /\\\n/g' | grep "\-I" | wc -l
     495

Out of curiousity: am I correct in understanding that cpptools intellisense is closed-source? (I didn't find a repo while poking around with obvious keywords at gh.com/Microsoft)

sean-mcmanus commented 6 years ago

@ihnorton The typescript side is open source. The code in the 2 Microsoft.VSCode.Extension processes is closed source. It sounds like you should use cmake to export a compile_commands.json and then use the compileCommands property, which will automatically use correct includePath/defines settings.

keton commented 6 years ago

It sounds like you should use cmake to export a compile_commands.json

@sean-mcmanus Currently there's no working way to do this on Windows with armgcc toolchain most embedded projects use. And believe me I've spent a lot of time to make it work. On other platforms debugging with armgdbserver doesn't work as I and others reported in other tickets. Maybe as you see that my use case is not that unique you'll reconsider adding recursive include option?

bobbrow commented 6 years ago

As a short term fix, I started a branch to support a wildcard in the include path. We didn't want to enable full recursion out of the box because of some excessive memory usage in the IntelliSense engine for large folder hierarchies. However, Visual Studio also uses this engine and is previewing a fix that resolves those problems, so I am hopeful that we can leverage that code soon. It probably won't be in the February release though.

ihnorton commented 6 years ago

export a compile_commands.json and then use the compileCommands property, which will automatically use correct includePath/defines settings

I tried that and it doesn't seem to help (still missing-include errors). My compile_commands.json file is almost 15k lines long though, with many lines consisting of those ~500 include compiler invocations... So without some feedback from the engine on what's happening behind the scenes I assume a background worker is either still chugging or already gave up (didn't notice anything on the console in devtools). I've worked around this myself for now with some awk'ing-and-pasting into c_cpp_properties.json.

Visual Studio also uses this engine and is previewing a fix that resolves those problems, so I am hopeful that we can leverage that code soon.

That's fantastic -- would make a big difference for easy-start onboarding.

sean-mcmanus commented 6 years ago

@bobbrow Isn't compile_commands.json supposed fix all the missing includes? Do you have any idea what could be going wrong? Maybe we're not handling a required flag?

ihnorton commented 6 years ago

I just noticed the file is about 65 MB. I can understand if you consider that unreasonable! :rofl:

(first thing I tried was pasting into a gist, which hard-crashed all of Chrome on me, not just the tab)

sean-mcmanus commented 6 years ago

If we give back missing includes messages, then that means our parser process didn't crash, so the size of the file seems to be acceptable. It's possible the missing include is some bug caused by us bailing out, but I'm not aware of any code that would behave that way. I don't know what the largest compile_commands.json we've tested with is (@bobbrow might know).

sean-mcmanus commented 6 years ago

@ihnorton After consulting with Bob, it seems the 3 known issues that could cause the compile_commands.json to still giving missing includes are:

  1. If the "command" doesn't start with the compiler, in which case we fail to get the correct system headers: https://github.com/Microsoft/vscode-cpptools/issues/1251 .
  2. If your code requires system defines set (i.e. compiles different headers based on the compiler). I don't remember the issue number.
  3. If your OS is different from the one you're compiling against (i.e. MinGW on Windows) we may have issues, not sure.
ihnorton commented 6 years ago

3 known issues

1: I don't think so -- all of my entries look like "command": "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ ... 2: I don't think so -- there aren't any system defines in a spot check of some compiler invocations. 3: no

sean-mcmanus commented 6 years ago

Okay, it looks like that's the bug -- we don't handle the "c++" symbolic link, only clang/clang++. Replacing that path by the target of the c++ link might fix your issue.

sean-mcmanus commented 6 years ago

Oops, ignore what I said...the compiler guessing code is used when there's no compile_commands.json. The code should be querying the c++ command, since it's the first arg. The code should output an error message in our C/C++ Output panel if it's failing to locate the default include paths from the c++ compiler. So this could be some different usage -- it appears only a small fraction of users are using compile_commands.json, so you may be the first person to report this failure.

ihnorton commented 6 years ago

I tried a little experiment: deleted all but the first ~600 lines (out of >14000) in compile_commands.json (making sure to preserve the array structure). Still getting missing includes, but seemingly able to find more than before. But it's a little hard to tell -- it would be really helpful to have some feedback of progress/status under the hood, beyond just the "missing" messages, because right now it feels like poking a black box. After reopening vscode and a single cxx file, I observe issues like:

  1. takes 2-3 clicks on a header name -> "Go to definition" to actually open (the first 1-2 times it gives "no definition found", then works the third time. This even happens after only closing-reopening the cxx file, in the same vscode workspace session).
  2. I get warnings in the Problems pane about "cannot open source file" for transitive headers. Then I open the parent header (after the 1-3 clicks in some cases as above), click on the supposedly missing header -> "go to definition" , and it opens up right away.
sean-mcmanus commented 6 years ago
  1. Oh, that is very strange (I've never seen that behavior). The Go to Definition behavior is not impacted by the compileCommands/includePath settings and uses browse.path instead. The data should be stored in a database and quickly retried, so it could occur the 1st time a file/workspace is opened, but "No definition found" means it wasn't in the database or the database wasn't available. The IntelliSense thread (red flame) can block Go to Definition results, but it should give the "blue cylon" and then results.

  2. "cannot open source file" is from IntelliSense using includePath/compleCommands, but Go to Definition uses browse.path, which is why the results are missing with one, but found by the other.

ihnorton commented 6 years ago

Perhaps some of my (and general?) confusion is coming from the following sequence. I set up debugging and put a breakpoint in DefaultClient.updateStatus.

edit: hmm, maybe "IntelliSense Ready" is referring to the fallback thing (tag parser?)

(I'm assuming that intellisense is only trying to work on the one file I have open; if it is doing other things too then maybe the different messages make sense)

ihnorton commented 6 years ago

Whoops, crossed posts there...

"cannot open source file" is from IntelliSense using includePath/compleCommands, but Go to Definition uses browse.path, which is why the results are missing with one, but found by the other.

Ok, now various behavior makes more sense. Still getting missing includes in the Problems window, but the above indicates my "does this work" test was not useful.

sean-mcmanus commented 6 years ago

This is available in 0.17.0-insiders2 or later: https://github.com/Microsoft/vscode-cpptools/releases , e.g. "${workspaceFolder}/**", but we're still working on improving the performance when the number of paths is large. The final release is planned for early next week.

polarapfel commented 6 years ago

I just tried the insider preview (0.17.0-insiders2).

I'm working with an mbed OS project. I noticed while headers are now found correctly, some symbols are not resolving anymore.

Example:

include "mbed.h"

DigitalOut led1(LED1);

// main() runs in its own thread in the OS int main() { while (true) { led1 = !led1; wait(2); } }

(Excuse the plain format past here, but the code formatting doesn't work for me.)

Intellisense stumbles over LED1 now. Of course, this sample compiles and executes correctly.

sean-mcmanus commented 6 years ago

@polarapfel It probably means the wrong folder was chosen (i.e. a folder with a file with the same name that is supposed to define LED1). When you use ${workspaceFolder}/** it'll expand to some list of child folders, which can be seen via setting the loggingLevel to the hidden value of "6" (we're working on trimming the potential large list of recursive directories to the ones that are actually used, but that shouldn't change the final results). To get it to choose the correct folder, you can either manually put the correct folder before the recursive include path or use a files.exclude/search.exclude for the folder that has a name conflict.

Does that seem like what's occurring or does it seem like we have some other bug?

polarapfel commented 6 years ago

@sean-mcmanus You're right. I was expecting / to expand to everything, but that doesn't seem the case. I included the workspace folder as well as a child folder with / and now it works. Thanks!

Update. I actually got to take that back. It takes Intellisense a bit to process this, but once it does, it still fails to find that symbol correctly.

bobbrow commented 6 years ago

The initial implementation of this is available in 0.17.0. Additional performance optimizations are coming in 0.17.1.

AlanWhereAreYou commented 6 years ago

@bobbrow you said that /** will

expand to some list of child folders

Can you explain the logic it uses?

bobbrow commented 6 years ago

If you use /some/path/** as your include path we will attempt to compute the true include path for your source files when IntelliSense starts. Any folder under /some/path is a potential candidate. We scan the #includes of the source file and its dependencies and match them up with the candidate folders until either we find a path that exists or we run out of folders to try. These results are stored in our browse database for fast lookup later.

AlanWhereAreYou commented 6 years ago

Thanks @bobbrow - I'm still confused though. At the moment I have a header file here:

"C:\Users\myuser\AppData\Local\Arduino15\packages\arduino\hardware\avr\1.6.21\firmwares\wifishield\wifi_dnld\src\SOFTWARE_FRAMEWORK\BOARDS\ARDUINO\arduino.h"

I have this include: #include <arduino.h>

And I have this includePath:

C:\\Users\\myuser\\AppData\\Local\\Arduino15\\packages\\arduino\\hardware\\avr\\1.6.21\\firmwares\\wifishield\\wifi_dnld\\**

From that includePath onwards, there is only one instance of arduino.h.

Intellisense doesn't find it, and is underlining it. But, if I add one more level to my includePath:

C:\\Users\\myuser\\AppData\\Local\\Arduino15\\packages\\arduino\\hardware\\avr\\1.6.21\\firmwares\\wifishield\\wifi_dnld\\src\\**

...then suddenly it can find the header.

Am I missing something?

Thanks in advance.

bobbrow commented 6 years ago

That's unexpected. If you can tell us how you set up your development environment we can try to reproduce your case and debug it. As this issue is now closed, could you please open a new issue with the details?

AlanWhereAreYou commented 6 years ago

@bobbrow Actually, it's not reliably reproducible. I've been editing the includePath, saving, and then rebuilding the Intellisense DB. With the same paths, sometimes it seems it will find the header and sometimes it will not. If I close VSCode and reopen completely, it seems to revert to not-finding the header (so I suspect that's the stable state).

What is the expected behaviour here? That with either of those two paths the header should be found via recursion? If so I can easily file a new issue. If not, then I think probably it's working correctly but there is some weirdness occurring while paths are being edited.

bobbrow commented 6 years ago

If the header is in the recursive folder you specified, we should find it. I want to debug this on my PC so that I can see what is going on.

If you can open a new issue and tell us where you got your arduino headers from (I had Arduino on my PC before but I don't think the headers ever lived in the AppData folder), we can investigate. It sounds like I should be able to reproduce this by just adding #include <arduino.h> to an empty source file and messing with the include path?

AlanWhereAreYou commented 6 years ago

@bobbrow Ok, will do. It's a good question where the headers came from - I actually assumed, probably incorrectly, that the arduino plugin added them. I'll see what I can find out while composing the issue description (might be a few days before I can do that though).

AlanWhereAreYou commented 6 years ago

@bobbrow I can't seem to reproduce it in what I think is a valid way. When I try repeating my config above, it seems to work reliably after a intellisense DB rebuild or a start/restart. I'll keep an eye out and, now that I know the expected behaviour, if I see it occur again in a repeatable way I'll open a new ticket. Sorry to have distracted you with this.