Re: [gold][aarch64]: Fixing gold pr/21491 - Errata workaround can produce broken images.

[Cary Coutant <ccoutant at gmail dot com>]

> Attached is the revised patch. ChangeLog is also updated. Sorry for
> the confusion.
>
> gold/ChangeLog:
>
>         2017-07-06  Han Shen  <shenhan@google.com>
>
>         * aarch64.cc (do_relocate_stub_tables): New method (implementation).
>         (do_relocate_sections): Removed relocate stub code.
>         * gold.cc (queue_final_tasks): Queue Relocstub_tasks.
>         * object.h (Relobj::relocate_stub_tables): New method.
>         (Relobj::clear_views): Pure virtual method.
>         (Relobj::do_relocate_stub_tables): Pure virtual method.
>         (Sized_relobj::do_relocate_stub_tables): New method declaration.
>         (Sized_relobj::clear_views): New method declaration.
>         (Sized_relobj_file::create_views): New method.
>         (Sized_relobj_file::get_views): New method.
>         (Sized_relobj_file::clear_views): New method declaration.
>         * object.cc (Sized_relobj::clear_views): New method (empty).
>         (Sized_relobj::do_relocate_stub_tables): New method (empty).
>         (Sized_relobj_file::clear_views): New method (implementation).
>         * reloc.h (Relocstub_task): New task class.
>         (Relocate_task::symbol_table): New method.
>         (Relocate_task::layout): New method.
>         (Relocate_task::object): New method.
>         (Relocate_task::defer_object_cleanup_): New member.
>         * reloc.cc (Relocstub_task): New task class definition.
>         (Sized_relobj_file::do_relocate): Removed relocate_stub code.

It seems to me that the problem is caused by the fact that you're
relocating the stubs for an entire output section when you process the
relocations for a particular input section that happened to be
designated as the stub table "owner". The Relocate_task for that input
section may or may not run before the Relocate_task for another input
section that contains the code that needs the erratum fix, but doesn't
"own" the stub table. If it runs before (or might even race with) that
other task, you end up with a copy of the unrelocated original
instruction.

When you call fix_errata() from
AArch64_relobj::do_relocate_sections(), you're going through the list
of errata stubs that are associated only with that object. This
routine updates the stored original instruction and replaces it in the
output view with a branch to the stub. Later, as you're going through
the object file's input sections, you then check for stub tables
"owned" by each input section, and write out all the stubs from that
stub table, regardless of what object file each stub is associated
with.

Wouldn't it be a simpler fix to have fix_errata() call
Stub_table::relocate_stub() for each stub, so you write the stubs per
object rather than per stub table? This would require no new tasks,
and would guarantee that the stub receives the relocated copy of the
original instruction.

If I'm missing something here, and you really do need to delay
relocate_stubs() to a later task, I have a few more comments on the
patch...

+  bool need_relocstub_tasks = !parameters->options().relocatable() &&
+      (parameters->options().fix_cortex_a53_843419()
+       || parameters->options().fix_cortex_a53_835769());

The check for whether or not you need to create these additional tasks
should be in target-dependent code. I'd suggest adding a new method
Target::needs_relocstub_task(). Also, shouldn't the answer depend on
whether or not you actually have any errata stubs?

Furthermore, if I'm reading this correctly, you're not going to run
any Relocate_stub_tasks if neither of the "fix" options is set. That
means that you won't ever call Target_aarch64::relocate_stub() for
regular relocation stubs.

+      // Blockers for n Relocstub_tasks.
+      if (need_relocstub_tasks)
+        input_sections_blocker->add_blockers(input_objects->number_of_relobjs());

I'd think that the number of errata is sufficiently small that trying
to parallelize these into one task per object would be
counter-productive. I'd suggest adding just one additional task that
runs after all Relocate_tasks, and before
Write_after_input_sections_task or Build_id_task_runner (depending on
any_postprocessing_sections).

-  // This is normally the last thing we will do with an object, so
-  // uncache all views.
-  this->object_->clear_view_cache_marks();
-
-  this->object_->release();
+  // When defer_object_cleanup_ is true, we do cleanup work at the end of
+  // Relocstub_task.
+  if (!this->defer_object_cleanup_) {
+    // This is normally the last thing we will do with an object, so
+    // uncache all views.
+    this->object_->clear_views();
+    this->object_->clear_view_cache_marks();
+    this->object_->release();

It's not clear to me why you need to save the views for the stub
relocation. Since fix_errata() has already patched the offending
instructions from the input files, the stub tables themselves are
always dealing with a view directly in the output file. I think you
should still be able to clear all the views at the end of
Relocate_task. I think you should be able to easily re-create the
output view in the Relocstub_task.

-  Task_token*
+  virtual Task_token*
   is_runnable();

-  void
+  virtual void
   locks(Task_locker*);

-  void
+  virtual void
   run(Workqueue*);

-  std::string
+  virtual std::string
   get_name() const;

Why these changes? The methods are declared virtual in workqueue.h,
and it should not need repeating.

-cary