From da2d8d97ed8ffdbc185052fc9249b4eb47a63d15 Mon Sep 17 00:00:00 2001 From: plf Date: Tue, 30 Apr 2019 08:00:34 -0700 Subject: [PATCH] C++: Fix crash with bad filetypes in compilation outputs #4570 RELNOTES:none PiperOrigin-RevId: 245947851 --- .../lib/bazel/rules/cpp/BazelCcModule.java | 9 +- .../build/lib/rules/cpp/CcModule.java | 42 +++- .../build/lib/rules/cpp/CppFileTypes.java | 14 +- .../skylarkbuildapi/cpp/BazelCcModuleApi.java | 3 +- .../skydoc/fakebuildapi/cpp/FakeCcModule.java | 2 +- .../lib/rules/cpp/SkylarkCcCommonTest.java | 189 ++++++++++++------ 6 files changed, 172 insertions(+), 87 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java index 2525e84670dc4e..c8c7c69ff99dfd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcModule.java @@ -142,13 +142,8 @@ public CcLinkingOutputs link( @Override public CcCompilationOutputs createCompilationOutputsFromSkylark( - Object objectsObject, Object picObjectsObject) { - CcCompilationOutputs.Builder ccCompilationOutputsBuilder = CcCompilationOutputs.builder(); - ccCompilationOutputsBuilder.addObjectFiles( - convertSkylarkListOrNestedSetToNestedSet(objectsObject, Artifact.class)); - ccCompilationOutputsBuilder.addPicObjectFiles( - convertSkylarkListOrNestedSetToNestedSet(picObjectsObject, Artifact.class)); - return ccCompilationOutputsBuilder.build(); + Object objectsObject, Object picObjectsObject, Location location) throws EvalException { + return super.createCompilationOutputsFromSkylark(objectsObject, picObjectsObject, location); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index 9b8ca47c163e41..94acf1438c5a06 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java @@ -67,7 +67,7 @@ import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.syntax.SkylarkNestedSet; import com.google.devtools.build.lib.syntax.SkylarkType; -import com.google.devtools.build.lib.util.FileType; +import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.StringUtil; import com.google.devtools.build.lib.vfs.PathFragment; @@ -1491,15 +1491,19 @@ protected Tuple compile( "srcs", sources, CppFileTypes.ALL_C_CLASS_SOURCE, - FileType.of( - ImmutableList.builder() - .addAll(CppFileTypes.CPP_SOURCE.getExtensions()) - .addAll(CppFileTypes.C_SOURCE.getExtensions()) - .build())); + FileTypeSet.of(CppFileTypes.CPP_SOURCE, CppFileTypes.C_SOURCE)); validateExtensions( - location, "public_hdrs", publicHeaders, CppFileTypes.CPP_HEADER, CppFileTypes.CPP_HEADER); + location, + "public_hdrs", + publicHeaders, + FileTypeSet.of(CppFileTypes.CPP_HEADER), + FileTypeSet.of(CppFileTypes.CPP_HEADER)); validateExtensions( - location, "private_hdrs", privateHeaders, CppFileTypes.CPP_HEADER, CppFileTypes.CPP_HEADER); + location, + "private_hdrs", + privateHeaders, + FileTypeSet.of(CppFileTypes.CPP_HEADER), + FileTypeSet.of(CppFileTypes.CPP_HEADER)); CcCompilationHelper helper = new CcCompilationHelper( @@ -1625,15 +1629,31 @@ protected CcLinkingOutputs link( } } + protected CcCompilationOutputs createCompilationOutputsFromSkylark( + Object objectsObject, Object picObjectsObject, Location location) throws EvalException { + CcCompilationOutputs.Builder ccCompilationOutputsBuilder = CcCompilationOutputs.builder(); + NestedSet objects = + convertSkylarkListOrNestedSetToNestedSet(objectsObject, Artifact.class); + validateExtensions( + location, "objects", objects.toList(), Link.OBJECT_FILETYPES, Link.OBJECT_FILETYPES); + NestedSet picObjects = + convertSkylarkListOrNestedSetToNestedSet(picObjectsObject, Artifact.class); + validateExtensions( + location, "pic_objects", picObjects.toList(), Link.OBJECT_FILETYPES, Link.OBJECT_FILETYPES); + ccCompilationOutputsBuilder.addObjectFiles(objects); + ccCompilationOutputsBuilder.addPicObjectFiles(picObjects); + return ccCompilationOutputsBuilder.build(); + } + private void validateExtensions( Location location, String paramName, List files, - FileType validFileType, - FileType fileTypeForErrorMessage) + FileTypeSet validFileTypeSet, + FileTypeSet fileTypeForErrorMessage) throws EvalException { for (Artifact file : files) { - if (!validFileType.matches(file)) { + if (!validFileTypeSet.matches(file.getFilename())) { throw new EvalException( location, String.format( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java index d7d01310ce2b5a..63a412b8ba03df 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java @@ -29,14 +29,12 @@ public final class CppFileTypes { public static final FileType C_SOURCE = FileType.of(".c"); public static final FileType OBJC_SOURCE = FileType.of(".m"); public static final FileType OBJCPP_SOURCE = FileType.of(".mm"); - public static final FileType ALL_C_CLASS_SOURCE = - FileType.of( - FileTypeSet.of( - CppFileTypes.CPP_SOURCE, - CppFileTypes.C_SOURCE, - CppFileTypes.OBJCPP_SOURCE, - CppFileTypes.OBJC_SOURCE) - .getExtensions()); + public static final FileTypeSet ALL_C_CLASS_SOURCE = + FileTypeSet.of( + CppFileTypes.CPP_SOURCE, + CppFileTypes.C_SOURCE, + CppFileTypes.OBJCPP_SOURCE, + CppFileTypes.OBJC_SOURCE); // Filetypes that generate LLVM bitcode when -flto is specified. public static final FileTypeSet LTO_SOURCE = diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java index 3b524d12607146..df48778e754d66 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BazelCcModuleApi.java @@ -319,6 +319,7 @@ LinkingOutputsT link( @SkylarkCallable( name = "create_compilation_outputs", doc = "Create compilation outputs object.", + useLocation = true, parameters = { @Param( name = "objects", @@ -344,7 +345,7 @@ LinkingOutputsT link( }), }) CompilationOutputsT createCompilationOutputsFromSkylark( - Object objectsObject, Object picObjectsObject); + Object objectsObject, Object picObjectsObject, Location location) throws EvalException; @SkylarkCallable( name = "merge_compilation_outputs", diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/cpp/FakeCcModule.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/cpp/FakeCcModule.java index 276f497141b6c1..ec42d59dcd6162 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/cpp/FakeCcModule.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/cpp/FakeCcModule.java @@ -259,7 +259,7 @@ public CcToolchainConfigInfoApi ccToolchainConfigInfoFromSkylark( @Override public CcCompilationOutputsApi createCompilationOutputsFromSkylark( - Object objectsObject, Object picObjectsObject) { + Object objectsObject, Object picObjectsObject, Location location) { return null; } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java index 4f96a129044a29..6727333dd8fdbc 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java @@ -4642,27 +4642,7 @@ public void testWrongExtensionThrowsError() throws Exception { @Test public void testCcOutputsMerging() throws Exception { - scratch.overwriteFile("tools/build_defs/foo/BUILD"); - scratch.file( - "tools/build_defs/foo/extension.bzl", - "load('//myinfo:myinfo.bzl', 'MyInfo')", - "def _cc_skylark_library_impl(ctx):", - " c1 = cc_common.create_compilation_outputs(objects=depset([ctx.file.object1]),", - " pic_objects=depset([ctx.file.pic_object1]))", - " c2 = cc_common.create_compilation_outputs(objects=depset([ctx.file.object2]),", - " pic_objects=depset([ctx.file.pic_object2]))", - " compilation_outputs = cc_common.merge_compilation_outputs(", - " compilation_outputs=[c1, c2])", - " return [MyInfo(compilation_outputs=compilation_outputs)]", - "cc_skylark_library = rule(", - " implementation = _cc_skylark_library_impl,", - " attrs = {", - " 'object1': attr.label(allow_single_file=True),", - " 'pic_object1': attr.label(allow_single_file=True),", - " 'object2': attr.label(allow_single_file=True),", - " 'pic_object2': attr.label(allow_single_file=True),", - " },", - ")"); + setupCcOutputsTest(); scratch.file( "foo/BUILD", "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", @@ -4687,6 +4667,26 @@ public void testCcOutputsMerging() throws Exception { .containsExactly("src foo/object1.o", "src foo/object2.o"); } + @Test + public void testObjectsWrongExtension() throws Exception { + doTestCcOutputsWrongExtension("object1", "objects"); + } + + @Test + public void testPicObjectsWrongExtension() throws Exception { + doTestCcOutputsWrongExtension("pic_object1", "pic_objects"); + } + + @Test + public void testObjectsRightExtension() throws Exception { + doTestCcOutputsRightExtension("object1"); + } + + @Test + public void testPicObjectsRightExtension() throws Exception { + doTestCcOutputsRightExtension("pic_object1"); + } + @Test public void testCreateOnlyPic() throws Exception { getAnalysisMock() @@ -4939,66 +4939,34 @@ public void testAdditionalInputs() throws Exception { @Test public void testPossibleSrcsExtensions() throws Exception { - doTestPossibleExtensions("srcs", CppFileTypes.ALL_C_CLASS_SOURCE.getExtensions()); + doTestPossibleExtensionsOfSrcsAndHdrs("srcs", CppFileTypes.ALL_C_CLASS_SOURCE.getExtensions()); } @Test public void testPossiblePrivateHdrExtensions() throws Exception { - doTestPossibleExtensions("private_hdrs", CppFileTypes.CPP_HEADER.getExtensions()); + doTestPossibleExtensionsOfSrcsAndHdrs("private_hdrs", CppFileTypes.CPP_HEADER.getExtensions()); } @Test public void testPossiblePublicHdrExtensions() throws Exception { - doTestPossibleExtensions("public_hdrs", CppFileTypes.CPP_HEADER.getExtensions()); - } - - private void doTestPossibleExtensions(String attrName, List extensions) throws Exception { - createFiles(scratch, "tools/build_defs/foo"); - reporter.removeHandler(failFastHandler); - - for (String extension : extensions) { - scratch.deleteFile("bar/BUILD"); - scratch.file( - "bar/BUILD", - "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", - "cc_skylark_library(", - " name = 'skylark_lib',", - " " + attrName + " = ['file" + extension + "'],", - ")"); - getConfiguredTarget("//bar:skylark_lib"); - assertNoEvents(); - } + doTestPossibleExtensionsOfSrcsAndHdrs("public_hdrs", CppFileTypes.CPP_HEADER.getExtensions()); } @Test public void testWrongSrcsExtensionGivesError() throws Exception { - doTestWrongExtension("srcs"); + doTestWrongExtensionOfSrcsAndHdrs("srcs"); } @Test public void testWrongPrivateHdrExtensionGivesError() throws Exception { - doTestWrongExtension("private_hdrs"); + doTestWrongExtensionOfSrcsAndHdrs("private_hdrs"); } @Test public void testWrongPublicHdrExtensionGivesError() throws Exception { - doTestWrongExtension("public_hdrs"); + doTestWrongExtensionOfSrcsAndHdrs("public_hdrs"); } - private void doTestWrongExtension(String attrName) throws Exception { - createFiles(scratch, "tools/build_defs/foo"); - scratch.file( - "bar/BUILD", - "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", - "cc_skylark_library(", - " name = 'skylark_lib',", - " " + attrName + " = ['skylark_lib.cannotpossiblybevalid'],", - ")"); - reporter.removeHandler(failFastHandler); - getConfiguredTarget("//bar:skylark_lib"); - assertContainsEvent( - "has wrong extension. The list of possible extensions for '" + attrName + "'"); - } @Test public void testWrongSrcExtensionGivesError() throws Exception { @@ -5140,4 +5108,107 @@ private static void createFiles( " deps = ['skylark_lib'],", ")"); } + + private void doTestWrongExtensionOfSrcsAndHdrs(String attrName) throws Exception { + createFiles(scratch, "tools/build_defs/foo"); + scratch.file( + "bar/BUILD", + "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", + "cc_skylark_library(", + " name = 'skylark_lib',", + " " + attrName + " = ['skylark_lib.cannotpossiblybevalid'],", + ")"); + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//bar:skylark_lib"); + assertContainsEvent( + "has wrong extension. The list of possible extensions for '" + attrName + "'"); + } + + private void doTestPossibleExtensionsOfSrcsAndHdrs(String attrName, List extensions) + throws Exception { + createFiles(scratch, "tools/build_defs/foo"); + reporter.removeHandler(failFastHandler); + + for (String extension : extensions) { + scratch.deleteFile("bar/BUILD"); + scratch.file( + "bar/BUILD", + "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", + "cc_skylark_library(", + " name = 'skylark_lib',", + " " + attrName + " = ['file" + extension + "'],", + ")"); + getConfiguredTarget("//bar:skylark_lib"); + assertNoEvents(); + } + } + + private void doTestCcOutputsWrongExtension(String attrName, String paramName) throws Exception { + setupCcOutputsTest(); + scratch.file( + "foo/BUILD", + "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", + "cc_skylark_library(", + " name = 'skylark_lib',", + " " + attrName + " = 'object.cannotpossiblybevalid',", + ")"); + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//foo:skylark_lib"); + assertContainsEvent( + "has wrong extension. The list of possible extensions for '" + paramName + "'"); + } + + private void doTestCcOutputsRightExtension(String paramName) throws Exception { + setupCcOutputsTest(); + reporter.removeHandler(failFastHandler); + + for (String extension : Link.OBJECT_FILETYPES.getExtensions()) { + scratch.deleteFile("foo/BUILD"); + scratch.file( + "foo/BUILD", + "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", + "cc_skylark_library(", + " name = 'skylark_lib',", + " " + paramName + " = 'object1" + extension + "',", + ")"); + getConfiguredTarget("//foo:skylark_lib"); + assertNoEvents(); + } + } + + private void setupCcOutputsTest() throws Exception { + scratch.overwriteFile("tools/build_defs/foo/BUILD"); + scratch.file( + "tools/build_defs/foo/extension.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "def _cc_skylark_library_impl(ctx):", + " objects = []", + " pic_objects = []", + " if ctx.file.object1 != None:", + " objects.append(ctx.file.object1)", + " if ctx.file.pic_object1 != None:", + " pic_objects.append(ctx.file.pic_object1)", + " c1 = cc_common.create_compilation_outputs(objects=depset(objects),", + " pic_objects=depset(pic_objects))", + " objects = []", + " pic_objects = []", + " if ctx.file.object2 != None:", + " objects.append(ctx.file.object2)", + " if ctx.file.pic_object2 != None:", + " pic_objects.append(ctx.file.pic_object2)", + " c2 = cc_common.create_compilation_outputs(objects=depset(objects),", + " pic_objects=depset(pic_objects))", + " compilation_outputs = cc_common.merge_compilation_outputs(", + " compilation_outputs=[c1, c2])", + " return [MyInfo(compilation_outputs=compilation_outputs)]", + "cc_skylark_library = rule(", + " implementation = _cc_skylark_library_impl,", + " attrs = {", + " 'object1': attr.label(allow_single_file=True),", + " 'pic_object1': attr.label(allow_single_file=True),", + " 'object2': attr.label(allow_single_file=True),", + " 'pic_object2': attr.label(allow_single_file=True),", + " },", + ")"); + } }