From b7f4e6154fa3c07ad3472bbd7c871df28d440a64 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Mon, 23 Nov 2020 10:22:48 +0200 Subject: Suppress duplicates when extracting library options (GitHub issue #114) --- libbuild2/cc/compile-rule.cxx | 77 +++++++++++++------- libbuild2/cc/compile-rule.hxx | 26 +++---- libbuild2/cc/functions.cxx | 72 +++++++++++++------ libbuild2/cc/link-rule.cxx | 162 +++++++++++++++++++++++++++++++++--------- libbuild2/cc/link-rule.hxx | 60 +++++++++++++++- 5 files changed, 298 insertions(+), 99 deletions(-) diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx index 6ddff08..a2ff1bd 100644 --- a/libbuild2/cc/compile-rule.cxx +++ b/libbuild2/cc/compile-rule.cxx @@ -376,20 +376,26 @@ namespace build2 } // Append or hash library options from a pair of *.export.* variables - // (first one is cc.export.*) recursively, prerequisite libraries first. + // (first is x.* then cc.*) recursively, prerequisite libraries first. // template void compile_rule:: - append_lib_options (T& args, - const scope& bs, - action a, const file& l, bool la, linfo li) const + append_library_options (appended_libraries& ls, T& args, + const scope& bs, + action a, const file& l, bool la, linfo li) const { + struct data + { + appended_libraries& ls; + T& args; + } d {ls, args}; + // See through utility libraries. // auto imp = [] (const file& l, bool la) {return la && l.is_a ();}; - auto opt = [&args, this] ( - const file& l, const string& t, bool com, bool exp) + auto opt = [&d, this] (const file& l, + const string& t, bool com, bool exp) { // Note that in our model *.export.poptions are always "interface", // even if set on liba{}/libs{}, unlike loptions. @@ -397,6 +403,15 @@ namespace build2 if (!exp) // Ignore libux. return; + // Suppress duplicates. + // + // Compilation is the simple case: we can add the options on the first + // occurrence of the library and ignore all subsequent occurrences. + // See GitHub issue #114 for details. + // + if (find (d.ls.begin (), d.ls.end (), &l) != d.ls.end ()) + return; + const variable& var ( com ? c_export_poptions @@ -404,7 +419,13 @@ namespace build2 ? x_export_poptions : l.ctx.var_pool[t + ".export.poptions"])); - append_options (args, l, var); + append_options (d.args, l, var); + + // From the process_libraries() semantics we know that the final call + // is always for the common options. + // + if (com) + d.ls.push_back (&l); }; process_libraries (a, bs, li, sys_lib_dirs, @@ -413,19 +434,21 @@ namespace build2 } void compile_rule:: - append_lib_options (strings& args, - const scope& bs, - action a, const file& l, bool la, linfo li) const + append_library_options (appended_libraries& ls, strings& args, + const scope& bs, + action a, const file& l, bool la, linfo li) const { - append_lib_options (args, bs, a, l, la, li); + append_library_options (ls, args, bs, a, l, la, li); } template void compile_rule:: - append_lib_options (T& args, - const scope& bs, - action a, const target& t, linfo li) const + append_library_options (T& args, + const scope& bs, + action a, const target& t, linfo li) const { + appended_libraries ls; + for (prerequisite_member p: group_prerequisite_members (a, t)) { if (include (a, t, p) != include_type::normal) // Excluded/ad hoc. @@ -444,7 +467,7 @@ namespace build2 (la = (f = pt->is_a ())) || ( (f = pt->is_a ()))) { - append_lib_options (args, bs, a, *f, la, li); + append_library_options (ls, args, bs, a, *f, la, li); } } } @@ -454,11 +477,11 @@ namespace build2 // recursively, prerequisite libraries first. // void compile_rule:: - append_lib_prefixes (prefix_map& m, - const scope& bs, - action a, - target& t, - linfo li) const + append_library_prefixes (prefix_map& m, + const scope& bs, + action a, + target& t, + linfo li) const { auto imp = [] (const file& l, bool la) {return la && l.is_a ();}; @@ -478,7 +501,7 @@ namespace build2 append_prefixes (m, l, var); }; - // The same logic as in append_lib_options(). + // The same logic as in append_library_options(). // const function impf (imp); const function optf (opt); @@ -711,7 +734,7 @@ namespace build2 // A dependency on a library is there so that we can get its // *.export.poptions, modules, etc. This is the library metadata - // protocol. See also append_lib_options(). + // protocol. See also append_library_options(). // if (pi == include_type::normal && (p.is_a () || @@ -899,7 +922,7 @@ namespace build2 // Hash *.export.poptions from prerequisite libraries. // - append_lib_options (cs, bs, a, t, li); + append_library_options (cs, bs, a, t, li); } append_options (cs, t, c_coptions); @@ -1485,7 +1508,7 @@ namespace build2 // Then process the include directories from prerequisite libraries. // - append_lib_prefixes (m, bs, a, t, li); + append_library_prefixes (m, bs, a, t, li); return m; } @@ -2917,7 +2940,7 @@ namespace build2 // Add *.export.poptions from prerequisite libraries. // - append_lib_options (args, bs, a, t, li); + append_library_options (args, bs, a, t, li); // Populate the src-out with the -I$out_base -I$src_base pairs. // @@ -4313,7 +4336,7 @@ namespace build2 append_options (args, t, x_poptions); append_options (args, t, c_poptions); - append_lib_options (args, t.base_scope (), a, t, li); + append_library_options (args, t.base_scope (), a, t, li); if (md.symexport) append_symexport_options (args, t); @@ -5940,7 +5963,7 @@ namespace build2 // Add *.export.poptions from prerequisite libraries. // - append_lib_options (args, bs, a, t, li); + append_library_options (args, bs, a, t, li); if (md.symexport) append_symexport_options (args, t); diff --git a/libbuild2/cc/compile-rule.hxx b/libbuild2/cc/compile-rule.hxx index cbbb142..e52fed9 100644 --- a/libbuild2/cc/compile-rule.hxx +++ b/libbuild2/cc/compile-rule.hxx @@ -54,10 +54,12 @@ namespace build2 perform_clean (action, const target&) const; public: + using appended_libraries = small_vector; + void - append_lib_options (strings&, - const scope&, - action, const file&, bool, linfo) const; + append_library_options (appended_libraries&, strings&, + const scope&, + action, const file&, bool, linfo) const; protected: static void functions (function_family&, const char*); // functions.cxx @@ -72,15 +74,15 @@ namespace build2 template void - append_lib_options (T&, - const scope&, - action, const file&, bool, linfo) const; + append_library_options (appended_libraries&, T&, + const scope&, + action, const file&, bool, linfo) const; template void - append_lib_options (T&, - const scope&, - action, const target&, linfo) const; + append_library_options (T&, + const scope&, + action, const target&, linfo) const; // Mapping of include prefixes (e.g., foo in ) for auto- // generated headers to directories where they will be generated. @@ -107,9 +109,9 @@ namespace build2 append_prefixes (prefix_map&, const target&, const variable&) const; void - append_lib_prefixes (prefix_map&, - const scope&, - action, target&, linfo) const; + append_library_prefixes (prefix_map&, + const scope&, + action, target&, linfo) const; prefix_map build_prefix_map (const scope&, action, target&, linfo) const; diff --git a/libbuild2/cc/functions.cxx b/libbuild2/cc/functions.cxx index ca93b63..78ee212 100644 --- a/libbuild2/cc/functions.cxx +++ b/libbuild2/cc/functions.cxx @@ -27,15 +27,16 @@ namespace build2 struct lib_data { const char* x; - void (*f) (strings&, + void (*f) (void*, strings&, const vector_view&, const module&, const scope&, action, const file&, bool, linfo); }; static value - lib_thunk (const scope* bs, - vector_view vs, - const function_overload& f) + lib_thunk_impl (void* ls, + const scope* bs, + vector_view vs, + const function_overload& f) { const lib_data& d (*reinterpret_cast (&f.data)); @@ -114,7 +115,7 @@ namespace build2 (la = (f = t.is_a ())) || ( (f = t.is_a ()))) { - d.f (r, vs, *m, *bs, a, *f, la, li); + d.f (ls, r, vs, *m, *bs, a, *f, la, li); } else fail << t << " is not a library target"; @@ -123,6 +124,16 @@ namespace build2 return value (move (r)); } + template + static value + lib_thunk (const scope* bs, + vector_view vs, + const function_overload& f) + { + L ls; + return lib_thunk_impl (&ls, bs, vs, f); + } + void compile_rule:: functions (function_family& f, const char* x) { @@ -132,19 +143,24 @@ namespace build2 // sources that depend on the specified libraries. The second argument // is the output target type (obje, objs, etc). // - // Note that this function can only be called during execution after all - // the specified library targets have been matched. Normally it is used - // in ad hoc recipes to implement custom compilation. + // Note that passing multiple targets at once is not a mere convenience: + // this also allows for more effective duplicate suppression. + // + // Note also that this function can only be called during execution + // after all the specified library targets have been matched. Normally + // it is used in ad hoc recipes to implement custom compilation. + // // f[".lib_poptions"].insert ( - &lib_thunk, + &lib_thunk, lib_data { x, - [] (strings& r, + [] (void* ls, strings& r, const vector_view&, const module& m, const scope& bs, action a, const file& l, bool la, linfo li) { - m.append_lib_options (r, bs, a, l, la, li); + m.append_library_options ( + *static_cast (ls), r, bs, a, l, la, li); }}); } @@ -164,16 +180,19 @@ namespace build2 // If the last argument is false, then do not return the specified // libraries themselves. // - // Note that this function can only be called during execution after all - // the specified library targets have been matched. Normally it is used - // in ad hoc recipes to implement custom linking. + // Note that passing multiple targets at once is not a mere convenience: + // this also allows for more effective duplicate suppression. + // + // Note also that this function can only be called during execution + // after all the specified library targets have been matched. Normally + // it is used in ad hoc recipes to implement custom linking. // f[".lib_libs"].insert, optional> ( - &lib_thunk, + &lib_thunk, lib_data { x, - [] (strings& r, + [] (void* ls, strings& r, const vector_view& vs, const module& m, const scope& bs, action a, const file& l, bool la, linfo li) { @@ -193,7 +212,9 @@ namespace build2 bool self (vs.size () > 3 ? convert (vs[3]) : true); - m.append_libraries (r, bs, a, l, la, lf, li, self); + m.append_libraries (*static_cast (ls), r, + bs, + a, l, la, lf, li, self); }}); // $.lib_rpaths(, [, [, ]]) @@ -209,22 +230,27 @@ namespace build2 // If the last argument is false, then do not return the options for the // specified libraries themselves. // - // Note that this function can only be called during execution after all - // the specified library targets have been matched. Normally it is used - // in ad hoc recipes to implement custom linking. + // Note that passing multiple targets at once is not a mere convenience: + // this also allows for more effective duplicate suppression. + + // Note also that this function can only be called during execution + // after all the specified library targets have been matched. Normally + // it is used in ad hoc recipes to implement custom linking. // f[".lib_rpaths"].insert, optional> ( - &lib_thunk, + &lib_thunk, lib_data { x, - [] (strings& r, + [] (void* ls, strings& r, const vector_view& vs, const module& m, const scope& bs, action a, const file& l, bool la, linfo li) { bool link (vs.size () > 2 ? convert (vs[2]) : false); bool self (vs.size () > 3 ? convert (vs[3]) : true); - m.rpath_libraries (r, bs, a, l, la, li, link, self); + m.rpath_libraries (*static_cast (ls), r, + bs, + a, l, la, li, link, self); }}); } } diff --git a/libbuild2/cc/link-rule.cxx b/libbuild2/cc/link-rule.cxx index 240881a..8175489 100644 --- a/libbuild2/cc/link-rule.cxx +++ b/libbuild2/cc/link-rule.cxx @@ -1489,19 +1489,20 @@ namespace build2 } void link_rule:: - append_libraries (strings& args, + append_libraries (appended_libraries& ls, strings& args, const scope& bs, action a, const file& l, bool la, lflags lf, linfo li, bool self) const { struct data { + appended_libraries& ls; strings& args; const file& l; action a; linfo li; compile_target_types tts; - } d {args, l, a, li, compile_types (li.type)}; + } d {ls, args, l, a, li, compile_types (li.type)}; auto imp = [] (const file&, bool la) { @@ -1515,6 +1516,61 @@ namespace build2 { const file* l (lc != nullptr ? *lc : nullptr); + // Suppress duplicates. + // + // Linking is the complicated case: we cannot add the libraries and + // options on the first occurrence of the library and ignore all + // subsequent occurrences because of the static linking semantics. + // Instead, we can ignore all the occurrences except the last which + // would normally be done with a pre-pass but would also complicate + // things quite a bit. So instead we are going to keep track of the + // duplicates like we've done in other places but in addition we will + // also keep track of the elements added to args corresponding to this + // library. And whenever we see a duplicate, we are going to "hoist" + // that range of elements to the end of args. See GitHub issue #114 + // for details. + // + // From the process_libraries() semantics we know that this callback + // is always called and always after the options callbacks. + // + appended_library& al (l != nullptr + ? d.ls.append (*l, d.args.size ()) + : d.ls.append (p, d.args.size ())); + + if (al.end != appended_library::npos) // Closed. + { + // Hoist the elements corresponding to this library to the end. + // + if (al.begin != al.end) + { + // Rotate to the left the subrange starting from the first element + // of this library and until the end so that the element after the + // last element of this library becomes the first element of this + // subrange. We also need to adjust begin/end of libraries + // affected by the rotation. + // + rotate (d.args.begin () + al.begin, + d.args.begin () + al.end, + d.args.end ()); + + size_t n (al.end - al.begin); + + for (appended_library& al1: d.ls) + { + if (al1.begin >= al.end) + { + al1.begin -= n; + al1.end -= n; + } + } + + al.end = d.args.size (); + al.begin = al.end - n; + } + + return; + } + if (l == nullptr) { // Don't try to link a library (whether -lfoo or foo.lib) to a @@ -1542,7 +1598,7 @@ namespace build2 { for (ptrdiff_t i (-1); lc[i] != nullptr; --i) if (!lc[i]->is_a ()) - return; + goto done; } if (d.li.type == otype::a) @@ -1558,10 +1614,10 @@ namespace build2 // care of by perform_update()). So we cut it off here. // if (!lu) - return; + goto done; if (l->mtime () == timestamp_unreal) // Binless. - return; + goto done; for (const target* pt: l->prerequisite_targets[d.a]) { @@ -1593,7 +1649,7 @@ namespace build2 // if (l->mtime () == timestamp_unreal) // Binless. - return; + goto done; // On Windows a shared library is a DLL with the import library as // an ad hoc group member. MinGW though can link directly to DLLs @@ -1622,13 +1678,16 @@ namespace build2 d.args.push_back ("-Wl,--whole-archive"); d.args.push_back (move (p)); d.args.push_back ("-Wl,--no-whole-archive"); - return; + goto done; } } d.args.push_back (move (p)); } } + + done: + al.end = d.args.size (); // Close. }; auto opt = [&d, this] (const file& l, @@ -1649,6 +1708,11 @@ namespace build2 if (d.li.type == otype::a || !exp) return; + // Suppress duplicates. + // + if (d.ls.append (l, d.args.size ()).end != appended_library::npos) + return; + // If we need an interface value, then use the group (lib{}). // if (const target* g = exp && l.is_a () ? l.group : &l) @@ -1673,6 +1737,11 @@ namespace build2 const scope& bs, action a, const file& l, bool la, lflags lf, linfo li) const { + // Note that we don't do any duplicate suppression here: there is no way + // to "hoist" things once they are hashed and hashing only the first + // occurrence could miss changes to the command line (e.g., due to + // "hoisting"). + struct data { sha256& cs; @@ -1770,7 +1839,7 @@ namespace build2 } void link_rule:: - rpath_libraries (strings& args, + rpath_libraries (rpathed_libraries& ls, strings& args, const scope& bs, action a, const file& l, bool la, linfo li, bool link, bool self) const @@ -1808,9 +1877,10 @@ namespace build2 // struct { - strings& args; - bool link; - } d {args, link}; + rpathed_libraries& ls; + strings& args; + bool link; + } d {ls, args, link}; auto lib = [&d, this] (const file* const* lc, const string& f, @@ -1832,6 +1902,16 @@ namespace build2 if (l->mtime () == timestamp_unreal) // Binless. return; + + // Suppress duplicates. + // + // We handle rpath similar to the compilation case by adding the + // options on the first occurrence and ignoring all the subsequent. + // + if (find (d.ls.begin (), d.ls.end (), l) != d.ls.end ()) + return; + + d.ls.push_back (l); } else { @@ -1882,7 +1962,10 @@ namespace build2 // It is either matched or imported so should be a cc library. // if (!cast_false (l.vars[c_system])) + { args.push_back ("-Wl,-rpath," + l.path ().directory ().string ()); + ls.push_back (&l); + } } } @@ -1896,6 +1979,8 @@ namespace build2 const scope& bs, action a, const target& t, linfo li, bool link) const { + rpathed_libraries ls; + for (const prerequisite_target& pt: t.prerequisite_targets[a]) { if (pt == nullptr) @@ -1908,7 +1993,7 @@ namespace build2 (la = (f = pt->is_a ())) || ( f = pt->is_a ())) { - rpath_libraries (args, bs, a, *f, la, li, link, true); + rpath_libraries (ls, args, bs, a, *f, la, li, link, true); } } } @@ -2862,35 +2947,42 @@ namespace build2 // inside append_libraries(). // bool seen_obj (false); - for (const prerequisite_target& p: t.prerequisite_targets[a]) { - const target* pt (p.target); + appended_libraries als; + for (const prerequisite_target& p: t.prerequisite_targets[a]) + { + const target* pt (p.target); - if (pt == nullptr) - continue; + if (pt == nullptr) + continue; - if (modules) - { - if (pt->is_a ()) // @@ MODHDR: hbmix{} has no objx{} - pt = find_adhoc_member (*pt, tts.obj); - } + if (modules) + { + if (pt->is_a ()) // @@ MODHDR: hbmix{} has no objx{} + pt = find_adhoc_member (*pt, tts.obj); + } - const file* f; - bool la (false), ls (false); + const file* f; + bool la (false), ls (false); - if ((f = pt->is_a ()) || - (!lt.utility && - (la = (f = pt->is_a ()))) || - (!lt.static_library () && - ((la = (f = pt->is_a ())) || - (ls = (f = pt->is_a ()))))) - { - if (la || ls) - append_libraries (sargs, bs, a, *f, la, p.data, li); - else + if ((f = pt->is_a ()) || + (!lt.utility && + (la = (f = pt->is_a ()))) || + (!lt.static_library () && + ((la = (f = pt->is_a ())) || + (ls = (f = pt->is_a ()))))) { - sargs.push_back (relative (f->path ()).string ()); // string()&& - seen_obj = true; + if (la || ls) + append_libraries (als, sargs, bs, a, *f, la, p.data, li); + else + { + // Do not hoist libraries over object files since such object + // files might satisfy symbols in the preceding libraries. + // + als.clear (); + sargs.push_back (relative (f->path ()).string ()); // string()&& + seen_obj = true; + } } } } diff --git a/libbuild2/cc/link-rule.hxx b/libbuild2/cc/link-rule.hxx index a93defc..b369da6 100644 --- a/libbuild2/cc/link-rule.hxx +++ b/libbuild2/cc/link-rule.hxx @@ -54,8 +54,62 @@ namespace build2 public: // Library handling. // + struct appended_library + { + static const size_t npos = size_t (~0); + + uintptr_t l; // Pointer to library target (last bit 0) or to + // library name (-lpthread or path; last bit 1). + size_t begin = npos; // First arg belonging to this library. + size_t end = npos; // Past last arg belonging to this library. + }; + + class appended_libraries: public small_vector + { + public: + // Find existing or append new entry. If appending new, use the second + // argument as the begin value. + // + appended_library& + append (const file& l, size_t b) + { + auto p (reinterpret_cast (&l)); + auto i (find_if (begin (), end (), + [p] (const appended_library& al) + { + return al.l == p; + })); + + if (i != end ()) + return *i; + + push_back (appended_library {p, b, appended_library::npos}); + return back (); + } + + appended_library& + append (const string& l, size_t b) + { + auto i (find_if (begin (), end (), + [&l] (const appended_library& al) + { + return (al.l & 1) != 0 && + l == *reinterpret_cast ( + al.l & ~uintptr_t (1)); + })); + + if (i != end ()) + return *i; + + push_back ( + appended_library { + reinterpret_cast (&l) | 1, b, appended_library::npos}); + return back (); + } + }; + void - append_libraries (strings&, + append_libraries (appended_libraries&, strings&, const scope&, action, const file&, bool, lflags, linfo, bool = true) const; @@ -64,8 +118,10 @@ namespace build2 const scope&, action, const file&, bool, lflags, linfo) const; + using rpathed_libraries = small_vector; + void - rpath_libraries (strings&, + rpath_libraries (rpathed_libraries&, strings&, const scope&, action, const file&, bool, linfo, bool, bool) const; -- cgit v1.1