Don't propagate rlibs through shared libraries for linking purposes. If a C++ target depends on an rlib R, the rlibs are treated as libraries for linking purposes, and all transitive dependencies of R are required at linking time. The above holds true within a single final linking target, such as an executable or shared library. However currently the rlibs inside a shared library get linked again into anything that depends on the shared library. This results in linking issues, such as missing symbols. executable(A) -> dylib(B) -> rlib(C) -> rlib(D) If A wants to use C or D, and they are public dependencies, it does not need to link them into A. They are already linked into B and A will be linked against B. I believe this becomes a more visible problem when C or D depends on a C++ static library which has non-public symbols, as then A ends up incorrectly linking C without linking its non-Rust dependencies too. Bug: 276 Change-Id: I8ee2c1d89190920cb5a9eee0e0b6da8a18d4a235 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/13080 Reviewed-by: Tyler Mandry <tmandry@google.com> Reviewed-by: Brett Wilson <brettw@google.com> Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Brett Wilson <brettw@chromium.org>
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc index 3321191..438e185 100644 --- a/src/gn/ninja_c_binary_target_writer.cc +++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -775,12 +775,13 @@ std::copy(input_deps.begin(), input_deps.end(), std::back_inserter(implicit_deps)); - // Any C++ target which depends on a Rust .rlib has to depend on its - // entire tree of transitive rlibs. + // Any C++ target which depends on a Rust .rlib has to depend on its entire + // tree of transitive rlibs found inside the linking target (which excludes + // rlibs only depended on inside a shared library dependency). std::vector<OutputFile> transitive_rustlibs; if (target_->IsFinal()) { for (const auto* dep : - target_->rust_transitive_inherited_libs().GetOrdered()) { + target_->rust_linkable_inherited_libs().GetOrdered()) { if (dep->output_type() == Target::RUST_LIBRARY) { transitive_rustlibs.push_back(dep->dependency_output_file()); implicit_deps.push_back(dep->dependency_output_file());
diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc index 61bc517..e1bdbcc 100644 --- a/src/gn/ninja_c_binary_target_writer_unittest.cc +++ b/src/gn/ninja_c_binary_target_writer_unittest.cc
@@ -1500,6 +1500,110 @@ } } +TEST_F(NinjaCBinaryTargetWriterTest, RustDepsOverDynamicLinking) { + Err err; + TestWithScope setup; + + Target rlib3(setup.settings(), Label(SourceDir("//baz/"), "baz")); + rlib3.set_output_type(Target::RUST_LIBRARY); + rlib3.visibility().SetPublic(); + SourceFile lib3("//baz/lib.rs"); + rlib3.sources().push_back(lib3); + rlib3.source_types_used().Set(SourceFile::SOURCE_RS); + rlib3.rust_values().set_crate_root(lib3); + rlib3.rust_values().crate_name() = "baz"; + rlib3.public_deps().push_back(LabelTargetPair(&rlib3)); + rlib3.SetToolchain(setup.toolchain()); + ASSERT_TRUE(rlib3.OnResolved(&err)); + + Target rlib2(setup.settings(), Label(SourceDir("//bar/"), "bar")); + rlib2.set_output_type(Target::RUST_LIBRARY); + rlib2.visibility().SetPublic(); + SourceFile lib2("//bar/lib.rs"); + rlib2.sources().push_back(lib2); + rlib2.source_types_used().Set(SourceFile::SOURCE_RS); + rlib2.rust_values().set_crate_root(lib2); + rlib2.rust_values().crate_name() = "bar"; + rlib2.public_deps().push_back(LabelTargetPair(&rlib2)); + rlib2.SetToolchain(setup.toolchain()); + ASSERT_TRUE(rlib2.OnResolved(&err)); + + Target rlib(setup.settings(), Label(SourceDir("//foo/"), "foo")); + rlib.set_output_type(Target::RUST_LIBRARY); + rlib.visibility().SetPublic(); + SourceFile lib("//foo/lib.rs"); + rlib.sources().push_back(lib); + rlib.source_types_used().Set(SourceFile::SOURCE_RS); + rlib.rust_values().set_crate_root(lib); + rlib.rust_values().crate_name() = "foo"; + rlib.public_deps().push_back(LabelTargetPair(&rlib2)); + rlib.SetToolchain(setup.toolchain()); + ASSERT_TRUE(rlib.OnResolved(&err)); + + Target cdylib(setup.settings(), Label(SourceDir("//sh/"), "mylib")); + cdylib.set_output_type(Target::SHARED_LIBRARY); + cdylib.visibility().SetPublic(); + SourceFile barlib("//sh/lib.rs"); + cdylib.sources().push_back(barlib); + cdylib.source_types_used().Set(SourceFile::SOURCE_RS); + cdylib.rust_values().set_crate_type(RustValues::CRATE_CDYLIB); + cdylib.rust_values().set_crate_root(barlib); + cdylib.rust_values().crate_name() = "mylib"; + cdylib.private_deps().push_back(LabelTargetPair(&rlib)); + cdylib.public_deps().push_back(LabelTargetPair(&rlib3)); + cdylib.SetToolchain(setup.toolchain()); + ASSERT_TRUE(cdylib.OnResolved(&err)); + + Target nearrlib(setup.settings(), Label(SourceDir("//near/"), "near")); + nearrlib.set_output_type(Target::RUST_LIBRARY); + nearrlib.visibility().SetPublic(); + SourceFile nearlib("//near/lib.rs"); + nearrlib.sources().push_back(nearlib); + nearrlib.source_types_used().Set(SourceFile::SOURCE_RS); + nearrlib.rust_values().set_crate_root(nearlib); + nearrlib.rust_values().crate_name() = "near"; + nearrlib.public_deps().push_back(LabelTargetPair(&cdylib)); + nearrlib.SetToolchain(setup.toolchain()); + ASSERT_TRUE(nearrlib.OnResolved(&err)); + + Target target(setup.settings(), Label(SourceDir("//exe/"), "binary")); + target.set_output_type(Target::EXECUTABLE); + target.visibility().SetPublic(); + target.sources().push_back(SourceFile("//exe/main.cc")); + target.source_types_used().Set(SourceFile::SOURCE_CPP); + target.private_deps().push_back(LabelTargetPair(&nearrlib)); + target.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target.OnResolved(&err)); + + std::ostringstream out; + NinjaCBinaryTargetWriter writer(&target, out); + writer.Run(); + + const char expected[] = + "defines =\n" + "include_dirs =\n" + "cflags =\n" + "cflags_cc =\n" + "root_out_dir = .\n" + "target_out_dir = obj/exe\n" + "target_output_name = binary\n" + "\n" + "build obj/exe/binary.main.o: cxx ../../exe/main.cc\n" + "\n" + "build ./binary: link obj/exe/binary.main.o obj/sh/libmylib.so | " + "obj/near/libnear.rlib\n" + " ldflags =\n" + " libs =\n" + " frameworks =\n" + " swiftmodules =\n" + " output_extension = \n" + " output_dir = \n" + " rlibs = obj/near/libnear.rlib\n"; + + std::string out_str = out.str(); + EXPECT_EQ(expected, out_str) << expected << "\n" << out_str; +} + TEST_F(NinjaCBinaryTargetWriterTest, ModuleMapInStaticLibrary) { TestWithScope setup; Err err;
diff --git a/src/gn/target.cc b/src/gn/target.cc index c491897..53c6510 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -769,8 +769,7 @@ if (dep->output_type() == STATIC_LIBRARY || dep->output_type() == SHARED_LIBRARY || - dep->output_type() == RUST_LIBRARY || - dep->output_type() == GROUP) { + dep->output_type() == RUST_LIBRARY || dep->output_type() == GROUP) { // Here we have: `this` --[depends-on]--> `dep` // // The `this` target has direct access to `dep` since its a direct @@ -792,6 +791,20 @@ rust_transitive_inherited_libs_.Append(dep, true); rust_transitive_inheritable_libs_.Append(dep, is_public); } + if (!dep->IsFinal()) { + // A "final" dependency is not an rlib, so while a final dependency would be + // part of another linking target, we don't inherit it here. Example: + // + // [bin E] -> [rlib A] -> [so D] -> [rlib B] + // + // The [so D] is linked into [bin E] along with [rlib A]. But we didn't add + // the edge to [so D] as an inherited rlib. + rust_linkable_inherited_libs_.Append(dep, true); + // Gathers the set of rlibs that are part of the current target's final + // linking target. + rust_linkable_inherited_libs_.AppendInherited( + dep->rust_linkable_inherited_libs(), true); + } if (dep->output_type() == RUST_LIBRARY || RustValues::InferredCrateType(dep) == RustValues::CRATE_DYLIB) {
diff --git a/src/gn/target.h b/src/gn/target.h index de076a8..31d4e3a 100644 --- a/src/gn/target.h +++ b/src/gn/target.h
@@ -322,6 +322,12 @@ const InheritedLibraries& rust_transitive_inheritable_libs() const { return rust_transitive_inheritable_libs_; } + // The transitive closure of libraries that are depended on by this target + // and are part of the current linking step. Previously-linked libraries are + // not included. + const InheritedLibraries& rust_linkable_inherited_libs() const { + return rust_linkable_inherited_libs_; + } const UniqueVector<SourceDir>& all_lib_dirs() const { return all_lib_dirs_; } const UniqueVector<LibFile>& all_libs() const { return all_libs_; } @@ -534,6 +540,10 @@ // For each library marked public: "If you depend on me, you get access to // these targets." InheritedLibraries rust_transitive_inheritable_libs_; + // Lists all transitive libraries in the current linking target. Unlike the + // two sets above, this does not include libraries that have already been + // linked into a dependency. The public bit is ignored. + InheritedLibraries rust_linkable_inherited_libs_; // User for Swift targets. std::unique_ptr<SwiftValues> swift_values_;