[6.4.0] Error on potential unsupported /showIncludes lines (#19611)

This error ensures that removing the English language pack for MSVC
without also refetching `@local_config_cc` doesn't result in silently
incorrect incremental builds.

I considered making this a warning instead, but that would either result
in warning spam (if a false positive) or drown in the unfiltered MSVC
output (if a true positive). Making this an error is better for
correctness and will lead users to report any false positive matches.

Fixes
#19439 (comment)

Closes #19580.

Commit
a0497a0

PiperOrigin-RevId: 568133958
Change-Id: If43924da6ba2f332edf4db0aed24056aa89fbb75

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -1614,6 +1614,22 @@ private NestedSet<Artifact> discoverInputsFromShowIncludesFilters(
       throws ActionExecutionException {
     Collection<Path> stdoutDeps = showIncludesFilterForStdout.getDependencies(execRoot);
     Collection<Path> stderrDeps = showIncludesFilterForStderr.getDependencies(execRoot);
+    if (stdoutDeps.isEmpty()
+        && stderrDeps.isEmpty()
+        && (showIncludesFilterForStdout.sawPotentialUnsupportedShowIncludesLine()
+            || showIncludesFilterForStderr.sawPotentialUnsupportedShowIncludesLine())) {
+      // /showIncludes parsing didn't result in any headers being found (unusual) *and* also
+      // encountered a line that looked like /showIncludes output in an unsupported encoding.
+      String message =
+          "While parsing the C++ compiler output for information about included headers, Bazel "
+              + "failed to find any headers but encountered a line that appears to be "
+              + "/showIncludes output in an unsupported encoding. This can result in incorrect "
+              + "incremental builds. If you are using the default Windows MSVC toolchain that "
+              + "ships with Bazel, ensure that the English language pack for Visual Studio is "
+              + "installed and then run 'bazel clean --expunge'.";
+      DetailedExitCode code = createDetailedExitCode(message, Code.FIND_USED_HEADERS_IO_EXCEPTION);
+      throw new ActionExecutionException(message, this, /* catastrophe= */ false, code);
+    }
     return HeaderDiscovery.discoverInputsFromDependencies(
         this,
         getSourceFile(),

src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java

@@ -148,10 +148,19 @@ public static class FilterShowIncludesOutputStream extends FilterOutputStream {
                 StandardCharsets.UTF_8) // Spanish
             );
     private final String sourceFileName;
+    private boolean sawPotentialUnsupportedShowIncludesLine;
     // Grab everything under the execroot base so that external repository header files are covered
     // in the sibling repository layout.
     private static final Pattern EXECROOT_BASE_HEADER_PATTERN =
         Pattern.compile(".*execroot\\\\(?<headerPath>.*)");
+    // Match a line of the form "fooo: bar:   C:\some\path\file.h". As this is relatively generic,
+    // we require the line to include an absolute path with drive letter. If remote workers rewrite
+    // the path to a relative one, we won't match it, but it is unlikely that such setups use an
+    // unsupported encoding. We also exclude any matches that contain numbers: MSVC warnings and
+    // errors always contain numbers, but the /showIncludes output doesn't in any encoding since all
+    // codepages are ASCII-compatible.
+    private static final Pattern POTENTIAL_UNSUPPORTED_SHOW_INCLUDES_LINE =
+        Pattern.compile("[^:0-9]+:\\s+[^:0-9]+:\\s+[A-Za-z]:\\\\[^:]*\\\\execroot\\\\[^:]*");
 
     public FilterShowIncludesOutputStream(OutputStream out, String sourceFileName) {
       super(out);
@@ -182,6 +191,15 @@ public void write(int b) throws IOException {
         }
         // cl.exe also prints out the file name unconditionally, we need to also filter it out.
         if (!prefixMatched && !line.trim().equals(sourceFileName)) {
+          // When the toolchain definition failed to force an English locale, /showIncludes lines
+          // can use non-UTF8 encodings, which the checks above fail to detect. As this results in
+          // incorrect incremental builds, we emit a warning if the raw byte sequence comprising the
+          // line looks like it could be a /showIncludes line.
+          if (POTENTIAL_UNSUPPORTED_SHOW_INCLUDES_LINE
+              .matcher(buffer.toString(StandardCharsets.ISO_8859_1).trim())
+              .matches()) {
+            sawPotentialUnsupportedShowIncludesLine = true;
+          }
           buffer.writeTo(out);
         }
         buffer.reset();
@@ -214,6 +232,10 @@ public void flush() throws IOException {
     public Collection<String> getDependencies() {
       return this.dependencies;
     }
+
+    public boolean sawPotentialUnsupportedShowIncludesLine() {
+      return sawPotentialUnsupportedShowIncludesLine;
+    }
   }
 
   public FilterOutputStream getFilteredOutputStream(OutputStream outputStream) {
@@ -232,6 +254,11 @@ public Collection<Path> getDependencies(Path root) {
     return Collections.unmodifiableCollection(dependenciesInPath);
   }
 
+  public boolean sawPotentialUnsupportedShowIncludesLine() {
+    return filterShowIncludesOutputStream != null
+        && filterShowIncludesOutputStream.sawPotentialUnsupportedShowIncludesLine();
+  }
+
   @VisibleForTesting
   Collection<String> getDependencies() {
     return filterShowIncludesOutputStream.getDependencies();

src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java

@@ -55,6 +55,7 @@ public void testNotMatch() throws IOException {
     // Normal output message with newline
     filterOutputStream.write(getBytes("I am compiling\n"));
     assertThat(output.toString()).isEqualTo("I am compiling\n");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -65,6 +66,7 @@ public void testNotMatchThenFlushing() throws IOException {
     filterOutputStream.flush();
     // flush to output should succeed
     assertThat(output.toString()).isEqualTo("Still compiling");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -78,6 +80,7 @@ public void testMatchPartOfNotePrefix() throws IOException {
     filterOutputStream.write(getBytes("other info"));
     filterOutputStream.flush();
     assertThat(output.toString()).isEqualTo("Note: other info");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -91,6 +94,7 @@ public void testMatchAllOfNotePrefix() throws IOException {
     // It's a match, output should be filtered, dependency on bar.h should be found.
     assertThat(output.toString()).isEmpty();
     assertThat(showIncludesFilter.getDependencies()).contains("bar.h");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -106,6 +110,7 @@ public void testFindHeaderFromAbsolutePathUnderExecrootBase() throws IOException
     // It's a match, output should be filtered, dependency on bar.h should be found.
     assertThat(output.toString()).isEmpty();
     assertThat(showIncludesFilter.getDependencies()).contains("..\\__main__\\foo\\bar\\bar.h");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -119,6 +124,7 @@ public void testFindHeaderFromAbsolutePathOutsideExecroot() throws IOException {
     // It's a match, output should be filtered, dependency on bar.h should be found.
     assertThat(output.toString()).isEmpty();
     assertThat(showIncludesFilter.getDependencies()).contains("C:\\system\\foo\\bar\\bar.h");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -127,6 +133,7 @@ public void testMatchSourceFileName() throws IOException {
     // It's a match, output should be filtered, no dependency found.
     assertThat(output.toString()).isEmpty();
     assertThat(showIncludesFilter.getDependencies()).isEmpty();
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 
   @Test
@@ -138,5 +145,36 @@ public void testMatchPartOfSourceFileName() throws IOException {
     filterOutputStream.write(getBytes(".h"));
     filterOutputStream.flush();
     assertThat(output.toString()).isEqualTo("foo.h");
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
+  }
+
+  @Test
+  public void testSawPotentialUnsupportedShowIncludesLine() throws IOException {
+    // MSVC output with French non-UTF-8 locale.
+    filterOutputStream.write(getBytes("Remarque"));
+    filterOutputStream.write(0xFF);
+    filterOutputStream.write(getBytes(": inclusion du fichier"));
+    filterOutputStream.write(0xFF);
+    filterOutputStream.write(getBytes(":  C:\\bazel\\execroot\\foo\n"));
+    filterOutputStream.flush();
+
+    assertThat(output.toString(UTF_8)).isNotEmpty();
+    assertThat(showIncludesFilter.getDependencies()).isEmpty();
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isTrue();
+  }
+
+  @Test
+  public void testSawPotentialUnsupportedShowIncludesLine_nearMatches() throws IOException {
+    filterOutputStream.write(getBytes("foo: bar: C:\\bazel\\foo\n"));
+    filterOutputStream.write(getBytes("foo: C:\\bazel\\execroot\\foo\n"));
+    filterOutputStream.write(getBytes("foo: bar: baz: C:\\bazel\\execroot\\foo\n"));
+    filterOutputStream.write(getBytes("foo: bar(123): C:\\bazel\\execroot\\foo\n"));
+    filterOutputStream.write(getBytes("foo: bar: C:\\bazel\\execroot\\foo: baz\n"));
+    filterOutputStream.write(getBytes("foo: bar: bazel\\execroot\\foo\n"));
+    filterOutputStream.flush();
+
+    assertThat(output.toString(UTF_8)).isNotEmpty();
+    assertThat(showIncludesFilter.getDependencies()).isEmpty();
+    assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
   }
 }