[PATCH] gold: Avoid sharing Plugin_list::iterator

[Alan Modra]

On Thu, Nov 05, 2020 at 03:49:55PM -0800, H.J. Lu wrote:
> On Thu, Nov 5, 2020 at 3:04 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Thu, Nov 05, 2020 at 06:07:55AM -0800, H.J. Lu via Binutils wrote:
> > > class Plugin_manager has
> > >
> > >   // A pointer to the current plugin.  Used while loading plugins.
> > >   Plugin_list::iterator current_;
> > >
> > > The same iterator is shared by all threads. It is OK to use it to load
> > > plugins since only one thread loads plugins.  Avoid sharing Plugin_list
> > > iterator in all other cases.
> >
> > What two pieces of code are simultaneously changing
> > Plugin_manager::current_?  I'm concerned that the underlying thread
> 
> Not about changing.  It is about sharing Plugin_list::iterator.  The current
> gold uses one global iterator for all threads, similar to

HJ, I don't need to be told why sharing variables among threads can go
wrong.  I understand that quite well.  I was asking for the specifics
of what was going wrong.  Now if you haven't analysed the problem to
that level, that's fine, just say so.

You do realise that if two threads are simultaneously running
Plugin_manager::all_symbols_read then your patch hasn't stopped the
underlying lto all_symbols_read from being called twice?

> 
> extern int i;
> 
> for (i = 0; i < n; i++)
>    ...
> 
> in multiple threads.
> 
> > problem still exists, that this patch (which I like BTW) is just
> > changing timing enough to hide the real problem.
> >
> > FWIW, adding
> >
> >   Hold_lock hl(*this->lock_);
> >
> > at the start of Plugin_manager::all_symbols_read also makes the
> > problem go away.
> >
> > >       PR gold/26200
> > >       * plugin.cc (Plugin_manager::claim_file): Don't share Plugin_list
> > >       iterator.
> > >       (Plugin_manager::all_symbols_read): Likewise.
> > >       (Plugin_manager::cleanup): Likewise.
> > > ---
> > >  gold/plugin.cc | 34 +++++++++++++++++-----------------
> > >  1 file changed, 17 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/gold/plugin.cc b/gold/plugin.cc
> > > index 8963c7fa4c..729ddca9f3 100644
> > > --- a/gold/plugin.cc
> > > +++ b/gold/plugin.cc
> > > @@ -755,17 +755,17 @@ Plugin_manager::claim_file(Input_file* input_file, off_t offset,
> > >      this->objects_.push_back(elf_object);
> > >    this->in_claim_file_handler_ = true;
> > >
> > > -  for (this->current_ = this->plugins_.begin();
> > > -       this->current_ != this->plugins_.end();
> > > -       ++this->current_)
> > > +  for (Plugin_list::iterator p = this->plugins_.begin();
> > > +       p != this->plugins_.end();
> > > +       ++p)
> > >      {
> > >        // If we aren't yet in replacement phase, allow plugins to claim input
> > >        // files, otherwise notify the plugin of the new input file, if needed.
> > >        if (!this->in_replacement_phase_)
> > > -        {
> > > -          if ((*this->current_)->claim_file(&this->plugin_input_file_))
> > > -            {
> > > -              this->any_claimed_ = true;
> > > +     {
> > > +       if ((*p)->claim_file(&this->plugin_input_file_))
> > > +         {
> > > +           this->any_claimed_ = true;
> > >                this->in_claim_file_handler_ = false;
> > >
> > >             if (this->recorder_ != NULL)
> > > @@ -775,7 +775,7 @@ Plugin_manager::claim_file(Input_file* input_file, off_t offset,
> > >                                               : elf_object->name());
> > >                 this->recorder_->claimed_file(objname,
> > >                                               offset, filesize,
> > > -                                             (*this->current_)->filename());
> > > +                                             (*p)->filename());
> > >               }
> > >
> > >                if (this->objects_.size() > handle
> > > @@ -790,7 +790,7 @@ Plugin_manager::claim_file(Input_file* input_file, off_t offset,
> > >          }
> > >        else
> > >          {
> > > -          (*this->current_)->new_input(&this->plugin_input_file_);
> > > +       (*p)->new_input(&this->plugin_input_file_);
> > >          }
> > >      }
> > >
> > > @@ -850,10 +850,10 @@ Plugin_manager::all_symbols_read(Workqueue* workqueue, Task* task,
> > >    layout->script_options()->set_defsym_uses_in_real_elf(symtab);
> > >    layout->script_options()->find_defsym_defs(this->defsym_defines_set_);
> > >
> > > -  for (this->current_ = this->plugins_.begin();
> > > -       this->current_ != this->plugins_.end();
> > > -       ++this->current_)
> > > -    (*this->current_)->all_symbols_read();
> > > +  for (Plugin_list::iterator p = this->plugins_.begin();
> > > +       p != this->plugins_.end();
> > > +       ++p)
> > > +    (*p)->all_symbols_read();
> > >
> > >    if (this->any_added_)
> > >      {
> > > @@ -1028,10 +1028,10 @@ Plugin_manager::cleanup()
> > >        close_all_descriptors();
> > >      }
> > >
> > > -  for (this->current_ = this->plugins_.begin();
> > > -       this->current_ != this->plugins_.end();
> > > -       ++this->current_)
> > > -    (*this->current_)->cleanup();
> > > +  for (Plugin_list::iterator p = this->plugins_.begin();
> > > +       p != this->plugins_.end();
> > > +       ++p)
> > > +    (*p)->cleanup();
> > >  }
> > >
> > >  // Make a new Pluginobj object.  This is called when the plugin calls
> > > --
> > > 2.28.0
> >
> > --
> > Alan Modra
> > Australia Development Lab, IBM
> 
> 
> 
> -- 
> H.J.

-- 
Alan Modra
Australia Development Lab, IBM