From a29f4fc978d12a6b98e525a221a33d10ad999595 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 25 Sep 2023 00:31:41 -0700 Subject: [PATCH] Error on potential unsupported `/showIncludes` lines 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 https://github.com/bazelbuild/bazel/issues/19439#issuecomment-1728701937 Closes #19580. PiperOrigin-RevId: 568133958 Change-Id: If43924da6ba2f332edf4db0aed24056aa89fbb75 --- .../build/lib/rules/cpp/CppCompileAction.java | 16 ++++++++ .../lib/rules/cpp/ShowIncludesFilter.java | 27 +++++++++++++ .../lib/rules/cpp/ShowIncludesFilterTest.java | 38 +++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index df4d467a667d50..7432e6d7ac6c9d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -1614,6 +1614,22 @@ private NestedSet discoverInputsFromShowIncludesFilters( throws ActionExecutionException { Collection stdoutDeps = showIncludesFilterForStdout.getDependencies(execRoot); Collection 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(), diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java index 5f457fa4358152..e6804184fb707f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java +++ b/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\\\\(?.*)"); + // 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 getDependencies() { return this.dependencies; } + + public boolean sawPotentialUnsupportedShowIncludesLine() { + return sawPotentialUnsupportedShowIncludesLine; + } } public FilterOutputStream getFilteredOutputStream(OutputStream outputStream) { @@ -232,6 +254,11 @@ public Collection getDependencies(Path root) { return Collections.unmodifiableCollection(dependenciesInPath); } + public boolean sawPotentialUnsupportedShowIncludesLine() { + return filterShowIncludesOutputStream != null + && filterShowIncludesOutputStream.sawPotentialUnsupportedShowIncludesLine(); + } + @VisibleForTesting Collection getDependencies() { return filterShowIncludesOutputStream.getDependencies(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java index 6bfaf66058ac1b..12c3cf88dfd440 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java +++ b/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(); } }