From 9d45f82f821f0663a7c21c69c26d93fa0613d48a Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 4 May 2021 12:32:07 +0200 Subject: Handle duplicate suppression of multi-element libraries (-l foo) See GitHub issue #114 for context. --- libbuild2/cc/common.cxx | 148 +++++++++++++++++++++++++++++++++++------ libbuild2/cc/common.hxx | 4 +- libbuild2/cc/compile-rule.cxx | 9 +-- libbuild2/cc/link-rule.cxx | 122 ++++++++++++++++++--------------- libbuild2/cc/link-rule.hxx | 60 +++++++++++------ libbuild2/cc/pkgconfig.cxx | 17 +++-- libbuild2/cc/windows-rpath.cxx | 95 ++++++++++++++------------ 7 files changed, 305 insertions(+), 150 deletions(-) diff --git a/libbuild2/cc/common.cxx b/libbuild2/cc/common.cxx index f307b77..dabc007 100644 --- a/libbuild2/cc/common.cxx +++ b/libbuild2/cc/common.cxx @@ -32,10 +32,6 @@ namespace build2 // *.export.libs is up-to-date (which will happen automatically if it is // listed as a prerequisite of this library). // - // Storing a reference to library path in proc_lib is legal (it comes - // either from the target's path or from one of the *.libs variables - // neither of which should change on this run). - // // Note that the order of processing is: // // 1. options (x.* then cc.* to be consistent with poptions/loptions) @@ -47,7 +43,23 @@ namespace build2 // array that contains the current library dependency chain all the way to // the library passed to process_libraries(). The first element of this // array is NULL. If this argument is NULL, then this is a library without - // a target (e.g., -lpthread) and its name is in the second argument. + // a target (e.g., -lpthread) and its name is in the second argument + // (which could be resolved to an absolute path or passed as an -l + // option). Otherwise, (the first argument is not NULL), the second + // argument contains the target path (which can be empty in case of the + // unknown DLL path). + // + // Initially, the second argument (library name) was a string (e.g., + // -lpthread) but there are cases where the library is identified with + // multiple options, such as -framework CoreServices (there are also cases + // like -Wl,--whole-archive -lfoo -lbar -Wl,--no-whole-archive). So now it + // is a vector_view that contains a fragment of options (from one of the + // *.libs variables) that corresponds to the library (or several + // libraries, as in the --whole-archive example above). + // + // Storing a reference to elements of library name in proc_lib is legal + // (they come either from the target's path or from one of the *.libs + // variables neither of which should change on this run). // // If proc_impl always returns false (that is, we are only interested in // interfaces), then top_li can be absent. This makes process_libraries() @@ -69,15 +81,16 @@ namespace build2 bool la, lflags lf, const function& proc_impl, // Implementation? - const function& proc_lib, // True if system library. + bool la)>& proc_impl, // Implementation? + const function, 2>&, // Library "name". + lflags, // Link flags. + bool sys)>& proc_lib, // System library? const function& proc_opt, // *.export. + const string& type, // cc.type + bool com, // cc. or x. + bool exp)>& proc_opt, // *.export. bool self /*= false*/, // Call proc_lib on l? small_vector* chain) const { @@ -196,6 +209,8 @@ namespace build2 // Next process the library itself if requested. // + small_vector, 2> proc_lib_name; // Reuse. + if (self && proc_lib) { chain->push_back (&l); @@ -211,7 +226,8 @@ namespace build2 ? cast_false (l.vars[c_system]) : !p.empty () && sys (top_sysd, p.string ())); - proc_lib (&chain->back (), p.string (), lf, s); + proc_lib_name = {p.string ()}; + proc_lib (&chain->back (), proc_lib_name, lf, s); } const scope& bs (t == nullptr || cc ? top_bs : l.base_scope ()); @@ -299,17 +315,79 @@ namespace build2 return s; }; - auto proc_int = [&l, - &proc_impl, &proc_lib, &proc_opt, chain, + // Determine the length of the library name fragment as well as + // whether it is a system library. Possible length values are: + // + // 1 - just the argument itself (-lpthread) + // 2 - argument and next element (-l pthread, -framework CoreServices) + // 0 - unrecognized/until the end (-Wl,--whole-archive ...) + // + auto sense_fragment = [&sys_simple, this] (const string& l) -> + pair + { + size_t n; + bool s (true); + + if (tsys == "win32-msvc") + { + if (l[0] == '/') + { + // Some other option (e.g., /WHOLEARCHIVE:). + // + n = 0; + } + else + { + // Presumably a path. + // + n = 1; + s = sys_simple (l); + } + } + else + { + if (l[0] == '-') + { + // -l, -l + // + if (l[1] == 'l') + { + n = l.size () == 2 ? 2 : 1; + } + // -framework (Mac OS) + // + else if (tsys == "darwin" && l == "-framework") + { + n = 2; + } + // Some other option (e.g., -Wl,--whole-archive). + // + else + n = 0; + } + else + { + // Presumably a path. + // + n = 1; + s = sys_simple (l); + } + } + + return make_pair (n, s); + }; + + auto proc_int = [&l, chain, + &proc_impl, &proc_lib, &proc_lib_name, &proc_opt, &sysd, &usrd, - &find_sysd, &find_linfo, &sys_simple, + &find_sysd, &find_linfo, &sense_fragment, &bs, a, &li, impl, this] (const lookup& lu) { const vector* ns (cast_null> (lu)); if (ns == nullptr || ns->empty ()) return; - for (auto i (ns->begin ()), e (ns->end ()); i != e; ++i) + for (auto i (ns->begin ()), e (ns->end ()); i != e; ) { const name& n (*i); @@ -321,7 +399,20 @@ namespace build2 // files). // if (proc_lib) - proc_lib (nullptr, n.value, 0, sys_simple (n.value)); + { + pair r (sense_fragment (n.value)); + + proc_lib_name.clear (); + for (auto e1 (r.first != 0 ? i + r.first : e); + i != e && i != e1 && i->simple (); + ++i) + { + proc_lib_name.push_back (i->value); + } + + proc_lib (nullptr, proc_lib_name, 0, r.second); + continue; + } } else { @@ -380,23 +471,36 @@ namespace build2 t, t.is_a () || t.is_a (), 0, proc_impl, proc_lib, proc_opt, true, chain); } + + ++i; } }; // Process libraries from *.libs (of type strings). // - auto proc_imp = [&proc_lib, &sys_simple] (const lookup& lu) + auto proc_imp = [&proc_lib, &proc_lib_name, + &sense_fragment] (const lookup& lu) { const strings* ns (cast_null (lu)); if (ns == nullptr || ns->empty ()) return; - for (const string& n: *ns) + for (auto i (ns->begin ()), e (ns->end ()); i != e; ) { // This is something like -lpthread or shell32.lib so should be a // valid path. // - proc_lib (nullptr, n, 0, sys_simple (n)); + pair r (sense_fragment (*i)); + + proc_lib_name.clear (); + for (auto e1 (r.first != 0 ? i + r.first : e); + i != e && i != e1; + ++i) + { + proc_lib_name.push_back (*i); + } + + proc_lib (nullptr, proc_lib_name, 0, r.second); } }; diff --git a/libbuild2/cc/common.hxx b/libbuild2/cc/common.hxx index 758c675..77819db 100644 --- a/libbuild2/cc/common.hxx +++ b/libbuild2/cc/common.hxx @@ -282,7 +282,9 @@ namespace build2 bool, lflags, const function&, - const function&, + const function, 2>&, + lflags, bool)>&, const function&, bool = false, small_vector* = nullptr) const; diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx index ce586bb..40fb5bb 100644 --- a/libbuild2/cc/compile-rule.cxx +++ b/libbuild2/cc/compile-rule.cxx @@ -6193,10 +6193,11 @@ namespace build2 const target*& lt; } d {a, ht, lt}; - auto lib = [&d] (const target* const* lc, - const string&, - lflags, - bool) + auto lib = [&d] ( + const target* const* lc, + const small_vector, 2>&, + lflags, + bool) { // It's unfortunate we have no way to bail out. // diff --git a/libbuild2/cc/link-rule.cxx b/libbuild2/cc/link-rule.cxx index 0ab61b1..b4175d6 100644 --- a/libbuild2/cc/link-rule.cxx +++ b/libbuild2/cc/link-rule.cxx @@ -1558,10 +1558,11 @@ namespace build2 return la; }; - auto lib = [&d, this] (const target* const* lc, - const string& p, - lflags f, - bool) + auto lib = [&d, this] ( + const target* const* lc, + const small_vector, 2>& ns, + lflags f, + bool) { // Note: see also make_header_sidebuild(). @@ -1584,15 +1585,15 @@ namespace build2 // 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 ())); + appended_library* al (l != nullptr + ? &d.ls.append (*l, d.args.size ()) + : d.ls.append (ns, d.args.size ())); - if (al.end != appended_library::npos) // Closed. + if (al != nullptr && al->end != appended_library::npos) // Closed. { // Hoist the elements corresponding to this library to the end. // - if (al.begin != al.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 @@ -1600,23 +1601,23 @@ namespace build2 // 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, + rotate (d.args.begin () + al->begin, + d.args.begin () + al->end, d.args.end ()); - size_t n (al.end - al.begin); + size_t n (al->end - al->begin); for (appended_library& al1: d.ls) { - if (al1.begin >= al.end) + if (al1.begin >= al->end) { al1.begin -= n; al1.end -= n; } } - al.end = d.args.size (); - al.begin = al.end - n; + al->end = d.args.size (); + al->begin = al->end - n; } return; @@ -1628,7 +1629,10 @@ namespace build2 // static library. // if (d.li.type != otype::a) - d.args.push_back (p); + { + for (const string& n: ns) + d.args.push_back (n); + } } else { @@ -1742,7 +1746,8 @@ namespace build2 } done: - al.end = d.args.size (); // Close. + if (al != nullptr) + al->end = d.args.size (); // Close. }; auto opt = [&d, this] (const target& lt, @@ -1813,17 +1818,21 @@ namespace build2 return la; }; - auto lib = [&d, this] (const target* const* lc, - const string& p, - lflags f, - bool) + auto lib = [&d, this] ( + const target* const* lc, + const small_vector, 2>& ns, + lflags f, + bool) { const file* l (lc != nullptr ? &(*lc)->as () : nullptr); if (l == nullptr) { if (d.li.type != otype::a) - d.cs.append (p); + { + for (const string& n: ns) + d.cs.append (n); + } } else { @@ -1939,10 +1948,11 @@ namespace build2 bool link; } d {ls, args, link}; - auto lib = [&d, this] (const target* const* lc, - const string& f, - lflags, - bool sys) + auto lib = [&d, this] ( + const target* const* lc, + const small_vector, 2>& ns, + lflags, + bool sys) { const file* l (lc != nullptr ? &(*lc)->as () : nullptr); @@ -1952,6 +1962,17 @@ namespace build2 if (sys) return; + auto append = [&d] (const string& f) + { + string o (d.link ? "-Wl,-rpath-link," : "-Wl,-rpath,"); + + size_t p (path::traits_type::rfind_separator (f)); + assert (p != string::npos); + + o.append (f, 0, (p != 0 ? p : 1)); // Don't include trailing slash. + d.args.push_back (move (o)); + }; + if (l != nullptr) { if (!l->is_a ()) @@ -1965,10 +1986,11 @@ namespace build2 // 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); + if (find (d.ls.begin (), d.ls.end (), l) == d.ls.end ()) + { + append (ns[0]); + d.ls.push_back (l); + } } else { @@ -1977,36 +1999,28 @@ namespace build2 // better than checking for a platform-specific extension (maybe // we should cache it somewhere). // - size_t p (path::traits_type::find_extension (f)); + for (const string& f: ns) + { + size_t p (path::traits_type::find_extension (f)); - if (p == string::npos) - return; + if (p == string::npos) + return; - ++p; // Skip dot. + ++p; // Skip dot. - bool c (true); - const char* e; + bool c (true); + const char* e; - if (tclass == "windows") {e = "dll"; c = false;} - else if (tsys == "darwin") e = "dylib"; - else e = "so"; + if (tclass == "windows") {e = "dll"; c = false;} + else if (tsys == "darwin") e = "dylib"; + else e = "so"; - if ((c - ? f.compare (p, string::npos, e) - : icasecmp (f.c_str () + p, e)) != 0) - return; + if ((c + ? f.compare (p, string::npos, e) + : icasecmp (f.c_str () + p, e)) == 0) + append (f); + } } - - // Ok, if we are here then it means we have a non-system, shared - // library and its absolute path is in f. - // - string o (d.link ? "-Wl,-rpath-link," : "-Wl,-rpath,"); - - size_t p (path::traits_type::rfind_separator (f)); - assert (p != string::npos); - - o.append (f, 0, (p != 0 ? p : 1)); // Don't include trailing slash. - d.args.push_back (move (o)); }; diff --git a/libbuild2/cc/link-rule.hxx b/libbuild2/cc/link-rule.hxx index baccf8d..33a4f1c 100644 --- a/libbuild2/cc/link-rule.hxx +++ b/libbuild2/cc/link-rule.hxx @@ -58,8 +58,14 @@ namespace build2 { 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). + // Each appended_library represents either a library target or a + // library name fragment up to 2 elements long: + // + // target | name + // -------------------------------------------------- + const void* l1; // library target | library name[1] or NULL + const void* l2; // NULL | library name[0] + size_t begin; // First arg belonging to this library. size_t end; // Past last arg belonging to this library. }; @@ -73,38 +79,52 @@ namespace build2 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) + [&l] (const appended_library& al) { - return al.l == p; + return al.l2 == nullptr && al.l1 == &l; })); if (i != end ()) return *i; - push_back (appended_library {p, b, appended_library::npos}); + push_back (appended_library {&l, nullptr, b, appended_library::npos}); return back (); } - appended_library& - append (const string& l, size_t b) + // Return NULL if no duplicate tracking can be performed for this + // library. + // + appended_library* + append (const small_vector, 2>& ns, + 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)); - })); + size_t n (ns.size ()); + + if (n > 2) + return nullptr; + + auto i ( + find_if ( + begin (), end (), + [&ns, n] (const appended_library& al) + { + return al.l2 != nullptr && + *static_cast (al.l2) == ns[0].get () && + (n == 2 + ? (al.l1 != nullptr && + *static_cast (al.l1) == ns[1].get ()) + : al.l1 == nullptr); + })); if (i != end ()) - return *i; + return &*i; - push_back ( - appended_library { - reinterpret_cast (&l) | 1, b, appended_library::npos}); - return back (); + push_back (appended_library { + n == 2 ? &ns[1].get () : nullptr, &ns[0].get (), + b, appended_library::npos}); + + return &back (); } }; diff --git a/libbuild2/cc/pkgconfig.cxx b/libbuild2/cc/pkgconfig.cxx index d44b0ec..671a51a 100644 --- a/libbuild2/cc/pkgconfig.cxx +++ b/libbuild2/cc/pkgconfig.cxx @@ -1635,11 +1635,11 @@ namespace build2 bool priv (false); auto imp = [&priv] (const target&, bool la) {return priv && la;}; - auto lib = [&save_library_target, - &save_library_name] (const target* const* lc, - const string& p, - lflags, - bool) + auto lib = [&save_library_target, &save_library_name] ( + const target* const* lc, + const small_vector, 2>& ns, + lflags, + bool) { const file* l (lc != nullptr ? &(*lc)->as () : nullptr); @@ -1649,7 +1649,12 @@ namespace build2 save_library_target (*l); } else - save_library_name (p); // Something "system'y", save as is. + { + // Something "system'y", save as is. + // + for (const string& n: ns) + save_library_name (n); + } }; auto opt = [] (const target&, const string&, bool, bool) diff --git a/libbuild2/cc/windows-rpath.cxx b/libbuild2/cc/windows-rpath.cxx index 70eef73..7b572df 100644 --- a/libbuild2/cc/windows-rpath.cxx +++ b/libbuild2/cc/windows-rpath.cxx @@ -58,10 +58,11 @@ namespace build2 // auto imp = [] (const target&, bool) {return true;}; - auto lib = [&r] (const target* const* lc, - const string& f, - lflags, - bool sys) + auto lib = [&r] ( + const target* const* lc, + const small_vector, 2>& ns, + lflags, + bool sys) { const file* l (lc != nullptr ? &(*lc)->as () : nullptr); @@ -76,8 +77,13 @@ namespace build2 { // This can be an "undiscovered" DLL (see search_library()). // - if (!l->is_a () || l->path ().empty ()) // Also covers binless. - return; + if (l->is_a () && !l->path ().empty ()) // Also covers binless. + { + timestamp t (l->load_mtime ()); + + if (t > r) + r = t; + } } else { @@ -91,20 +97,19 @@ namespace build2 // // Though this can happen on MinGW with direct DLL link... // - size_t p (path::traits_type::find_extension (f)); - - if (p == string::npos || icasecmp (f.c_str () + p + 1, "dll") != 0) - return; - } + for (const string& f: ns) + { + size_t p (path::traits_type::find_extension (f)); - // Ok, this is a DLL. - // - timestamp t (l != nullptr - ? l->load_mtime () - : mtime (f.c_str ())); + if (p != string::npos && icasecmp (f.c_str () + p + 1, "dll") == 0) + { + timestamp t (mtime (f.c_str ())); - if (t > r) - r = t; + if (t > r) + r = t; + } + } + } }; for (const prerequisite_target& pt: t.prerequisite_targets[a]) @@ -139,10 +144,11 @@ namespace build2 auto imp = [] (const target&, bool) {return true;}; - auto lib = [&r, &bs] (const target* const* lc, - const string& f, - lflags, - bool sys) + auto lib = [&r, &bs] ( + const target* const* lc, + const small_vector, 2>& ns, + lflags, + bool sys) { const file* l (lc != nullptr ? &(*lc)->as () : nullptr); @@ -161,7 +167,7 @@ namespace build2 : nullptr); r.insert ( windows_dll { - f, + ns[0], pdb != nullptr ? &pdb->as ().path ().string () : nullptr, string () }); @@ -169,35 +175,38 @@ namespace build2 } else { - size_t p (path::traits_type::find_extension (f)); - - if (p != string::npos && icasecmp (f.c_str () + p + 1, "dll") == 0) + for (const string& f: ns) { - // See if we can find a corresponding .pdb. - // - windows_dll wd {f, nullptr, string ()}; - string& pdb (wd.pdb_storage); - - // First try "our" naming: foo.dll.pdb. - // - pdb = f; - pdb += ".pdb"; + size_t p (path::traits_type::find_extension (f)); - if (!exists (path (pdb))) + if (p != string::npos && icasecmp (f.c_str () + p + 1, "dll") == 0) { - // Then try the usual naming: foo.pdb. + // See if we can find a corresponding .pdb. // - pdb.assign (f, 0, p); + windows_dll wd {f, nullptr, string ()}; + string& pdb (wd.pdb_storage); + + // First try "our" naming: foo.dll.pdb. + // + pdb = f; pdb += ".pdb"; if (!exists (path (pdb))) - pdb.clear (); - } + { + // Then try the usual naming: foo.pdb. + // + pdb.assign (f, 0, p); + pdb += ".pdb"; - if (!pdb.empty ()) - wd.pdb = &pdb; + if (!exists (path (pdb))) + pdb.clear (); + } - r.insert (move (wd)); + if (!pdb.empty ()) + wd.pdb = &pdb; + + r.insert (move (wd)); + } } } }; -- cgit v1.1