Parsing of /showIncludes MSVC compiler output is incorrect when INCLUDES contains relative paths.

[konste]

Description of the problem / feature request:

On Windows Bazel uses /showIncludes compiler option to get the list of headers used during compilation and generate possible "undeclared header usage" errors. Unfortunately it seems the code parsing compiler output expects the paths to always be absolute. In reality MSVC follows what it is told in INCLUDES - if the path there is absolute, then the path in compiler output would be absolute, but if the path in INCLUDES is relative, then in compiler output it is also relative, which is logical.
Using absolute paths in INCLUDES effectively blocks hermeticity.
The problem is not apparent because by default Bazel discovers local MSVC installation and uses that. Of course, all standard MSVC installations only have absolute paths used in INCLUDES. Which is Ok as nobody expects discovered toolchain to be hermetic. But if we want to use our own fully hermetic MSVC toolchain we must use relative paths everywhere and in INCLUDES which causes Bazel to wrongfully complain about undeclared inclusions. Bummer.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

It would take me substantial effort to produce a standalone repro, so I would like to know if it is needed before I spend that time. In short it is Windows build with some system include folders specified in INCLUDES env.var. as relative (to execroot) paths.

What operating system are you running Bazel on?

This problem is specific for Windows and MSVC.

What's the output of bazel info release?

4.2.1

Have you found anything relevant by searching the web?

I found the code which does the parsing here. @meteorcloudy your name is all over it, so you must have a good idea about it.

I even have trouble understanding the following comment from that code:

// Prefix the matched header path with "..\". This way, external repo header paths are
// resolved to "<execroot>\..\<repo name>\<path>", and main repo file paths are
// resolved to "<execroot>\..\<main repo>\<path>", which is nicely normalized to
// "<execroot>\<path>".

If two dots are inserted AFTER repo name I would understand it, but inserting it BEFORE repo name does not make sense to me. Could you please clarify?

[meteorcloudy]

// Prefix the matched header path with "..\". This way, external repo header paths are
// resolved to "<execroot>\..\<repo name>\<path>", and main repo file paths are
// resolved to "<execroot>\..\<main repo>\<path>", which is nicely normalized to
// "<execroot>\<path>".

@konste This change was introduced at in 24c980b for
7149f57

I actually never noticed that INCULDES could make cl.exe output relative paths for header discovery. If that's true, we can indeed take advantage of that to make Windows toolchain more hermetic. Can you somehow construct a minimal reproduce case?

/cc @andrewkatson This is an interesting Windows problem to look into.

[konste]

@meteorcloudy Thank you for the prompt response! Now that I know it is not in vain I will invest time into making the minimal standalone repro and try to debug the relevant parts of Bazel code.

[meteorcloudy]

@konste Nice, thanks for looking into it!

[konste]

After many experiments I discovered that MSVC option /showIncludes indeed outputs absolute paths most of the time, BUT there is at least one exception: when INCLUDE env var contains relative paths AND debug information is not requested – it outputs relative paths! Bazel MSVC “discovery” toolchain does not encounter it because 1. Most of the time INCLUDE env var contains absolute paths and also 2. /Z7 is used always , even for fastbuild. Striving to make our MSVC toolchain hermetic we switched to relative paths in INCLUDE and our fastbuild configuration deliberately skips debug info generation (for speed), so we hit that case.

I have shared pretty compact and self-contained repro here. It uses “nocopts” to remove /Z7 set by the toolchain and also adds the folder “prop_msvc_toolchain” (which is obviously relative) to “includes” attribute.
Compiler processes relative include path just fine and the header “prop_system_header.h” is successfully found in subfolder “prop_msvc_toolchain”, but for some reason Bazel does not flag it as undeclared header even though it is not included in srcs (deliberately). My gut feeling is that it may be connected to the fact that the header is coming from the external (local) workspace, but I still don't see why not report it as undeclared. It is not listed in cxx_builtin_include_directories of the discovered toolchain. @meteorcloudy any clue why this may be happening?

[meteorcloudy]

I think it's related to this flag --strict_system_includes. By default it's turned off to allow system include paths specified with "-isystem" (instead of "-i"). But on Windows, there is no such difference, so any directory specified with "/I" is allowed. If you don't use includes = ["prop_msvc_toolchain"] or you turn on this flag, you'll get a proper header check error. (You may need to do a bazel clean --expunge first)

Also on Linux and macOS, we have sandbox enabled, so this missing dependency is even caught before the header check, because the compiling action cannot even see the prop_system_header.h file.

/cc @oquenchil This looks like a bug on Windows, include path specified by the includes attribute shouldn't be collected as system include path.

[meteorcloudy]

Oh, this is not a bug. includes is supposed to be used for system include paths.
https://docs.bazel.build/versions/main/be/c-cpp.html#cc_binary.includes

[meteorcloudy]

@konste Maybe you intended to use https://docs.bazel.build/versions/main/be/c-cpp.html#cc_library.include_prefix ? This is only available in cc_library.

[konste]

@meteorcloudy thank you, things become more clear now! Let me ask a couple of questions and then I will summarize the knowledge for anybody finding this issue in the future.

Also on Linux and macOS, we have sandbox enabled, so this missing dependency is even caught before the header check

Is there any value then in running header check when sandbox is available and enabled? If no then how can it be disabled or it is disabled automatically?

Regarding header folders. Those are collected from multiple sources. One is cxx_builtin_include_directories parameter to cc_common.create_cc_toolchain_config_info. Those supposed to be system header folders and when the headers comes from there it is not flagged as undeclared, right? Does --strict_system_includes change that? I hope not, it would be bad.

Next set of header folders comes from the includes attribute. It is a big surprise for me (and it looks like for you too) that those are also interpreted as system header folders, because they specify the location within the package, so definitely not system.

I have created a tiny repro here and my experiments with it on Windows show that by default only the header from the root folder is flagged as undeclared, but when I specify --strict_system_includes both headers are flagged and at the same time real system headers are not flagged.

Having both headers flagged matches my expectation. I am surprised it is not the default, but having the flag --strict_system_includes to enable it is good enough.

Is my understanding correct that --strict_system_includes affects the headers from includes and does not affect the headers from cxx_builtin_include_directories?

I will keep experimenting on all platforms hoping to be able to enable --strict_system_includes across the board.

[meteorcloudy]

Is there any value then in running header check when sandbox is available and enabled? If no then how can it be disabled or it is disabled automatically?

I believe it is still enabled. However, I cannot think of a case where sandbox won't catch the error but header checking will.

The code is here:
https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java;l=632

Looks like dirs specified in cxx_builtin_include_directories are always ignored no matter if the flag is on or off.

Is my understanding correct that --strict_system_includes affects the headers from includes and does not affect the headers from cxx_builtin_include_directories?

Yes, I think that's the case.

[konste]

The summary:

1. MSVC /showIncludes option indeed switches to reporting relative header file paths in some cases, but it does not seem to be a problem as Bazel still correctly parses that.
2. To enable inclusion checking for the headers coming from the folders specified with includes attribute --strict_system_headers is required. It is False by default.
3. When sandbox is present header validation does not make sense, but still happens regardless. I was trying to get rid of it by negation of parse_showincludes feature, but it does not allow me to turn this feature off.

May be #3 is worth tracking as a separate bug?

[meteorcloudy]

When sandbox is present header validation does not make sense, but still happens regardless. I was trying to get rid of it by negation of parse_showincludes feature, but it does not allow me to turn this feature off.

parse_showincludes feature is only effective on Windows, because it relies on the /showIncudles option in MSVC. On other platforms, we use the dot d file to do header check, which is enabled by dependency_file. And on Windows, we don't have sandbox support. So this is not a bug.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

[matt-sm]

I am seeing something similar. I have removed all absolute paths from an msvc toolchain.

I could not get INCLUDES to work with a relative path, but have set:

/Iexternal/mytoolchain_files//VC/Tools/MSVC/14.16.27023/include

when I compile with /showincludes from command line, the compilation succeeds and I can see the relative paths eg.

Note: including file:    external/mytoolchain_files/VC/Tools/MSVC/14.16.27023/include\stdarg.h

But when I try with Bazel, the parse_showincludes feature fails to parse them correctly. For those files I get the "missing declaration" error. For now I've had to switch of parse_showincludes but would like to know what's failing in Bazel.

[matt-sm]

I went back and retested relative paths in INCLUDES without /showIncludes and that does indeed work. No need for the extra /I. I have tried reading some of the parse_showincludes source but it's difficult to determine exactly where the problem is.