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

[Han Shen via binutils]

Hi Cary, thanks for your review.

On Sun, Jul 9, 2017 at 12:39 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> 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.

Yes. I couldn't have put it better myself.

>
> 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.

Yes, that's reasonable and better solution. I've reworked the patch accordingly.

>
> 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.

Ah, correct, that's fatal problem. I guess the reason why android
testing didn't catch it is that --fix-erratum-843419 is always turned
on for android build.

>
> +      // 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

Here is the revised (and cleaner) patch:

    gold/ChangeLog:

      2017-07-06  Han Shen  <shenhan@google.com>
        * aarch64.cc (Erratum_stub::invalidate_erratum_stub): New method.
        (Erratum_stub::is_invalidated_erratum_stub): New method.
        (Stub_table::relocate_reloc_stub): Renamed from "relocate_stub".
        (Stub_table::relocate_reloc_stubs): Renamed from "relocate_stubs".
        (Stub_table::relocate_erratum_stub): New method.
        (AArch64_relobj::fix_errata_and_relocate_erratum_stubs): Renamed from
        "fix_errata".
        (Target_aarch64::relocate_reloc_stub): Renamed from "relocate_stub".

Thanks,
-- 
Han Shen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr21491-new.patch
Type: text/x-patch
Size: 14708 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20170710/66ae2a16/attachment.bin>