[PATCH] gold: Avoid sharing Plugin_list::iterator

[Cary Coutant]

> > Thanks for the analysis, which I'll study in more detail. I'm trying
> > to figure out what's going on here. There really shouldn't be two
> > threads both calling all_symbols_read().
>
> There is only one thread calling all_symbols_read().
> all_symbols_read() doesn't hold the lock.  Problem is that another
> thread runs claim_file() for the LTO output object, claim_file() sets
> Plugin_manager::current_ back to plugins_.begin(), affecting the
> all_symbols_read() loop over plugins.

Aha! I think I see the problem now -- let me think about it a little
more and do some more investigation.

As I said before, we should be running single-threaded at the point we
call all_symbols_read(), so there shouldn't be another thread calling
claim_file(). For the LTO output object, which is injected while
all_symbols_read() is running, I'm pretty sure that the problem is
that the Read_symbols task we create for that object is ready to run
immediately, so it starts running before all_symbols_read() has
finished and invokes claim_file() concurrently.

I suspect it dates back to this commit, which apparently I didn't
review carefully enough (sorry!):

commit c4e648430f3c5c135310f87280d45dba581aaa7b
Author: Stephen Crane <sjc@immunant.com>
Date:   Mon Dec 11 14:58:38 2017 -0800

    Add plugin API for processing plugin-added input files

    Gold plugins may wish to further process an input file added by a
plugin. For
    example, the plugin may need to assign a unique segment for sections in a
    plugin-generated input file. This patch adds a plugin callback
that the linker
    will call when reading symbols from a new input file added after the
    all_symbols_read event (i.e. an input file added by a plugin).

    2017-12-11  Stephen Crane  <sjc@immunant.com>

            * plugin-api.h: Add new plugin hook to allow processing of input
            files added by a plugin.
            (ld_plugin_new_input_handler): New function hook type.
            (ld_plugin_register_new_input): New interface.
            (LDPT_REGISTER_NEW_INPUT_HOOK): New enum val.
            (tv_register_new_input): New member.

            * plugin.cc (Plugin::load): Include hooks for register_new_input
            in transfer vector.
            (Plugin::new_input): New function.
            (register_new_input): New function.
            (Plugin_manager::claim_file): Call Plugin::new_input if in
            replacement phase.
            * plugin.h (Plugin::set_new_input_handler): New function.
            * testsuite/plugin_new_section_layout.c: New plugin to test
            new_input plugin API.
            * testsuite/plugin_final_layout.sh: Add new input test.
            * testsuite/Makefile.am (plugin_layout_new_file): New test case.
            * testsuite/Makefile.in: Regenerate.

Before that patch, claim_file() would return immediately when
in_replacement_phase_ was true, not giving any plugin the opportunity
to see the file. Now, it enters the loop and messes with current_.

I also see now that Plugin_manager::current_ is only ever used during
plugin loading, so it is in fact OK not to use it at all during
claim_file, all_symbols_read(), and cleanup(). (In my original design,
I anticipated needing it during claim_file() at least, but it turns
out we don't.)

I withdraw my objection to HJ's patch. OK to apply, with my thanks and
apologies for sidetracking the discussion. But at least now I
understand why the patch works and why it won't break what I thought
it would break.

Thanks again to you and HJ for all the analysis, which helped me see
what I hadn't thought to look at before. Claiming injected input files
weren't part of the original plugin design, and I had blinders on. It
didn't help that my attempts to repro under vmware fusion were
fruitless. I've got a docker container setup now for future
development, and I hope that will make it easier. (I'm also
considering a standalone box for Gnu/Linux development, like the
system76 Meerkat or Gigabyte BRIX -- anyone have anything in a
headless, mini-PC form factor that they can recommend?)

-cary