This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
commit 2ac1a0a15d54d7a1daae16ebf390da95744b3711
Author: Han Shen <shenhan@google.com>
Date:   Mon Jul 10 15:23:05 2017 -0700

    Fixing for gold pr/21491 - Errata workaround can produce broken images.
    
    The problem is caused by the fact that gold is relocating the stubs
    for an entire output section when it processes 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, it
    ends up with a copy of the unrelocated original instruction.
    
    In other words - when calling fix_errata() from
    do_relocate_sections(), gold is 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 gold is going through the object file's
    input sections, it then checks for stub tables "owned" by each input
    section, and writes out all the stubs from that stub table, regardless
    of what object file each stub is associated with.
    
    Fixed by relocating the erratum stub only after the corresponding
    errata spot is fixed. That is to have fix_errata() call
    Stub_table::relocate_erratum_stub() for each stub.
    
    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".

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index 11bb48e3b0..3953e4a4f8 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1049,6 +1049,17 @@ public:
     return this->sh_offset_ < k.sh_offset_;
   }
 
+  void
+  invalidate_erratum_stub()
+  {
+     gold_assert(this->relobj_ != NULL);
+     this->relobj_ = NULL;
+  }
+
+  bool
+  is_invalidated_erratum_stub()
+  { return this->relobj_ == NULL; }
+
 protected:
   virtual void
   do_write(unsigned char*, section_size_type);
@@ -1346,7 +1357,8 @@ Reloc_stub<size, big_endian>::stub_type_for_reloc(
   return ST_LONG_BRANCH_ABS;
 }
 
-// A class to hold stubs for the ARM target.
+// A class to hold stubs for the ARM target. This contains 2 different types of
+// stubs - reloc stubs and erratum stubs.
 
 template<int size, bool big_endian>
 class Stub_table : public Output_data
@@ -1438,14 +1450,18 @@ class Stub_table : public Output_data
     return (p != this->reloc_stubs_.end()) ? p->second : NULL;
   }
 
-  // Relocate stubs in this stub table.
+  // Relocate reloc stubs in this stub table. This does not relocate erratum stubs.
   void
-  relocate_stubs(const The_relocate_info*,
-		 The_target_aarch64*,
-		 Output_section*,
-		 unsigned char*,
-		 AArch64_address,
-		 section_size_type);
+  relocate_reloc_stubs(const The_relocate_info*,
+                       The_target_aarch64*,
+                       Output_section*,
+                       unsigned char*,
+                       AArch64_address,
+                       section_size_type);
+
+  // Relocate an erratum stub.
+  void
+  relocate_erratum_stub(The_erratum_stub*, unsigned char*);
 
   // Update data size at the end of a relaxation pass.  Return true if data size
   // is different from that of the previous relaxation pass.
@@ -1485,15 +1501,15 @@ class Stub_table : public Output_data
   { this->set_data_size(this->current_data_size()); }
 
  private:
-  // Relocate one stub.
+  // Relocate one reloc stub.
   void
-  relocate_stub(The_reloc_stub*,
-		const The_relocate_info*,
-		The_target_aarch64*,
-		Output_section*,
-		unsigned char*,
-		AArch64_address,
-		section_size_type);
+  relocate_reloc_stub(The_reloc_stub*,
+                      const The_relocate_info*,
+                      The_target_aarch64*,
+                      Output_section*,
+                      unsigned char*,
+                      AArch64_address,
+                      section_size_type);
 
  private:
   // Owner of this stub table.
@@ -1593,76 +1609,85 @@ Stub_table<size, big_endian>::add_reloc_stub(
 }
 
 
-// Relocate all stubs in this stub table.
+// Relocate an erratum stub.
+
+template<int size, bool big_endian>
+void
+Stub_table<size, big_endian>::
+relocate_erratum_stub(The_erratum_stub* estub,
+                      unsigned char* view)
+{
+  // Just for convenience.
+  const int BPI = AArch64_insn_utilities<big_endian>::BYTES_PER_INSN;
+
+  gold_assert(!estub->is_invalidated_erratum_stub());
+  AArch64_address stub_address = this->erratum_stub_address(estub);
+  // The address of "b" in the stub that is to be "relocated".
+  AArch64_address stub_b_insn_address;
+  // Branch offset that is to be filled in "b" insn.
+  int b_offset = 0;
+  switch (estub->type())
+  {
+    case ST_E_843419:
+    case ST_E_835769:
+      // The 1st insn of the erratum could be a relocation spot,
+      // in this case we need to fix it with
+      // "(*i)->erratum_insn()".
+      elfcpp::Swap<32, big_endian>::writeval(
+          view + (stub_address - this->address()),
+          estub->erratum_insn());
+      // For the erratum, the 2nd insn is a b-insn to be patched
+      // (relocated).
+      stub_b_insn_address = stub_address + 1 * BPI;
+      b_offset = estub->destination_address() - stub_b_insn_address;
+      AArch64_relocate_functions<size, big_endian>::construct_b(
+          view + (stub_b_insn_address - this->address()),
+          ((unsigned int)(b_offset)) & 0xfffffff);
+      break;
+    default:
+      gold_unreachable();
+      break;
+  }
+  estub->invalidate_erratum_stub();
+}
+
+
+// Relocate only reloc stubs in this stub table. This does not relocate erratum
+// stubs.
 
 template<int size, bool big_endian>
 void
 Stub_table<size, big_endian>::
-relocate_stubs(const The_relocate_info* relinfo,
-	       The_target_aarch64* target_aarch64,
-	       Output_section* output_section,
-	       unsigned char* view,
-	       AArch64_address address,
-	       section_size_type view_size)
+relocate_reloc_stubs(const The_relocate_info* relinfo,
+                     The_target_aarch64* target_aarch64,
+                     Output_section* output_section,
+                     unsigned char* view,
+                     AArch64_address address,
+                     section_size_type view_size)
 {
   // "view_size" is the total size of the stub_table.
   gold_assert(address == this->address() &&
 	      view_size == static_cast<section_size_type>(this->data_size()));
   for(Reloc_stub_map_const_iter p = this->reloc_stubs_.begin();
       p != this->reloc_stubs_.end(); ++p)
-    relocate_stub(p->second, relinfo, target_aarch64, output_section,
-		  view, address, view_size);
-
-  // Just for convenience.
-  const int BPI = AArch64_insn_utilities<big_endian>::BYTES_PER_INSN;
-
-  // Now 'relocate' erratum stubs.
-  for(Erratum_stub_set_iter i = this->erratum_stubs_.begin();
-      i != this->erratum_stubs_.end(); ++i)
-    {
-      AArch64_address stub_address = this->erratum_stub_address(*i);
-      // The address of "b" in the stub that is to be "relocated".
-      AArch64_address stub_b_insn_address;
-      // Branch offset that is to be filled in "b" insn.
-      int b_offset = 0;
-      switch ((*i)->type())
-	{
-	case ST_E_843419:
-	case ST_E_835769:
-	  // The 1st insn of the erratum could be a relocation spot,
-	  // in this case we need to fix it with
-	  // "(*i)->erratum_insn()".
-	  elfcpp::Swap<32, big_endian>::writeval(
-	      view + (stub_address - this->address()),
-	      (*i)->erratum_insn());
-	  // For the erratum, the 2nd insn is a b-insn to be patched
-	  // (relocated).
-	  stub_b_insn_address = stub_address + 1 * BPI;
-	  b_offset = (*i)->destination_address() - stub_b_insn_address;
-	  AArch64_relocate_functions<size, big_endian>::construct_b(
-	      view + (stub_b_insn_address - this->address()),
-	      ((unsigned int)(b_offset)) & 0xfffffff);
-	  break;
-	default:
-	  gold_unreachable();
-	  break;
-	}
-    }
+    relocate_reloc_stub(p->second, relinfo, target_aarch64, output_section,
+                        view, address, view_size);
 }
 
 
-// Relocate one stub.  This is a helper for Stub_table::relocate_stubs().
+// Relocate one reloc stub. This is a helper for
+// Stub_table::relocate_reloc_stubs().
 
 template<int size, bool big_endian>
 void
 Stub_table<size, big_endian>::
-relocate_stub(The_reloc_stub* stub,
-	      const The_relocate_info* relinfo,
-	      The_target_aarch64* target_aarch64,
-	      Output_section* output_section,
-	      unsigned char* view,
-	      AArch64_address address,
-	      section_size_type view_size)
+relocate_reloc_stub(The_reloc_stub* stub,
+                    const The_relocate_info* relinfo,
+                    The_target_aarch64* target_aarch64,
+                    Output_section* output_section,
+                    unsigned char* view,
+                    AArch64_address address,
+                    section_size_type view_size)
 {
   // "offset" is the offset from the beginning of the stub_table.
   section_size_type offset = stub->offset();
@@ -1670,8 +1695,8 @@ relocate_stub(The_reloc_stub* stub,
   // "view_size" is the total size of the stub_table.
   gold_assert(offset + stub_size <= view_size);
 
-  target_aarch64->relocate_stub(stub, relinfo, output_section,
-				view + offset, address + offset, view_size);
+  target_aarch64->relocate_reloc_stub(stub, relinfo, output_section,
+                                      view + offset, address + offset, view_size);
 }
 
 
@@ -1829,9 +1854,11 @@ class AArch64_relobj : public Sized_relobj_file<size, big_endian>
 			 Stringpool_template<char>*);
 
  private:
-  // Fix all errata in the object.
+  // Fix all errata in the object, and for each erratum, relocate corresponding
+  // erratum stub.
   void
-  fix_errata(typename Sized_relobj_file<size, big_endian>::Views* pviews);
+  fix_errata_and_relocate_erratum_stubs(
+      typename Sized_relobj_file<size, big_endian>::Views* pviews);
 
   // Try to fix erratum 843419 in an optimized way. Return true if patch is
   // applied.
@@ -1943,11 +1970,12 @@ AArch64_relobj<size, big_endian>::do_count_local_symbols(
 }
 
 
-// Fix all errata in the object.
+// Fix all errata in the object and for each erratum, we relocate the
+// corresponding erratum stub (by calling Stub_table::relocate_erratum_stub).
 
 template<int size, bool big_endian>
 void
-AArch64_relobj<size, big_endian>::fix_errata(
+AArch64_relobj<size, big_endian>::fix_errata_and_relocate_erratum_stubs(
     typename Sized_relobj_file<size, big_endian>::Views* pviews)
 {
   typedef typename elfcpp::Swap<32,big_endian>::Valtype Insntype;
@@ -1988,6 +2016,17 @@ AArch64_relobj<size, big_endian>::fix_errata(
 	      AArch64_relocate_functions<size, big_endian>::construct_b(
 		pview.view + stub->sh_offset(), b_offset & 0xfffffff);
 	    }
+
+          // Erratum fix is done (or skipped), continue to relocate erratum
+          // stub. Note, when erratumf ix is skipped (either because we
+          // proactively change the code sequence or the code sequence is
+          // changed by relaxation, etc), we can still safely relocate the
+          // erratum stub, ignoring the fact the erratum could never be
+          // executed.
+          stub_table->relocate_erratum_stub(
+              stub, pview.view + (stub_table->address() - pview.address));
+
+          // Next erratum stub.
 	  ++p;
 	}
     }
@@ -2086,16 +2125,19 @@ AArch64_relobj<size, big_endian>::do_relocate_sections(
   if (parameters->options().relocatable())
     return;
 
+  // This part only relocates erratum stubs that belong to input sections of this
+  // object file.
   if (parameters->options().fix_cortex_a53_843419()
       || parameters->options().fix_cortex_a53_835769())
-    this->fix_errata(pviews);
+    this->fix_errata_and_relocate_erratum_stubs(pviews);
 
   Relocate_info<size, big_endian> relinfo;
   relinfo.symtab = symtab;
   relinfo.layout = layout;
   relinfo.object = this;
 
-  // Relocate stub tables.
+  // This part relocates all reloc stubs that are contained in stub_tables of
+  // this object file.
   unsigned int shnum = this->shnum();
   The_target_aarch64* target = The_target_aarch64::current_target();
 
@@ -2124,8 +2166,8 @@ AArch64_relobj<size, big_endian>::do_relocate_sections(
 	  unsigned char* view = view_struct.view + offset;
 	  AArch64_address address = stub_table->address();
 	  section_size_type view_size = stub_table->data_size();
-	  stub_table->relocate_stubs(&relinfo, target, os, view, address,
-				     view_size);
+	  stub_table->relocate_reloc_stubs(&relinfo, target, os, view, address,
+					   view_size);
 	}
     }
 }
@@ -3016,11 +3058,11 @@ class Target_aarch64 : public Sized_target<size, big_endian>
       Address view_address,
       section_size_type);
 
-  // Relocate a single stub.
+  // Relocate a single reloc stub.
   void
-  relocate_stub(The_reloc_stub*, const Relocate_info<size, big_endian>*,
-		Output_section*, unsigned char*, Address,
-		section_size_type);
+  relocate_reloc_stub(The_reloc_stub*, const Relocate_info<size, big_endian>*,
+                      Output_section*, unsigned char*, Address,
+                      section_size_type);
 
   // Get the default AArch64 target.
   static This*
@@ -4035,16 +4077,16 @@ Target_aarch64<size, big_endian>::scan_section_for_stubs(
 }
 
 
-// Relocate a single stub.
+// Relocate a single reloc stub.
 
 template<int size, bool big_endian>
 void Target_aarch64<size, big_endian>::
-relocate_stub(The_reloc_stub* stub,
-	      const The_relocate_info*,
-	      Output_section*,
-	      unsigned char* view,
-	      Address address,
-	      section_size_type)
+relocate_reloc_stub(The_reloc_stub* stub,
+                    const The_relocate_info*,
+                    Output_section*,
+                    unsigned char* view,
+                    Address address,
+                    section_size_type)
 {
   typedef AArch64_relocate_functions<size, big_endian> The_reloc_functions;
   typedef typename The_reloc_functions::Status The_reloc_functions_status;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]