[RFC] Move static archive dependencies into ld

[RFC] Move static archive dependencies into ld

[Michael Matz]

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

Re: [RFC] Move static archive dependencies into ld

[Howard Chu]

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/

Re: [RFC] Move static archive dependencies into ld

[Clément Chigot]

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

Re: [RFC] Move static archive dependencies into ld

[Howard Chu]

[-- 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/

Re: [RFC] Move static archive dependencies into ld

[Michael Matz]

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;
   }