From 54d76753a03e67a980b6cd27f51678cb22d3ef93 Mon Sep 17 00:00:00 2001 From: brettw Date: Thu, 16 Apr 2015 10:40:49 -0700 Subject: [PATCH] Propagate GN public shared libraries through shared libraries Previously all dependencies would stop propagating for link purposes when a shared library or executable boundary is reached. This patch makes public shared library dependencies propagate through shared libraries. Since the ability to use header files is propagated through these boundaries, so do the link dependencies. See the comment in target.cc for more details. BUG=475091 Review URL: https://codereview.chromium.org/1083663007 Cr-Commit-Position: refs/heads/master@{#325471} --- tools/gn/BUILD.gn | 3 + tools/gn/gn.gyp | 3 + tools/gn/inherited_libraries.cc | 76 +++++++++++++++++ tools/gn/inherited_libraries.h | 69 ++++++++++++++++ tools/gn/inherited_libraries_unittest.cc | 135 +++++++++++++++++++++++++++++++ tools/gn/ninja_binary_target_writer.cc | 3 +- tools/gn/target.cc | 74 +++++++++++------ tools/gn/target.h | 15 ++-- tools/gn/target_unittest.cc | 84 +++++++++++++++---- tools/gn/variables.cc | 21 +++-- 10 files changed, 426 insertions(+), 57 deletions(-) create mode 100644 tools/gn/inherited_libraries.cc create mode 100644 tools/gn/inherited_libraries.h create mode 100644 tools/gn/inherited_libraries_unittest.cc diff --git a/tools/gn/BUILD.gn b/tools/gn/BUILD.gn index ccbb6bb69e2d..a90456c8c82f 100644 --- a/tools/gn/BUILD.gn +++ b/tools/gn/BUILD.gn @@ -77,6 +77,8 @@ static_library("gn_lib") { "header_checker.h", "import_manager.cc", "import_manager.h", + "inherited_libraries.cc", + "inherited_libraries.h", "input_conversion.cc", "input_conversion.h", "input_file.cc", @@ -240,6 +242,7 @@ test("gn_unittests") { "functions_target_unittest.cc", "functions_unittest.cc", "header_checker_unittest.cc", + "inherited_libraries_unittest.cc", "input_conversion_unittest.cc", "label_pattern_unittest.cc", "label_unittest.cc", diff --git a/tools/gn/gn.gyp b/tools/gn/gn.gyp index ed2cf3d583b2..fca6943fd91e 100644 --- a/tools/gn/gn.gyp +++ b/tools/gn/gn.gyp @@ -79,6 +79,8 @@ 'header_checker.h', 'import_manager.cc', 'import_manager.h', + 'inherited_libraries.cc', + 'inherited_libraries.h', 'input_conversion.cc', 'input_conversion.h', 'input_file.cc', @@ -217,6 +219,7 @@ 'functions_target_unittest.cc', 'functions_unittest.cc', 'header_checker_unittest.cc', + 'inherited_libraries_unittest.cc', 'input_conversion_unittest.cc', 'label_pattern_unittest.cc', 'label_unittest.cc', diff --git a/tools/gn/inherited_libraries.cc b/tools/gn/inherited_libraries.cc new file mode 100644 index 000000000000..06ac9ae4480f --- /dev/null +++ b/tools/gn/inherited_libraries.cc @@ -0,0 +1,76 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "tools/gn/inherited_libraries.h" + +#include "tools/gn/target.h" + +InheritedLibraries::InheritedLibraries() { +} + +InheritedLibraries::~InheritedLibraries() { +} + +std::vector InheritedLibraries::GetOrdered() const { + std::vector result; + result.resize(map_.size()); + + // The indices in the map should be from 0 to the number of items in the + // map, so insert directly into the result (with some sanity checks). + for (const auto& pair : map_) { + size_t index = pair.second.index; + DCHECK(index < result.size()); + DCHECK(!result[index]); + result[index] = pair.first; + } + + return result; +} + +std::vector> +InheritedLibraries::GetOrderedAndPublicFlag() const { + std::vector> result; + result.resize(map_.size()); + + for (const auto& pair : map_) { + size_t index = pair.second.index; + DCHECK(index < result.size()); + DCHECK(!result[index].first); + result[index] = std::make_pair(pair.first, pair.second.is_public); + } + + return result; +} + +void InheritedLibraries::Append(const Target* target, bool is_public) { + // Try to insert a new node. + auto insert_result = map_.insert( + std::make_pair(target, Node(map_.size(), is_public))); + + if (!insert_result.second) { + // Element already present, insert failed and insert_result indicates the + // old one. The old one may need to have its public flag updated. + if (is_public) { + Node& existing_node = insert_result.first->second; + existing_node.is_public = true; + } + } +} + +void InheritedLibraries::AppendInherited(const InheritedLibraries& other, + bool is_public) { + // Append all items in order, mark them public only if the're already public + // and we're adding them publically. + for (const auto& cur : other.GetOrderedAndPublicFlag()) + Append(cur.first, is_public && cur.second); +} + +void InheritedLibraries::AppendPublicSharedLibraries( + const InheritedLibraries& other, + bool is_public) { + for (const auto& cur : other.GetOrderedAndPublicFlag()) { + if (cur.first->output_type() == Target::SHARED_LIBRARY && cur.second) + Append(cur.first, is_public); + } +} diff --git a/tools/gn/inherited_libraries.h b/tools/gn/inherited_libraries.h new file mode 100644 index 000000000000..84114db65953 --- /dev/null +++ b/tools/gn/inherited_libraries.h @@ -0,0 +1,69 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef TOOLS_GN_INHERITED_LIBRARIES_H_ +#define TOOLS_GN_INHERITED_LIBRARIES_H_ + +#include +#include +#include + +#include "base/basictypes.h" + +class Target; + +// Represents an ordered uniquified set of all shared/static libraries for +// a given target. These are pushed up the dependency tree. +// +// Maintaining the order is important so GN links all libraries in the same +// order specified in the build files. +// +// Since this list is uniquified, appending to the list will not actually +// append a new item if the target already exists. However, the existing one +// may have its is_public flag updated. "Public" always wins, so is_public will +// be true if any dependency with that name has been set to public. +class InheritedLibraries { + public: + InheritedLibraries(); + ~InheritedLibraries(); + + // Returns the list of dependencies in order, optionally with the flag + // indicating whether the dependency is public. + std::vector GetOrdered() const; + std::vector> GetOrderedAndPublicFlag() const; + + // Adds a single dependency to the end of the list. See note on adding above. + void Append(const Target* target, bool is_public); + + // Appends all items from the "other" list to the current one. The is_public + // parameter indicates how the current target depends on the items in + // "other". If is_public is true, the existing public flags of the appended + // items will be preserved (propogating the public-ness up the dependency + // chain). If is_public is false, all deps will be added as private since + // the current target isn't forwarding them. + void AppendInherited(const InheritedLibraries& other, bool is_public); + + // Like AppendInherited but only appends the items in "other" that are of + // type SHARED_LIBRARY and only when they're marked public. This is used + // to push shared libraries up the dependency chain, following only public + // deps, to dependent targets that need to use them. + void AppendPublicSharedLibraries(const InheritedLibraries& other, + bool is_public); + + private: + struct Node { + Node() : index(static_cast(-1)), is_public(false) {} + Node(size_t i, bool p) : index(i), is_public(p) {} + + size_t index; + bool is_public; + }; + + typedef std::map LibraryMap; + LibraryMap map_; + + DISALLOW_COPY_AND_ASSIGN(InheritedLibraries); +}; + +#endif // TOOLS_GN_INHERITED_LIBRARIES_H_ diff --git a/tools/gn/inherited_libraries_unittest.cc b/tools/gn/inherited_libraries_unittest.cc new file mode 100644 index 000000000000..51152095352b --- /dev/null +++ b/tools/gn/inherited_libraries_unittest.cc @@ -0,0 +1,135 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "testing/gtest/include/gtest/gtest.h" +#include "tools/gn/inherited_libraries.h" +#include "tools/gn/target.h" +#include "tools/gn/test_with_scope.h" + +namespace { + +// In these tests, Pair can't be used conveniently because the +// "const" won't be inferred and the types won't match. This helper makes the +// right type of pair with the const Target. +std::pair Pair(const Target* t, bool b) { + return std::pair(t, b); +} + +} // namespace + +TEST(InheritedLibraries, Unique) { + TestWithScope setup; + + Target a(setup.settings(), Label(SourceDir("//foo/"), "a")); + Target b(setup.settings(), Label(SourceDir("//foo/"), "b")); + + // Setup, add the two targets as private. + InheritedLibraries libs; + libs.Append(&a, false); + libs.Append(&b, false); + auto result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(2u, result.size()); + EXPECT_EQ(Pair(&a, false), result[0]); + EXPECT_EQ(Pair(&b, false), result[1]); + + // Add again as private, this should be a NOP. + libs.Append(&a, false); + libs.Append(&b, false); + result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(2u, result.size()); + EXPECT_EQ(Pair(&a, false), result[0]); + EXPECT_EQ(Pair(&b, false), result[1]); + + // Add as public, this should make both public. + libs.Append(&a, true); + libs.Append(&b, true); + result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(2u, result.size()); + EXPECT_EQ(Pair(&a, true), result[0]); + EXPECT_EQ(Pair(&b, true), result[1]); + + // Add again private, they should stay public. + libs.Append(&a, false); + libs.Append(&b, false); + result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(2u, result.size()); + EXPECT_EQ(Pair(&a, true), result[0]); + EXPECT_EQ(Pair(&b, true), result[1]); +} + +TEST(InheritedLibraries, AppendInherited) { + TestWithScope setup; + + Target a(setup.settings(), Label(SourceDir("//foo/"), "a")); + Target b(setup.settings(), Label(SourceDir("//foo/"), "b")); + Target w(setup.settings(), Label(SourceDir("//foo/"), "w")); + Target x(setup.settings(), Label(SourceDir("//foo/"), "x")); + Target y(setup.settings(), Label(SourceDir("//foo/"), "y")); + Target z(setup.settings(), Label(SourceDir("//foo/"), "z")); + + InheritedLibraries libs; + libs.Append(&a, false); + libs.Append(&b, false); + + // Appending these things with private inheritance should make them private, + // no matter how they're listed in the appended class. + InheritedLibraries append_private; + append_private.Append(&a, true); + append_private.Append(&b, false); + append_private.Append(&w, true); + append_private.Append(&x, false); + libs.AppendInherited(append_private, false); + + auto result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(4u, result.size()); + EXPECT_EQ(Pair(&a, false), result[0]); + EXPECT_EQ(Pair(&b, false), result[1]); + EXPECT_EQ(Pair(&w, false), result[2]); + EXPECT_EQ(Pair(&x, false), result[3]); + + // Appending these things with public inheritance should convert them. + InheritedLibraries append_public; + append_public.Append(&a, true); + append_public.Append(&b, false); + append_public.Append(&y, true); + append_public.Append(&z, false); + libs.AppendInherited(append_public, true); + + result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(6u, result.size()); + EXPECT_EQ(Pair(&a, true), result[0]); // Converted to public. + EXPECT_EQ(Pair(&b, false), result[1]); + EXPECT_EQ(Pair(&w, false), result[2]); + EXPECT_EQ(Pair(&x, false), result[3]); + EXPECT_EQ(Pair(&y, true), result[4]); // Appended as public. + EXPECT_EQ(Pair(&z, false), result[5]); +} + +TEST(InheritedLibraries, AppendPublicSharedLibraries) { + TestWithScope setup; + InheritedLibraries append; + + // Two source sets. + Target set_pub(setup.settings(), Label(SourceDir("//foo/"), "set_pub")); + set_pub.set_output_type(Target::SOURCE_SET); + append.Append(&set_pub, true); + Target set_priv(setup.settings(), Label(SourceDir("//foo/"), "set_priv")); + set_priv.set_output_type(Target::SOURCE_SET); + append.Append(&set_priv, false); + + // Two shared libraries. + Target sh_pub(setup.settings(), Label(SourceDir("//foo/"), "sh_pub")); + sh_pub.set_output_type(Target::SHARED_LIBRARY); + append.Append(&sh_pub, true); + Target sh_priv(setup.settings(), Label(SourceDir("//foo/"), "sh_priv")); + sh_priv.set_output_type(Target::SHARED_LIBRARY); + append.Append(&sh_priv, false); + + InheritedLibraries libs; + libs.AppendPublicSharedLibraries(append, true); + + auto result = libs.GetOrderedAndPublicFlag(); + ASSERT_EQ(1u, result.size()); + EXPECT_EQ(Pair(&sh_pub, true), result[0]); +} diff --git a/tools/gn/ninja_binary_target_writer.cc b/tools/gn/ninja_binary_target_writer.cc index 84b819bcd239..a05bdd5db355 100644 --- a/tools/gn/ninja_binary_target_writer.cc +++ b/tools/gn/ninja_binary_target_writer.cc @@ -383,7 +383,8 @@ void NinjaBinaryTargetWriter::GetDeps( } // Inherited libraries. - for (const auto& inherited_target : target_->inherited_libraries()) { + for (const auto& inherited_target : + target_->inherited_libraries().GetOrdered()) { ClassifyDependency(inherited_target, extra_object_files, linkable_deps, non_linkable_deps); } diff --git a/tools/gn/target.cc b/tools/gn/target.cc index d5615c061aa1..40c8a21e2adf 100644 --- a/tools/gn/target.cc +++ b/tools/gn/target.cc @@ -126,7 +126,7 @@ bool Target::OnResolved(Err* err) { all_libs_.append(cur.libs().begin(), cur.libs().end()); } - PullDependentTargetInfo(); + PullDependentTargets(); PullForwardedDependentConfigs(); PullRecursiveHardDeps(); @@ -208,32 +208,56 @@ bool Target::SetToolchain(const Toolchain* toolchain, Err* err) { return false; } -void Target::PullDependentTargetInfo() { - // Gather info from our dependents we need. - for (const auto& pair : GetDeps(DEPS_LINKED)) { - const Target* dep = pair.ptr; - MergeAllDependentConfigsFrom(dep, &configs_, &all_dependent_configs_); - MergePublicConfigsFrom(dep, &configs_); - - // Direct dependent libraries. - if (dep->output_type() == STATIC_LIBRARY || - dep->output_type() == SHARED_LIBRARY || - dep->output_type() == SOURCE_SET) - inherited_libraries_.push_back(dep); - - // Inherited libraries and flags are inherited across static library - // boundaries. - if (!dep->IsFinal()) { - inherited_libraries_.Append(dep->inherited_libraries().begin(), - dep->inherited_libraries().end()); - - // Inherited library settings. - all_lib_dirs_.append(dep->all_lib_dirs()); - all_libs_.append(dep->all_libs()); - } +void Target::PullDependentTarget(const Target* dep, bool is_public) { + MergeAllDependentConfigsFrom(dep, &configs_, &all_dependent_configs_); + MergePublicConfigsFrom(dep, &configs_); + + // Direct dependent libraries. + if (dep->output_type() == STATIC_LIBRARY || + dep->output_type() == SHARED_LIBRARY || + dep->output_type() == SOURCE_SET) + inherited_libraries_.Append(dep, is_public); + + if (dep->output_type() == SHARED_LIBRARY) { + // Shared library dependendencies are inherited across public shared + // library boundaries. + // + // In this case: + // EXE -> INTERMEDIATE_SHLIB --[public]--> FINAL_SHLIB + // The EXE will also link to to FINAL_SHLIB. The public dependeny means + // that the EXE can use the headers in FINAL_SHLIB so the FINAL_SHLIB + // will need to appear on EXE's link line. + // + // However, if the dependency is private: + // EXE -> INTERMEDIATE_SHLIB --[private]--> FINAL_SHLIB + // the dependency will not be propogated because INTERMEDIATE_SHLIB is + // not granting permission to call functiosn from FINAL_SHLIB. If EXE + // wants to use functions (and link to) FINAL_SHLIB, it will need to do + // so explicitly. + // + // Static libraries and source sets aren't inherited across shared + // library boundaries because they will be linked into the shared + // library. + inherited_libraries_.AppendPublicSharedLibraries( + dep->inherited_libraries(), is_public); + } else if (!dep->IsFinal()) { + // The current target isn't linked, so propogate linked deps and + // libraries up the dependency tree. + inherited_libraries_.AppendInherited(dep->inherited_libraries(), is_public); + + // Inherited library settings. + all_lib_dirs_.append(dep->all_lib_dirs()); + all_libs_.append(dep->all_libs()); } } +void Target::PullDependentTargets() { + for (const auto& dep : public_deps_) + PullDependentTarget(dep.ptr, true); + for (const auto& dep : private_deps_) + PullDependentTarget(dep.ptr, false); +} + void Target::PullForwardedDependentConfigs() { // Pull public configs from each of our dependency's public deps. for (const auto& dep : public_deps_) @@ -377,7 +401,7 @@ bool Target::CheckNoNestedStaticLibs(Err* err) const { } // Verify no inherited libraries are static libraries. - for (const auto& lib : inherited_libraries()) { + for (const auto& lib : inherited_libraries().GetOrdered()) { if (lib->output_type() == Target::STATIC_LIBRARY) { *err = MakeStaticLibDepsError(this, lib); return false; diff --git a/tools/gn/target.h b/tools/gn/target.h index 440fc49ae558..081f81ee2c64 100644 --- a/tools/gn/target.h +++ b/tools/gn/target.h @@ -15,6 +15,7 @@ #include "base/synchronization/lock.h" #include "tools/gn/action_values.h" #include "tools/gn/config_values.h" +#include "tools/gn/inherited_libraries.h" #include "tools/gn/item.h" #include "tools/gn/label_ptr.h" #include "tools/gn/ordered_set.h" @@ -189,7 +190,7 @@ class Target : public Item { return allow_circular_includes_from_; } - const UniqueVector& inherited_libraries() const { + const InheritedLibraries& inherited_libraries() const { return inherited_libraries_; } @@ -241,7 +242,8 @@ class Target : public Item { private: // Pulls necessary information from dependencies to this one when all // dependencies have been resolved. - void PullDependentTargetInfo(); + void PullDependentTarget(const Target* dep, bool is_public); + void PullDependentTargets(); // These each pull specific things from dependencies to this one when all // deps have been resolved. @@ -281,12 +283,9 @@ class Target : public Item { std::set