Bug 23460 - regression: ar can not create archive containing many (>1024) lto object files
Summary: regression: ar can not create archive containing many (>1024) lto object files
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.31
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-27 11:36 UTC by Bartek Szady
Modified: 2018-08-23 15:25 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2018-08-21 00:00:00


Attachments
strace output recorded during invocation of version 2.31 of ar (9.19 MB, application/x-xz)
2018-07-27 11:36 UTC, Bartek Szady
Details
Proposed patch (408 bytes, patch)
2018-08-17 10:01 UTC, Nick Clifton
Details | Diff
Patch implementing Comment 18 (821 bytes, patch)
2018-08-17 17:02 UTC, zenith432
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bartek Szady 2018-07-27 11:36:12 UTC
Created attachment 11155 [details]
strace output recorded during invocation  of version 2.31 of ar

Version 2.31 of ar invoked with 2113 object files ( ar  cqs libQtWebKit.a .obj/release-static/YarrInterpreter.o .obj/release-static/YarrPattern.o ... ) complains about plugin for some of them. Eg.:

ar: .obj/release-static/qrc_WebCore.o: plugin needed to handle lto object

According to strace, ar opens every object file twice but closes once and reaches the limit on the number of open files.

Version 2.30 of ar works.
Comment 1 zenith432 2018-07-28 12:14:47 UTC
This can be reproduced with even a single object file

The first invocation of open is done via _bfd_real_fopen which takes care that the file descriptor is eventually closed.
== BEGIN QUOTE
openat(AT_FDCWD, "a.o", O_RDONLY) = 4
#0  0x00007ffff78fdc50 in open64 () from /lib64/libc.so.6
#1  0x00007ffff788f0f2 in __GI__IO_file_open () from /lib64/libc.so.6
#2  0x00007ffff788f29d in __GI__IO_file_fopen () from /lib64/libc.so.6
#3  0x00007ffff7883549 in __fopen_internal () from /lib64/libc.so.6
#4  0x00007ffff7ed27bf in _bfd_real_fopen () from /lib64/libbfd-2.31.1-4.fc29.so
#5  0x00007ffff7edbd18 in bfd_fopen () from /lib64/libbfd-2.31.1-4.fc29.so
#6  0x000055555555b775 in ar_emul_default_append ()
#7  0x0000555555559ddf in replace_members ()
#8  0x0000555555556f31 in main ()
== END QUOTE

The second invocation of open is done via bfd_plugin_object_p that leaves the file descriptor leaking
== BEGIN QUOTE
openat(AT_FDCWD, "a.o", O_RDONLY) = 7
#0  0x00007ffff78fdc50 in open64 () from /lib64/libc.so.6
#1  0x00007ffff7f6411c in bfd_plugin_open_input () from /lib64/libbfd-2.31.1-4.fc29.so
#2  0x00007ffff7f642a9 in try_load_plugin () from /lib64/libbfd-2.31.1-4.fc29.so
#3  0x00007ffff7f6453f in bfd_plugin_object_p () from /lib64/libbfd-2.31.1-4.fc29.so
#4  0x00007ffff7ed4b2a in bfd_check_format_matches () from /lib64/libbfd-2.31.1-4.fc29.so
#5  0x00007ffff7ecec88 in _bfd_write_archive_contents () from /lib64/libbfd-2.31.1-4.fc29.so
#6  0x00007ffff7edc06e in bfd_close () from /lib64/libbfd-2.31.1-4.fc29.so
#7  0x00005555555593cd in write_archive ()
#8  0x0000555555559f7d in replace_members ()
#9  0x0000555555556f31 in main ()
== END QUOTE
Comment 2 Alan Modra 2018-07-30 00:26:59 UTC
The change between 2.30 and 2.31 is almost certainly due to the fix for pr23254.  What plugin is being used, gcc or llvm, and what version?
Comment 3 zenith432 2018-07-30 10:27:12 UTC
This patch should fix it

===== Begin Patch
diff --git a/bfd/plugin.c b/bfd/plugin.c
index 7c5bba22..98c79c87 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -209,8 +209,7 @@ try_claim (bfd *abfd)
   if (!bfd_plugin_open_input (abfd, &file))
     return 0;
   claim_file (&file, &claimed);
-  if (!claimed)
-    close (file.fd);
+  close (file.fd);
   return claimed;
 }
 
@@ -223,6 +222,9 @@ try_load_plugin (const char *pname, bfd *abfd, int *has_plugin_p)
   ld_plugin_onload onload;
   enum ld_plugin_status status;
 
+  if (claim_file)
+    goto have_claim_file;
+
   *has_plugin_p = 0;
 
   plugin_handle = dlopen (pname, RTLD_NOW);
@@ -257,6 +259,7 @@ try_load_plugin (const char *pname, bfd *abfd, int *has_plugin_p)
   if (status != LDPS_OK)
     goto err;
 
+have_claim_file:
   *has_plugin_p = 1;
 
   abfd->plugin_format = bfd_plugin_no;
===== End Patch

There are two leaks there
- try_load_plugin() (in bfd/plugin.c) calls dlopen() and onload() again and again to load the plugin for each object file processed via bfd_plugin_object_p.  There is no intervening dlclose() and the plugin memory is not cleaned up.  Moreover, add_symbols() keeps the syms array along with the symbol name strings that were allocated by the plugin.  Presumably this memory is released via the plugin's cleanup_handler which is not interfaced and never called.
It is ok I think to keep the plugin loaded permanently, but calling dlopen() over and over increases its refcount and calling onload() over and over is a violation of the plugin API.  onload() should be called once, and claim_file can be called any number of times.  So the patch skips the dlopen() and onload() if claim_file is already registered.

Note however, that calling claim_file many times for 1000s of LTO objects allocates memory each time (for the symbols passed in via add_symbols) and this memory is not released (it is kept used from bfd_plugin_canonicalize_symtab).  So this solution can still potentially run out-of-memory - because the memory for the symtab passed in to add_symbols() is never released.

- The file descriptor opened for claim_file in try_claim() should always be closed after it.  The plugin doesn't use the descriptor anymore.  If the plugin needs a file descriptor again later during the "all symbols read" event, it calls get_input_file() to get another one.  However, neither all_symbols_read_handler nor get_input_file are used by bfd/plugin.c.  See the documentation
http://gcc.gnu.org/wiki/whopr/driver
If the descriptor opened for claim_file is not closed after, it leaks because nobody closes it.  The add_symbols() callback is called from inside claim_file.
Comment 4 zenith432 2018-07-30 15:51:24 UTC
FYI
I took a look at the code for GCC 8.2 plugin and LLVM 6.0.1 gold plugin.

As best as I can tell

- Neither plugins access the file through the descriptor passed to claim_file_hook() after it returns.

  The llvm plugin uses the fd value as an index for determining which claimed files come from the same archive, but does not later access the file through the fd.  Moreover, if the llvm plugin is asked to register its  all_symbols_all_read_hook(), it requires the linker to provide get_input_file() and release_input_file() callbacks - none of which is done by bfd/plugin.c.

  The gcc plugin, for its all_symbols_all_read_hook() passes the input file names and offsets to lto-wrapper and doesn't need any further descriptors.

- Calls to onload() only allocate memory if passing LDPT_OPTIONs to onload() which bfd/plugin.c doesn't do.  Without options, onload() uses only static memory.

- The gcc plugin never releases memory allocated for LDPT_OPTIONs passed to onload().

- The gcc plugin retains all memory allocated in claim_file_hook() (including the memory passed to add_symbols() callback - and frees it in the cleanup_hook().  The cleanup_hook() is not called by bfd/plugin.c today.

- The llvm plugin is written in C++ so all memory it allocates is released by the static destructors called when dlclose() is done on the plugin.  It appears that its cleanup_hook() only deletes temporary files and releases some cached memory - but does not fully release all memory previously allocated - including memory allocated in claim_file_hook().

So to summarize:
- It's better to call dlopen() and onload() only once.  onload() doesn't leak (due to the lack of LDPT_OPTION) - but it still violates the plugin API spec to call onload() many times.

- The file descriptor passed to claim_file_hook() should be closed right after the call or it's never closed.

- The memory allocated in claim_file_hook(), part of which is passed to the all_symbols() callback - can be used by bfd/plugin.c.  However, this memory accumulates more and more with each call to claim_file_hook().  For the gcc plugin, it's enough to call its cleanup_hook() to release the memory and "start over" to claim more input files.  For the llvm plugin this isn't enough - it needs to be dlclose()-ed.
Comment 5 zenith432 2018-07-30 19:14:43 UTC
Correction:

After another review, I noticed that the gcc plugin does NOT free the syms array passed to add_symbols() during cleanup_hook().  This array is freed during the all_symbols_read_hook(), and cleanup_hook() assumes it's already free.

So...  it's hopeless to prevent a memory leak for what bfd/plugin.c is doing

A file descriptor leak can still be avoided because...
- for the gcc plugin - doesn't use fd after claim_file_hook().
- as I said, the llvm plugin uses fd as an index to detect which input files come from the same archive - but this does not mess it up for what bfd/plugin.c is doing.  If fd is closed and its value is reused, llvm plugin incorrectly concludes that all input files with the same fd come from the same archive.  But this only messes it up in all_symbols_read_hook() when using an llvm feature called "thinlto" with a "-thinlto" option passed to the plugin.

So... I believe the patch in Comment 3 is ok.  At least for the 2 plugins the way they are today.

I noticed one more potential leak of fd though... in bfd_plugin_open_input()

=====
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -187,7 +187,10 @@ bfd_plugin_open_input (bfd *ibfd, struct ld_plugin_input_file *file)
     {
       struct stat stat_buf;
       if (fstat (file->fd, &stat_buf))
-       return 0;
+       {
+         close(file->fd);
+         return 0;
+       }
       file->offset = 0;
       file->filesize = stat_buf.st_size;
     }
=====
Comment 6 Bartek Szady 2018-07-31 22:34:57 UTC
Binutils 2.31.1 with patch from Comment 3 works for me.
Comment 7 cvs-commit@gcc.gnu.org 2018-08-01 13:35:54 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=103da91bc083f94769e3758175a96d06cef1f8fe

commit 103da91bc083f94769e3758175a96d06cef1f8fe
Author: Nick Clifton <nickc@redhat.com>
Date:   Wed Aug 1 14:34:41 2018 +0100

    Close resource leaks in the BFD library's plugin handler.
    
    	PR 23460
    	* plugin.c (bfd_plugin_open_input): Close file descriptor if the
    	call to fstat fails.
    	(try_claim): Always close the file descriptor at the end of the
    	function.
    	(try_load_plugin): If a plugin has already been registered, then
    	skip the dlopen and onload steps and go straight to claiming the
    	file.  If these is an error, close the plugin.
Comment 8 Nick Clifton 2018-08-01 13:39:20 UTC
Thanks for the patches.  I have applied them along with an extra fix to try_load_plugin() to close the plugin if it was unable to claim the file.
Comment 9 cvs-commit@gcc.gnu.org 2018-08-02 12:09:54 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=36a900f483789dc2ec4bfd31386e29b52fd2e019

commit 36a900f483789dc2ec4bfd31386e29b52fd2e019
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Thu Aug 2 05:00:45 2018 -0700

    Add a testcase for PR binutils/23460
    
    Add a testcase to limit open files to 16 for AR with plugin.  Before
    
    commit 103da91bc083f94769e3758175a96d06cef1f8fe
    Author: Nick Clifton <nickc@redhat.com>
    Date:   Wed Aug 1 14:34:41 2018 +0100
    
        Close resource leaks in the BFD library's plugin handler.
    
    it failed with:
    
    ../binutils/ar: tmpdir/pr23460f.o: plugin needed to handle lto object
    
    	PR binutils/23460
    	* testsuite/ld-plugin/lto.exp: Run the PR binutils/23460 test.
    	* testsuite/ld-plugin/pr23460a.c: New file.
    	* testsuite/ld-plugin/pr23460b.c: Likewise.
    	* testsuite/ld-plugin/pr23460c.c: Likewise.
    	* testsuite/ld-plugin/pr23460d.c: Likewise.
    	* testsuite/ld-plugin/pr23460e.c: Likewise.
    	* testsuite/ld-plugin/pr23460f.c: Likewise.
Comment 10 Evangelos Foutras 2018-08-07 22:55:48 UTC
Is it possible to avoid calling dlclose() on the plugin handle? I'm asking because the LLVM gold plugin (LLVMgold.so) will throw the following error when loaded more than once:

  > CommandLine Error: Option 'asm-instrumentation' registered more than once!

libLLVM.so used to have the same problem, but it's worked around using NODELETE (https://reviews.llvm.org/D40459). However, LLVMgold.so is considered a module and not a shared library. As such, it's not linked with "-z nodelete".

Perhaps an fix on LLVM's side would be to specify nodelete for LLVMgold.so; I have uploaded a tentative patch doing that here: https://reviews.llvm.org/D50416

In any case, I thought I'd also ask here if it might make sense to remove dlclose() from bfd's plugin loader.
Comment 11 Nick Clifton 2018-08-14 08:55:26 UTC
(In reply to Evangelos Foutras from comment #10)

Hi Evangelos,

> Is it possible to avoid calling dlclose() on the plugin handle? 

I have to say that I am not really liking this idea...

> I'm asking
> because the LLVM gold plugin (LLVMgold.so) will throw the following error
> when loaded more than once:
> 
>   > CommandLine Error: Option 'asm-instrumentation' registered more than
> once!

Which seems correct to me.  Why would you want to load a plugin more than 
once ?

The most likely reason it seems to me is if the build system is quite
complicated and there are multiple places where the plugin command line
option could be enabled.  If this is the case, wouldn't it make more
sense for the tool (llvm, gcc, ld, etc) to accumulate plugin options and
eliminate duplicates ?

> In any case, I thought I'd also ask here if it might make sense to remove
> dlclose() from bfd's plugin loader.

I just do not like the idea of not tidying up after ourselves.  Mind you it is only linker plugins that call dlclose.  Plugins for other tools (eg ar, nm, etc) do not bother with the call.  Plus the linker only calls dlclose when it is closing *all* plugins as part of its exit sequence.  So I am not sure if there really is a problem (for the binutils) here.

Cheers
  Nick
Comment 12 Michał Górny 2018-08-14 09:15:32 UTC
Nick, can you think of any issues with building the plugin with -Wl,-z,nodelete? I'm wondering binutils plugin API in particular.
Comment 13 Nick Clifton 2018-08-14 09:44:19 UTC
(In reply to Michał Górny from comment #12)
> Nick, can you think of any issues with building the plugin with
> -Wl,-z,nodelete? I'm wondering binutils plugin API in particular.

There are none that I can think of, although that does not mean that
there aren't any.  You could always give it a go and run the binutils
testsuite to see if anything changes.
Comment 14 Evangelos Foutras 2018-08-14 15:52:25 UTC
(In reply to Nick Clifton from comment #11)
> I just do not like the idea of not tidying up after ourselves.  Mind you it
> is only linker plugins that call dlclose.  Plugins for other tools (eg ar,
> nm, etc) do not bother with the call.  Plus the linker only calls dlclose
> when it is closing *all* plugins as part of its exit sequence.  So I am not
> sure if there really is a problem (for the binutils) here.

(To reproduce the following on Fedora Rawhide, rebuild binutils with the patch from comment #7 and pass "--plugin /usr/lib64/LLVMgold.so" to ar/nm. The examples below are without the --plugin option, because LLVMgold.so is symlinked from /usr/lib/bfd-plugins on Arch.)

The main issue seems to be with ar and nm:

  $ touch a.c b.c
  $ gcc -c a.c b.c
  $ ar r ab.a a.o b.o 
  ar: creating ab.a
  : CommandLine Error: Option 'asm-instrumentation' registered more than once!
  LLVM ERROR: inconsistency in registered CommandLine options

  $ nm a.o b.o
  a.o:
  : CommandLine Error: Option 'asm-instrumentation' registered more than once!
  LLVM ERROR: inconsistency in registered CommandLine options

I believe the above error is caused by a sequence of "dlopen -> dlclose -> dlopen" on LLVMgold.so. (liblto_plugin.so doesn't seem to mind being unloaded and loaded again.)

Is the repeated dlopen expected here?
Comment 15 zenith432 2018-08-16 10:01:10 UTC
(In reply to Evangelos Foutras from comment #14)
> I believe the above error is caused by a sequence of "dlopen -> dlclose ->
> dlopen" on LLVMgold.so. (liblto_plugin.so doesn't seem to mind being
> unloaded and loaded again.)
> 
> Is the repeated dlopen expected here?

Yes, because bfd/plugin.c unloads the plugin on any input file that is not claimed by the plugin.

I propose the following change

===== begin patch
diff --git a/bfd/plugin.c b/bfd/plugin.c
index d9b9e2f1..3b738c38 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -274,7 +274,7 @@ have_claim_file:
     goto err;
 
   if (!try_claim (abfd))
-    goto err;
+    return 0;
 
   abfd->plugin_format = bfd_plugin_yes;

===== end patch

On a different subject, since you're submitting commits to llvm, could you please fix the design flaw in llvm plugin described in Comment #5?  It uses the file descriptor fd passed to claim_file_hook() as a uniqueness-detector to determine which input files are found in the same archive.  This is a no-no because the plugin API disallows using the fd passed to claim_file_hook() after it returns.  It is essential that the plugin client be free to reuse file descriptors for input files because there may be more input files than open descriptors allowed by the kernel.

The plugin should not really have to detect whether input files come from the same archive, but if it must should use some other method such as...
1) Use the filename instead of descriptor as a unique key.
2) do fstat() on the file descriptor and use a <device_id,inode> pair as a unique key.

Thanks.
Comment 16 Evangelos Foutras 2018-08-17 00:16:20 UTC
(In reply to zenith432 from comment #15)
> ===== begin patch
> diff --git a/bfd/plugin.c b/bfd/plugin.c
> index d9b9e2f1..3b738c38 100644
> --- a/bfd/plugin.c
> +++ b/bfd/plugin.c
> @@ -274,7 +274,7 @@ have_claim_file:
>      goto err;
>  
>    if (!try_claim (abfd))
> -    goto err;
> +    return 0;
>  
>    abfd->plugin_format = bfd_plugin_yes;
> 
> ===== end patch

The above didn't allow a ThinLTO Chromium build to finish; there were missing symbols during the final linking of the main binary.

Chromium built successfully with the following change (and unpatched LLVM):

==============================
diff --git a/bfd/plugin.c b/bfd/plugin.c
index d9b9e2f174..e12f37d36a 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -274,7 +274,7 @@ have_claim_file:
     goto err;
 
   if (!try_claim (abfd))
-    goto err;
+    goto err_no_dlclose;
 
   abfd->plugin_format = bfd_plugin_yes;
 
@@ -283,6 +283,7 @@ have_claim_file:
  err:
   if (plugin_handle)
     dlclose (plugin_handle);
+ err_no_dlclose:
   register_claim_file (NULL);
   return 0;
 }
==============================

The proposed approach seems to skip dlclose for most (all?) plugins. To my untrained eye, it looks preferable to just do away with dlclose() altogether instead of selectively skipping it.

The workaround on the LLVM side that builds LLVMgold.so with -Wl,-z,nodelete has been tentatively merged. [1] Assuming it doesn't cause any regressions, the use of dlclose() shouldn't be an issue anymore.

Finally, I will say that I don't understand the need to dlclose() plugins if they don't claim a specific file. Doesn't this create unnecessary overhead of opening and closing plugins for each member added to an archive (in the case of ar)? Since previous binutils versions worked fine, I would think that the problem was with file handles to object files being left open, and not plugins that remained loaded.

[1] https://reviews.llvm.org/D50416
Comment 17 Nick Clifton 2018-08-17 10:01:53 UTC
Created attachment 11192 [details]
Proposed patch

Hi Guys,

  I agree that the current proposed patch is essentially the same as not
  calling dlopen at all.  So would one of you like to try out the uploaded
  patch and let me know that it works ?  (Or does not work...)  It basically
  just removes the dlclose() and replaces it with a big comment, explaining
  what is going on.

Cheers
  Nick
Comment 18 Alan Modra 2018-08-17 12:09:10 UTC
If we aren't closing plugins, I think we should probably put open plugins on a list so they aren't reopened.  glibc may well cope with dlopening the same module  possibly many thousands of times, but that might not be true of other libcs.
Comment 19 Evangelos Foutras 2018-08-17 15:25:02 UTC
I believe it's safe to remove the call to dlclose() which, as far as I can tell, would restore the previous behavior of the plugin loader. I will do a Chromium build to verify that the patch from comment #17 works with unpatched LLVM. (I'm 99% sure it won't fail though.)

(Bellow excerpts are for reference.)

dlopen(3) / glibc says:

  If the same shared object is loaded again with dlopen(), the same
  object handle is returned. The dynamic linker maintains reference
  counts for object handles, so a dynamically loaded shared object
  is not deallocated until dlclose() has been called on it as many
  times as dlopen() has succeeded on it.

dlopen(3p) / POSIX also touches on this:

  Only a single copy of an executable object file shall be brought
  into the address space, even if dlopen() is invoked multiple times
  in reference to the executable object file, and even if different
  pathnames are used to reference the executable object file.
Comment 20 zenith432 2018-08-17 16:02:26 UTC
It's not just repetitive calls to dlopen() on the same plugin.

It is also repetitive calls to onload() on the same plugin.  This isn't supported by the plugin API.  Today this works on gcc & llvm plugins because they don't allocate memory in onload() unless LDPT_OPTIONs are passed.

If try_load_plugin() in bfd/plugin.c can be called with different plugins during the same run the right solution is what Alan Modra says in comment 18.
Comment 21 zenith432 2018-08-17 17:02:14 UTC
Created attachment 11193 [details]
Patch implementing Comment 18

Attached is a patch that implements algorithm suggested in comment 18.
Should solve outstanding problems with try_load_plugin() in bfd/plugin.c.
Each plugin is loaded once, not unloaded, refcount not recursed.
onload() called only once for each plugin.
Supports multiple plugins in same run.
Please review.
Comment 22 Nick Clifton 2018-08-21 12:14:18 UTC
(In reply to zenith432 from comment #21)

Hi Zenith,

> Attached is a patch that implements algorithm suggested in comment 18.

I am confused by this part of the patch:

  for (plugin_list_iter = plugin_list;
       plugin_list_iter != NULL;
       plugin_list_iter = plugin_list_iter->next)
    {
      if (plugin_handle == plugin_list_iter->handle)
        {
          dlclose (plugin_handle);
          if (!plugin_list_iter->claim_file)
            return 0;
          register_claim_file (plugin_list_iter->claim_file);
          goto have_claim_file;
        }
    }

If I am reading this correctly, this says that if we are attempting to 
load a plugin that was previously loaded, then we call dlclose() on it,
and then, if the plugin has a file claim function, we register this 
function as the global file claimant, despite the plugin having been closed...
Is that right ?

Also  - it seems to me that if we have a list of loaded plugins, then
we no longer need the claim_file global variable, and the try_claim() 
function should walk the list of plugins, trying each in turn.

Cheers
  Nick
Comment 23 zenith432 2018-08-21 14:24:25 UTC
(In reply to Nick Clifton from comment #22)
> I am confused by this part of the patch:
> 
>   for (plugin_list_iter = plugin_list;
>        plugin_list_iter != NULL;
>        plugin_list_iter = plugin_list_iter->next)
>     {
>       if (plugin_handle == plugin_list_iter->handle)
>         {
>           dlclose (plugin_handle);
>           if (!plugin_list_iter->claim_file)
>             return 0;
>           register_claim_file (plugin_list_iter->claim_file);
>           goto have_claim_file;
>         }
>     }
> 
> If I am reading this correctly, this says that if we are attempting to 
> load a plugin that was previously loaded, then we call dlclose() on it,
> and then, if the plugin has a file claim function, we register this 
> function as the global file claimant, despite the plugin having been
> closed...
> Is that right ?

Not quite.  The first time a plugin is loaded, it's not in the linked list, so the for loop you quoted gets skipped.  The plugin's dlopen() refcount is set to 1, the code later on creates a linked-list item for it and initializes it with the handle, it's claim_file and links the item.
The 2nd or subsequent times a plugin is loaded, the dlopen() bumps its refcount to 2, the plugin's handle is found in the linked list, calling dlclose() reduces the refcount back to 1 (to keep it from growing unbounded).  The plugin is still loaded (with refcount 1), and the cached claim_file pointer which was found during the initial load is installed.

> Also  - it seems to me that if we have a list of loaded plugins, then
> we no longer need the claim_file global variable, and the try_claim() 
> function should walk the list of plugins, trying each in turn.

I agree, but I was trying to keep the logic localized to try_claim_plugin() which is called with a specific plugin name and input file.  Your suggestion requires refactoring the code at a higher level.  It is entered via bfd_plugin_object_p() which is referenced from plugin_vec - and is called for a specific input file.  Then it goes to load_plugin() which iterates over plugins by name for a given input file.  load_plugin() in turn calls try_claim_plugin() with a specific plugin name and input file.
Comment 24 cvs-commit@gcc.gnu.org 2018-08-23 15:24:00 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b0ceb98aec8e1ab610deea9fee9ee75c5911bbc0

commit b0ceb98aec8e1ab610deea9fee9ee75c5911bbc0
Author: Zenith423 <zenith432@users.sourceforge.net>
Date:   Thu Aug 23 16:22:56 2018 +0100

    Avoid problems with plugins being loaded multiple times.
    
    	PR 23460
    	* plugin.c (struct plugin_list_entry): New structure.
    	(plugin_list): New variable.
    	(try_load_plugin): Place opened plugins on a list.  Ensure that
    	the refcount in the dynamic loader is kept at 1.
Comment 25 Nick Clifton 2018-08-23 15:25:08 UTC
Hi Zenith,

  Thanks for the explanation.  Now that I understand the code, I agree
  that is should work, so I have applied your patch.

Cheers
  Nick