[PATCH] gold: Avoid sharing Plugin_list::iterator

[H.J. Lu]

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

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.