* [RFC] Move static archive dependencies into ld @ 2023-02-17 15:03 Michael Matz 2023-02-17 20:53 ` Howard Chu 0 siblings, 1 reply; 5+ messages in thread From: Michael Matz @ 2023-02-17 15:03 UTC (permalink / raw) To: binutils Hello, this is an RFC for now. When met with some acceptance I'll cobble up a similar thing for gold as well and then make the libdep.so plugin only emit a warning message that its functionality is obsoleted and moved into ld itself. As Cary put it back in 2020 when the plugin was added: ( https://sourceware.org/pipermail/binutils/2020-December/114536.html ) "Did I miss any discussion about whether this feature should/could be implemented directly in the linkers, rather than via a plugin? I wouldn't be opposed to it." I pretty much think that yes, the functionality absolutely needs to be in the linker itself, not in a plugin, at least if we want to see some use of it in the real world. Amusingly I was triggered to look into this by libbfd itself: Recently it got a dependency to libsframe and when shipping only static archives to build against (which we as distro consciously do) that leads to build errors in various 3rdparty packages wanting to link against libbfd (no matter how bad an idea that is). Now, adding a '-plugin $magicdir/libdep.so' to the build flags of each such package is no better than just adding '-lsframe' in each case, so the plugin really doesn't help anything. Had the functionality already been in the linker itself we could have just added "--record-libdeps='-lsframe'" when building libbfd.a and be done with it. So, let's try to prepare for the future and at least now make ld interpret such dependencies on its own. So, what do people think? Ciao, Michael. --------------------------------------- needing a plugin to interpret static archive dependencies (as added by 'ar --record-libdeps') effectively means they are unused. Which is too bad, because they are actually useful. So implement them in ld itself. ld/ldmain.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 143 insertions(+), 1 deletion(-) diff --git a/ld/ldmain.c b/ld/ldmain.c index 8c2fc9b8d8c..dc86ddf3ca8 100644 --- a/ld/ldmain.c +++ b/ld/ldmain.c @@ -876,6 +876,143 @@ add_keepsyms_file (const char *filename) link_info.strip = strip_some; fclose (file); } + +/* Turn a string into an argvec. */ + +static char ** +str2vec (char *in) +{ + char **res; + char *s, *first, *end; + char *sq, *dq; + int i; + + end = in + strlen (in); + s = in; + while (ISSPACE (*s)) s++; + first = s; + + i = 1; + while ((s = strchr (s, ' '))) + { + s++; + i++; + } + res = (char **)xmalloc ((i+1) * sizeof (char *)); + if (!res) + return res; + + i = 0; + sq = NULL; + dq = NULL; + res[0] = first; + for (s = first; *s; s++) + { + if (*s == '\\') + { + memmove (s, s+1, end-s-1); + end--; + } + if (ISSPACE (*s)) + { + if (sq || dq) + continue; + *s++ = '\0'; + while (ISSPACE (*s)) s++; + if (*s) + res[++i] = s; + } + if (*s == '\'' && !dq) + { + if (sq) + { + memmove (sq, sq+1, s-sq-1); + memmove (s-2, s+1, end-s-1); + end -= 2; + s--; + sq = NULL; + } + else + { + sq = s; + } + } + if (*s == '"' && !sq) + { + if (dq) + { + memmove (dq, dq+1, s-dq-1); + memmove (s-2, s+1, end-s-1); + end -= 2; + s--; + dq = NULL; + } + else + { + dq = s; + } + } + } + res[++i] = NULL; + return res; +} + +#define LIBDEPS "__.LIBDEP" + +/* Check if ARCHIVE has a '__.LIBDEP' member and if so interpret its + content as linker command line arguments (only -L and -l are accepted). + The special member is expected amongst the first few. */ + +static void +check_archive_deps (bfd *archive) +{ + int count = 3; + bfd *member; + + if (bfd_is_thin_archive (archive)) + return; + + /* We look for the magic member only in the first few archive + members. */ + member = bfd_openr_next_archived_file (archive, NULL); + while (member != NULL && count--) + { + ufile_ptr memsize = bfd_get_file_size (member); + if (memsize + && !strcmp (bfd_get_filename (member), LIBDEPS)) + { + char *buf = (char *) xmalloc (memsize); + if (buf + && bfd_seek (member, (file_ptr) 0, SEEK_SET) == 0 + && bfd_bread (buf, memsize, member) == memsize) + { + char **vec; + vec = str2vec (buf); + if (vec) + { + int i; + for (i = 0; vec[i]; i++) + { + if (vec[i][0] != '-') + einfo ("ignoring libdep argument %s", vec[i]); + else if (vec[i][1] == 'l') + lang_add_input_file (xstrdup (vec[i]+2), + lang_input_file_is_l_enum, + NULL); + else if (vec[i][1] == 'L') + ldfile_add_library_path (vec[i]+2, false); + else + einfo ("ignoring libdep argument %s", vec[i]); + } + free (vec); + } + } + free (buf); + break; + } + member = bfd_openr_next_archived_file (archive, member); + } +} \f /* Callbacks from the BFD linker routines. */ @@ -941,7 +1078,12 @@ add_archive_element (struct bfd_link_info *info, from the archive. See ldlang.c:find_rescan_insertion. */ parent = bfd_usrdata (abfd->my_archive); if (parent != NULL && !parent->flags.reload) - parent->next = input; + { + if (!parent->next) + /* The first time we see an archive use we check for dependencies. */ + check_archive_deps (abfd->my_archive); + parent->next = input; + } ldlang_add_file (input); -- 2.39.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Move static archive dependencies into ld 2023-02-17 15:03 [RFC] Move static archive dependencies into ld Michael Matz @ 2023-02-17 20:53 ` Howard Chu 2023-04-13 9:10 ` Clément Chigot 2023-06-05 14:16 ` Howard Chu 0 siblings, 2 replies; 5+ messages in thread From: Howard Chu @ 2023-02-17 20:53 UTC (permalink / raw) To: Michael Matz, binutils Michael Matz via Binutils wrote: > Hello, > > this is an RFC for now. When met with some acceptance I'll cobble up a > similar thing for gold as well and then make the libdep.so plugin only > emit a warning message that its functionality is obsoleted and moved > into ld itself. As Cary put it back in 2020 when the plugin was added: > > ( https://sourceware.org/pipermail/binutils/2020-December/114536.html ) > > "Did I miss any discussion about whether this feature should/could be > implemented directly in the linkers, rather than via a plugin? I > wouldn't be opposed to it." I'm all for it. I went the plugin route initially because it seemed the easiest way to get the feature written and accepted but I agree with you, it'd be much more useful if it was implicitly supported by default. > > I pretty much think that yes, the functionality absolutely needs to be in > the linker itself, not in a plugin, at least if we want to see some use of > it in the real world. Amusingly I was triggered to look into this by > libbfd itself: Recently it got a dependency to libsframe and when > shipping only static archives to build against (which we as distro > consciously do) that leads to build errors in various 3rdparty packages > wanting to link against libbfd (no matter how bad an idea that is). Now, > adding a '-plugin $magicdir/libdep.so' to the build flags of each such > package is no better than just adding '-lsframe' in each case, so the > plugin really doesn't help anything. Had the functionality already been > in the linker itself we could have just added > "--record-libdeps='-lsframe'" when building libbfd.a and be done with it. > > So, let's try to prepare for the future and at least now make ld interpret > such dependencies on its own. > > So, what do people think? > > > Ciao, > Michael. > > --------------------------------------- > > needing a plugin to interpret static archive dependencies (as added > by 'ar --record-libdeps') effectively means they are unused. > Which is too bad, because they are actually useful. So implement > them in ld itself. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Move static archive dependencies into ld 2023-02-17 20:53 ` Howard Chu @ 2023-04-13 9:10 ` Clément Chigot 2023-06-05 14:16 ` Howard Chu 1 sibling, 0 replies; 5+ messages in thread From: Clément Chigot @ 2023-04-13 9:10 UTC (permalink / raw) To: Howard Chu, Michael Matz; +Cc: binutils On Fri, Feb 17, 2023 at 9:53 PM Howard Chu <hyc@symas.com> wrote: > > Michael Matz via Binutils wrote: > > Hello, > > > > this is an RFC for now. When met with some acceptance I'll cobble up a > > similar thing for gold as well and then make the libdep.so plugin only > > emit a warning message that its functionality is obsoleted and moved > > into ld itself. As Cary put it back in 2020 when the plugin was added: > > > > ( https://sourceware.org/pipermail/binutils/2020-December/114536.html ) > > > > "Did I miss any discussion about whether this feature should/could be > > implemented directly in the linkers, rather than via a plugin? I > > wouldn't be opposed to it." > > I'm all for it. I went the plugin route initially because it seemed the > easiest way to get the feature written and accepted but I agree with you, > it'd be much more useful if it was implicitly supported by default. I do support this feature as well. As seen recently, the plugin is not correctly built when shared support is disabled. It means that a feature made to enhance static support does not work when only static support is available... I was planning to disable its creation (see [1]) because of potential harmful side effects on Windows. But this solution is far better. Thanks Chu for pointing it out to me. [1] https://sourceware.org/pipermail/binutils/2023-April/127045.html Thanks, Clément ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Move static archive dependencies into ld 2023-02-17 20:53 ` Howard Chu 2023-04-13 9:10 ` Clément Chigot @ 2023-06-05 14:16 ` Howard Chu 2023-06-05 15:32 ` Michael Matz 1 sibling, 1 reply; 5+ messages in thread From: Howard Chu @ 2023-06-05 14:16 UTC (permalink / raw) To: Michael Matz, binutils [-- Attachment #1: Type: text/plain, Size: 2485 bytes --] What needs to happen to move this RFC forward? There seems to be at least some support for the idea (attached) Howard Chu wrote: > Michael Matz via Binutils wrote: >> Hello, >> >> this is an RFC for now. When met with some acceptance I'll cobble up a >> similar thing for gold as well and then make the libdep.so plugin only >> emit a warning message that its functionality is obsoleted and moved >> into ld itself. As Cary put it back in 2020 when the plugin was added: >> >> ( https://sourceware.org/pipermail/binutils/2020-December/114536.html ) >> >> "Did I miss any discussion about whether this feature should/could be >> implemented directly in the linkers, rather than via a plugin? I >> wouldn't be opposed to it." > > I'm all for it. I went the plugin route initially because it seemed the > easiest way to get the feature written and accepted but I agree with you, > it'd be much more useful if it was implicitly supported by default. >> >> I pretty much think that yes, the functionality absolutely needs to be in >> the linker itself, not in a plugin, at least if we want to see some use of >> it in the real world. Amusingly I was triggered to look into this by >> libbfd itself: Recently it got a dependency to libsframe and when >> shipping only static archives to build against (which we as distro >> consciously do) that leads to build errors in various 3rdparty packages >> wanting to link against libbfd (no matter how bad an idea that is). Now, >> adding a '-plugin $magicdir/libdep.so' to the build flags of each such >> package is no better than just adding '-lsframe' in each case, so the >> plugin really doesn't help anything. Had the functionality already been >> in the linker itself we could have just added >> "--record-libdeps='-lsframe'" when building libbfd.a and be done with it. >> >> So, let's try to prepare for the future and at least now make ld interpret >> such dependencies on its own. >> >> So, what do people think? >> >> >> Ciao, >> Michael. >> >> --------------------------------------- >> >> needing a plugin to interpret static archive dependencies (as added >> by 'ar --record-libdeps') effectively means they are unused. >> Which is too bad, because they are actually useful. So implement >> them in ld itself. > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/ [-- Attachment #2: [PATCH] ld: build libdep plugin only when shared library support is enabled.eml --] [-- Type: message/rfc822, Size: 1379 bytes --] From: Howard Chu <hyc@symas.com> To: "Clément Chigot" <chigot@adacore.com>, binutils@sourceware.org Subject: Re: [PATCH] ld: build libdep plugin only when shared library support is enabled Date: Thu, 13 Apr 2023 09:54:02 +0100 Message-ID: <5216d67e-ab2b-8142-edd8-4cab38ee2dc6@symas.com> Clément Chigot via Binutils wrote: > Otherwise, libtool will create a static plugin which cannot be loaded. > Even worse on Windows, it will create a .a file, resulting in a similar > bug than PR 27113: some tools will try to open it and Windows will raise > a popup error. At this point I think it'd be better to integrate the libdep functionality directly into ld, as suggested before: https://sourceware.org/pipermail/binutils/2023-February/126155.html -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Move static archive dependencies into ld 2023-06-05 14:16 ` Howard Chu @ 2023-06-05 15:32 ` Michael Matz 0 siblings, 0 replies; 5+ messages in thread From: Michael Matz @ 2023-06-05 15:32 UTC (permalink / raw) To: Howard Chu; +Cc: binutils Heyho, On Mon, 5 Jun 2023, Howard Chu wrote: > What needs to happen to move this RFC forward? There seems to be at > least some support for the idea (attached) I was fiddling with also adding it to gold, and the support for threads there confused me, so I put it on hold a bit. I did hack on it February, and got it working, but couldn't really convince myself that I got the dealing with the work-queues right. See below. (Of the BFD part already send back then I'm convinced). So, if someone could look over the gold patch and tell me if the workqueue handling is somewhat okayish, we could proceed. (The "interesting" thing about this is that I need to add items to a work-queue while it is processed, _at the place right after it_, not merely scheduled to run "eventually") Ciao, Michael. ------------- diff --git a/gold/archive.cc b/gold/archive.cc index 3983995d09a..5a439cfa412 100644 --- a/gold/archive.cc +++ b/gold/archive.cc @@ -190,7 +190,7 @@ const char Archive::sym64name[7] = { '/', 'S', 'Y', 'M', '6', '4', '/' }; Archive::Archive(const std::string& name, Input_file* input_file, bool is_thin_archive, Dirsearch* dirpath, Task* task) - : Library_base(task), name_(name), input_file_(input_file), armap_(), + : Library_base(task), libdeps_(NULL), name_(name), input_file_(input_file), armap_(), armap_names_(), extended_names_(), armap_checked_(), seen_offsets_(), members_(), is_thin_archive_(is_thin_archive), included_member_(false), nested_archives_(), dirpath_(dirpath), num_members_(0), @@ -1005,6 +1005,7 @@ Archive::include_member(Symbol_table* symtab, Layout* layout, { obj->layout(symtab, layout, sd); obj->add_symbols(symtab, sd, layout); + this->maybe_handle_libdeps(); this->included_member_ = true; } delete sd; @@ -1039,6 +1040,7 @@ Archive::include_member(Symbol_table* symtab, Layout* layout, if (pluginobj != NULL) { pluginobj->add_symbols(symtab, NULL, layout); + this->maybe_handle_libdeps(); this->included_member_ = true; return true; } @@ -1059,10 +1061,33 @@ Archive::include_member(Symbol_table* symtab, Layout* layout, obj->add_symbols(symtab, &sd, layout); } + this->maybe_handle_libdeps(); this->included_member_ = true; return true; } +void +Archive::maybe_handle_libdeps(void) +{ + if (this->included_member_) + return; + if (this->is_thin_archive_) + return; + fprintf(stderr, "handle_libdeps in %s\n", name_.c_str()); + for (Archive::const_iterator p = this->begin(); + p != this->end(); + ++p) + { + fprintf(stderr, " %u %u %u %s\n", (unsigned)p->off, (unsigned)p->nested_off, (unsigned)p->size, p->name.c_str()); + if (p->name == "__.LIBDEP") + { + const unsigned char* s = this->get_view(p->off + sizeof(Archive_header), p->size, false, false); + fprintf(stderr, " got it: %s\n", s); + this->libdeps_ = reinterpret_cast<const char*>(s); + } + } +} + // Iterate over all unused symbols, and call the visitor class V for each. void @@ -1119,6 +1144,87 @@ Add_archive_symbols::locks(Task_locker* tl) tl->add(this, this->archive_->token()); } +/* Turn a string into an argvec. Copied from the original implementation + in the plugin, hence written in C. */ + +static char ** +str2vec (char *in) +{ + char **res; + char *s, *first, *end; + char *sq, *dq; + int i; + + end = in + strlen (in); + s = in; + while (isspace ((unsigned char) *s)) s++; + first = s; + + i = 1; + while ((s = strchr (s, ' '))) + { + s++; + i++; + } + res = (char **)malloc ((i+1) * sizeof (char *)); + if (!res) + return res; + + i = 0; + sq = NULL; + dq = NULL; + res[0] = first; + for (s = first; *s; s++) + { + if (*s == '\\') + { + memmove (s, s+1, end-s-1); + end--; + } + if (isspace ((unsigned char) *s)) + { + if (sq || dq) + continue; + *s++ = '\0'; + while (isspace ((unsigned char) *s)) s++; + if (*s) + res[++i] = s; + } + if (*s == '\'' && !dq) + { + if (sq) + { + memmove (sq, sq+1, s-sq-1); + memmove (s-2, s+1, end-s-1); + end -= 2; + s--; + sq = NULL; + } + else + { + sq = s; + } + } + if (*s == '"' && !sq) + { + if (dq) + { + memmove (dq, dq+1, s-dq-1); + memmove (s-2, s+1, end-s-1); + end -= 2; + s--; + dq = NULL; + } + else + { + dq = s; + } + } + } + res[++i] = NULL; + return res; +} + void Add_archive_symbols::run(Workqueue* workqueue) { @@ -1135,6 +1241,79 @@ Add_archive_symbols::run(Workqueue* workqueue) bool added = this->archive_->add_symbols(this->symtab_, this->layout_, this->input_objects_, this->mapfile_); + if (added && this->archive_->libdeps_) + { + std::vector<Input_argument *> newinputs; + char **vec; + vec = str2vec (strdup (this->archive_->libdeps_)); + if (vec) + { + std::string extra_search_path = ""; + int i; + for (i = 0; vec[i]; i++) + { + if (vec[i][0] != '-') + fprintf (stderr, "ignoring libdep argument %s", vec[i]); + else if (vec[i][1] == 'l') + { + Input_file_argument file( + vec[i]+2, + Input_file_argument::INPUT_FILE_TYPE_LIBRARY, + extra_search_path.c_str(), + false, + this->archive_->input_file()->options()); + Input_argument* input_argument = new Input_argument(file); + newinputs.push_back(input_argument); + } + else if (vec[i][1] == 'L') + extra_search_path = vec[i]+2; + else + fprintf (stderr, "ignoring libdep argument %s", vec[i]); + } + free (vec); + } + + Task_token* this_blocker = NULL; + for (std::vector<Input_argument*>::const_iterator i = newinputs.begin(); + i != newinputs.end(); + ++i) + { + const Input_argument* arg = *i; + + Task_token* next_blocker; + if (i != newinputs.end() - 1) + { + next_blocker = new Task_token(true); + next_blocker->add_blocker(); + //fprintf(stderr, "XXX create new blocker %p\n", next_blocker); + } + else + { + next_blocker = this->next_blocker_; + /* The originally next task is blocked on _this_ task (about + to end right soon) and the last inserted new task. */ + //next_blocker->add_blocker(); + workqueue->add_blocker(next_blocker); + //fprintf(stderr, "XXX reuse old blocker %p\n", next_blocker); + } + + //fprintf(stderr, "XXX queue with this_blocker %p\n", this_blocker); + workqueue->queue_soon(new Read_symbols(this->input_objects_, + this->symtab_, + this->layout_, + this->dirpath_, + 0, + this->mapfile_, + arg, + NULL, + NULL, + this_blocker, + next_blocker)); + this_blocker = next_blocker; + } + } + + this->archive_->unlock_nested_archives(); this->archive_->release(); diff --git a/gold/archive.h b/gold/archive.h index 6e20d9c5f14..488563e7624 100644 --- a/gold/archive.h +++ b/gold/archive.h @@ -264,6 +264,8 @@ class Archive : public Library_base no_export() { return this->no_export_; } + const char *libdeps_; + private: Archive(const Archive&); Archive& operator=(const Archive&); @@ -340,6 +342,10 @@ class Archive : public Library_base include_member(Symbol_table*, Layout*, Input_objects*, off_t off, Mapfile*, Symbol*, const char* why); + // Possibly search for and deal with recorded library dependencies. + void + maybe_handle_libdeps(void); + // Return whether we found this archive by searching a directory. bool searched_for() const diff --git a/gold/token.h b/gold/token.h index b58fcc2e1cd..723729c84b8 100644 --- a/gold/token.h +++ b/gold/token.h @@ -97,6 +97,7 @@ class Task_token ~Task_token() { + //fprintf(stderr, "XXX dtor blocker %p\n", this); gold_assert(this->blockers_ == 0); gold_assert(this->writer_ == NULL); } @@ -121,6 +122,7 @@ class Task_token void add_writer(const Task* t) { + //fprintf(stderr, "XXX add writer %p\n", this); gold_assert(!this->is_blocker_ && this->writer_ == NULL); this->writer_ = t; } @@ -158,6 +160,7 @@ class Task_token bool remove_blocker() { + //fprintf(stderr, "XXX remove_blocker %p\n", this); gold_assert(this->is_blocker_ && this->blockers_ > 0); --this->blockers_; this->writer_ = NULL; @@ -168,6 +171,7 @@ class Task_token bool is_blocked() const { + //fprintf(stderr, "XXX is_blocked %p\n", this); gold_assert(this->is_blocker_); return this->blockers_ > 0; } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-05 15:32 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-17 15:03 [RFC] Move static archive dependencies into ld Michael Matz 2023-02-17 20:53 ` Howard Chu 2023-04-13 9:10 ` Clément Chigot 2023-06-05 14:16 ` Howard Chu 2023-06-05 15:32 ` Michael Matz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).