This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [gold][aarch64]: Fixing gold pr/21491 - Errata workaround can produce broken images.
- From: "Han Shen via binutils" <binutils at sourceware dot org>
- To: Cary Coutant <ccoutant at gmail dot com>, binutils <binutils at sourceware dot org>, Peter Smith <peter dot smith at linaro dot org>
- Cc: Eric Christopher <echristo at gmail dot com>, Jing Yu <jingyu at google dot com>, Luis Lozano <llozano at google dot com>, Caroline Tice <cmtice at google dot com>, Rahul Chaudhry <rahulchaudhry at google dot com>
- Date: Fri, 7 Jul 2017 10:48:30 -0700
- Subject: Re: [gold][aarch64]: Fixing gold pr/21491 - Errata workaround can produce broken images.
- Authentication-results: sourceware.org; auth=none
- References: <CACkGtrgGO2ToZ1T-9kr7Uu2XnqVp8qhgH_N4=chaJOi__fZO0w@mail.gmail.com>
- Reply-to: Han Shen <shenhan at google dot com>
Hi Cary, android bot reported a compilation failure on Mac Darwin
systems due to duplicated instantiated template methods. This was
fixed.
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.
Han
On Thu, Jul 6, 2017 at 11:03 AM, Han Shen <shenhan@google.com> wrote:
> Hi Cary, this patch fixes gold pr/21491 - Errata workaround can
> produce broken images.
>
> The root cause of the bug is that in the origin implementation,
> relocate_sections and relocate_stubs phases are interleaved, in some
> cases, relocate_stubs copy instructions before they are relocated in
> relocate_sections phase, resulting stale and invalid insns in the
> final stub table.
>
> This patch fixes the problem by adding a new task "Relocstub_task"
> that runs after "Relocate_task", so relocate_stubs always pick the
> relocated insn.
>
> Tested: test case in bug passed and from Android toolchain team -
> "built all platform source tree for all Android architectures; pushed
> Angler build onto device & tested device."
>
> gold/ChangeLog:
>
> 2017-07-06 Han Shen <shenhan@google.com>
>
> * aarch64.cc (do_relocate_stub_tables): New method.
> (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): New method.
> (Relobj::do_relocate_stub_tables): New method.
> (Sized_relobj_file::do_relocate_stub_tables): New method.
> (Sized_relobj_file::create_views): New method.
> (Sized_relobj_file::get_views): New method.
> (Sized_relobj_file::clear_views): New method.
> * reloc.h (Relocstub_task): New task class.
> (Relocate_task::symbol_table): New method.
> (Relocate_task::layout): New method.
> (Relocate_task::object): New method.
> * reloc.cc (Relocstub_task): New task class definition.
> (Sized_relobj_file::do_relocate): Removed relocate_stub code.
> (Sized_relobj_file::do_relocate_stub_tables): New specialized functions.
> (Sized_relobj_file::do_clear_views): New specialized functions.
>
> Ok for trunk?
--
Han Shen | Software Engineer | shenhan@google.com | +1-650-440-3330
commit c116fc9649d73b396b51a521cdb04a42cbfa7bda
Author: Han Shen <shenhan@google.com>
Date: Thu Jun 22 14:39:03 2017 -0700
Fixing for gold pr/21491 - Errata workaround can produce broken images.
The root cause of the bug is that in the origin implementation,
relocate_sections and relocate_stubs phases are interleaved, in some cases,
relocate_stubs copy instructions before they are relocated in
relocate_sections phase, resulting stale and invalid insns in the final stub
table.
This patch fixes the problem by adding a new task "Relocstub_task" that runs
after "Relocate_task", so relocate_stubs always pick the relocated insn.
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.
diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index 11bb48e3b0..dbfe9a1c5c 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1781,6 +1781,56 @@ class AArch64_relobj : public Sized_relobj_file<size, big_endian>
this->set_relocs_must_follow_section_writes();
}
+ // This action is executed during Relocstub_task phase.
+ void do_relocate_stub_tables(const Symbol_table* symtab, const Layout* layout)
+ {
+ unsigned int shnum = this->shnum();
+ if (shnum == 0)
+ return;
+ const unsigned char* pshdrs =
+ this->get_view(this->elf_file()->shoff(),
+ shnum * elfcpp::Elf_sizes<size>::shdr_size,
+ true,
+ true);
+ typename Sized_relobj_file<size, big_endian>::Views* pviews = this->get_views();
+
+ Relocate_info<size, big_endian> relinfo;
+ relinfo.symtab = symtab;
+ relinfo.layout = layout;
+ relinfo.object = this;
+
+ // Relocate stub tables.
+ The_target_aarch64* target = The_target_aarch64::current_target();
+
+ for (unsigned int i = 1; i < shnum; ++i) {
+ The_aarch64_input_section* aarch64_input_section =
+ target->find_aarch64_input_section(this, i);
+ if (aarch64_input_section != NULL &&
+ aarch64_input_section->is_stub_table_owner() &&
+ !aarch64_input_section->stub_table()->empty()) {
+ Output_section* os = this->output_section(i);
+ gold_assert(os != NULL);
+
+ relinfo.reloc_shndx = elfcpp::SHN_UNDEF;
+ relinfo.reloc_shdr = NULL;
+ relinfo.data_shndx = i;
+ relinfo.data_shdr = pshdrs + i * elfcpp::Elf_sizes<size>::shdr_size;
+
+ typename Sized_relobj_file<size, big_endian>::View_size& view_struct =
+ (*pviews)[i];
+ gold_assert(view_struct.view != NULL);
+
+ The_stub_table* stub_table = aarch64_input_section->stub_table();
+ off_t offset = stub_table->address() - view_struct.address;
+ 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);
+ }
+ }
+ }
+
// Structure for mapping symbol position.
struct Mapping_symbol_position
{
@@ -2089,45 +2139,6 @@ AArch64_relobj<size, big_endian>::do_relocate_sections(
if (parameters->options().fix_cortex_a53_843419()
|| parameters->options().fix_cortex_a53_835769())
this->fix_errata(pviews);
-
- Relocate_info<size, big_endian> relinfo;
- relinfo.symtab = symtab;
- relinfo.layout = layout;
- relinfo.object = this;
-
- // Relocate stub tables.
- unsigned int shnum = this->shnum();
- The_target_aarch64* target = The_target_aarch64::current_target();
-
- for (unsigned int i = 1; i < shnum; ++i)
- {
- The_aarch64_input_section* aarch64_input_section =
- target->find_aarch64_input_section(this, i);
- if (aarch64_input_section != NULL
- && aarch64_input_section->is_stub_table_owner()
- && !aarch64_input_section->stub_table()->empty())
- {
- Output_section* os = this->output_section(i);
- gold_assert(os != NULL);
-
- relinfo.reloc_shndx = elfcpp::SHN_UNDEF;
- relinfo.reloc_shdr = NULL;
- relinfo.data_shndx = i;
- relinfo.data_shdr = pshdrs + i * elfcpp::Elf_sizes<size>::shdr_size;
-
- typename Sized_relobj_file<size, big_endian>::View_size&
- view_struct = (*pviews)[i];
- gold_assert(view_struct.view != NULL);
-
- The_stub_table* stub_table = aarch64_input_section->stub_table();
- off_t offset = stub_table->address() - view_struct.address;
- 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);
- }
- }
}
@@ -8462,4 +8473,34 @@ Target_selector_aarch64<32, false> target_selector_aarch64elf32;
Target_selector_aarch64<64, true> target_selector_aarch64elfb;
Target_selector_aarch64<64, false> target_selector_aarch64elf;
+// Specializations for AArch64_relobj methods.
+
+#ifdef HAVE_TARGET_32_LITTLE
+template
+void
+AArch64_relobj<32, false>::do_relocate_stub_tables(const Symbol_table* symtab,
+ const Layout* layout);
+#endif
+
+#ifdef HAVE_TARGET_32_BIG
+template
+void
+AArch64_relobj<32, true>::do_relocate_stub_tables(const Symbol_table* symtab,
+ const Layout* layout);
+#endif
+
+#ifdef HAVE_TARGET_64_LITTLE
+template
+void
+AArch64_relobj<64, false>::do_relocate_stub_tables(const Symbol_table* symtab,
+ const Layout* layout);
+#endif
+
+#ifdef HAVE_TARGET_64_BIG
+template
+void
+AArch64_relobj<64, true>::do_relocate_stub_tables(const Symbol_table* symtab,
+ const Layout* layout);
+#endif
+
} // End anonymous namespace.
diff --git a/gold/gold.cc b/gold/gold.cc
index a76d155f94..8f5a61f3c0 100644
--- a/gold/gold.cc
+++ b/gold/gold.cc
@@ -805,6 +805,10 @@ queue_final_tasks(const General_options& options,
bool any_postprocessing_sections = layout->any_postprocessing_sections();
+ bool need_relocstub_tasks = !parameters->options().relocatable() &&
+ (parameters->options().fix_cortex_a53_843419()
+ || parameters->options().fix_cortex_a53_835769());
+
// Use a blocker to wait until all the input sections have been
// written out.
Task_token* input_sections_blocker = NULL;
@@ -814,6 +818,9 @@ queue_final_tasks(const General_options& options,
// Write_symbols_task, Relocate_tasks.
input_sections_blocker->add_blocker();
input_sections_blocker->add_blockers(input_objects->number_of_relobjs());
+ // Blockers for n Relocstub_tasks.
+ if (need_relocstub_tasks)
+ input_sections_blocker->add_blockers(input_objects->number_of_relobjs());
}
// Use a blocker to block any objects which have to wait for the
@@ -827,6 +834,9 @@ queue_final_tasks(const General_options& options,
// Relocate_tasks.
final_blocker->add_blockers(3);
final_blocker->add_blockers(input_objects->number_of_relobjs());
+ // Blockers for n Relocstub_tasks.
+ if (need_relocstub_tasks)
+ final_blocker->add_blockers(input_objects->number_of_relobjs());
if (!any_postprocessing_sections)
final_blocker->add_blocker();
@@ -855,7 +865,17 @@ queue_final_tasks(const General_options& options,
workqueue->queue(new Relocate_task(symtab, layout, *p, of,
input_sections_blocker,
output_sections_blocker,
- final_blocker));
+ final_blocker,
+ need_relocstub_tasks));
+
+ // Queue a task for each input object to relocate stub tables.
+ if (need_relocstub_tasks)
+ for (Input_objects::Relobj_iterator p = input_objects->relobj_begin();
+ p != input_objects->relobj_end(); ++p)
+ workqueue->queue(new Relocstub_task(symtab, layout, *p, of,
+ input_sections_blocker,
+ output_sections_blocker,
+ final_blocker));
// Queue a task to write out the output sections which depend on
// input sections. If there are any sections which require
diff --git a/gold/object.cc b/gold/object.cc
index 4110686ff3..bf5d117f22 100644
--- a/gold/object.cc
+++ b/gold/object.cc
@@ -454,6 +454,18 @@ Sized_relobj<size, big_endian>::do_output_section_address(
return os->address();
}
+template<int size, bool big_endian>
+void
+Sized_relobj<size, big_endian>::do_relocate_stub_tables(
+ const Symbol_table*, const Layout*)
+{}
+
+ // Discard output_views_ created in create_views().
+template<int size, bool big_endian>
+void
+Sized_relobj<size, big_endian>::clear_views()
+{}
+
// Class Sized_relobj_file.
template<int size, bool big_endian>
@@ -501,6 +513,17 @@ Sized_relobj_file<size, big_endian>::do_setup()
this->set_shnum(shnum);
}
+// Free view memory created during Relocate_task.
+
+template<int size, bool big_endian>
+void
+Sized_relobj_file<size, big_endian>::clear_views()
+{
+ gold_assert(this->output_views_);
+ delete this->output_views_;
+ this->output_views_ = NULL;
+}
+
// Find the SHT_SYMTAB section, given the section headers. The ELF
// standard says that maybe in the future there can be more than one
// SHT_SYMTAB section. Until somebody figures out how that could
diff --git a/gold/object.h b/gold/object.h
index 508e79cb3c..b9c4efbb62 100644
--- a/gold/object.h
+++ b/gold/object.h
@@ -1279,6 +1279,10 @@ class Relobj : public Object
relocate(const Symbol_table* symtab, const Layout* layout, Output_file* of)
{ return this->do_relocate(symtab, layout, of); }
+ void
+ relocate_stub_tables(const Symbol_table* symtab, const Layout* layout)
+ { this->do_relocate_stub_tables(symtab, layout); }
+
// Return whether an input section is being included in the link.
bool
is_section_included(unsigned int shndx) const
@@ -1375,6 +1379,9 @@ class Relobj : public Object
is_big_endian() const
{ return this->do_is_big_endian(); }
+ virtual void
+ clear_views() = 0;
+
protected:
// The output section to be used for each input section, indexed by
// the input section number. The output section is NULL if the
@@ -1457,6 +1464,9 @@ class Relobj : public Object
virtual void
do_relocate(const Symbol_table* symtab, const Layout*, Output_file* of) = 0;
+ virtual void
+ do_relocate_stub_tables(const Symbol_table*, const Layout*) = 0;
+
// Set the offset of a section--implemented by child class.
virtual void
do_set_section_offset(unsigned int shndx, uint64_t off) = 0;
@@ -2040,6 +2050,14 @@ class Sized_relobj : public Relobj
void
do_for_all_local_got_entries(Got_offset_list::Visitor* v) const;
+ virtual
+ void
+ do_relocate_stub_tables(const Symbol_table*, const Layout*);
+
+ virtual
+ void
+ clear_views();
+
protected:
typedef Relobj::Output_sections Output_sections;
@@ -2503,6 +2521,10 @@ class Sized_relobj_file : public Sized_relobj<size, big_endian>
do_get_global_symbols() const
{ return &this->symbols_; }
+ virtual
+ void
+ clear_views();
+
// Adjust a section index if necessary.
unsigned int
adjust_shndx(unsigned int shndx)
@@ -2531,7 +2553,7 @@ class Sized_relobj_file : public Sized_relobj<size, big_endian>
elfcpp::Elf_file<size, big_endian, Object>*
elf_file()
{ return &this->elf_file_; }
-
+
// Allow a child class to access the local values.
Local_values*
local_values()
@@ -2584,6 +2606,22 @@ class Sized_relobj_file : public Sized_relobj<size, big_endian>
unsigned char*
do_get_output_view(unsigned int, section_size_type*) const;
+ Views*
+ create_views()
+ {
+ gold_assert(this->output_views_ == NULL);
+ this->output_views_ = new Views();
+ return this->output_views_;
+ }
+
+ // Allow child access to output_views_.
+ Views*
+ get_views() const
+ {
+ gold_assert(this->output_views_);
+ return this->output_views_;
+ }
+
private:
// For convenience.
typedef Sized_relobj_file<size, big_endian> This;
@@ -2853,7 +2891,7 @@ class Sized_relobj_file : public Sized_relobj<size, big_endian>
// The list of relocation sections whose layout was deferred.
std::vector<Deferred_layout> deferred_layout_relocs_;
// Pointer to the list of output views; valid only during do_relocate().
- const Views* output_views_;
+ Views* output_views_;
};
// A class to manage the list of all objects.
diff --git a/gold/reloc.cc b/gold/reloc.cc
index 26d84d0aa8..cea4b260f7 100644
--- a/gold/reloc.cc
+++ b/gold/reloc.cc
@@ -238,11 +238,15 @@ Relocate_task::run(Workqueue*)
{
this->object_->relocate(this->symtab_, this->layout_, this->of_);
- // 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();
+ }
}
// Return a debugging name for the task.
@@ -253,6 +257,25 @@ Relocate_task::get_name() const
return "Relocate_task " + this->object_->name();
}
+std::string
+Relocstub_task::get_name() const
+{
+ return "Relocstub_task " + this->object()->name();
+}
+
+void
+Relocstub_task::run(Workqueue*)
+{
+ this->object()->relocate_stub_tables(this->symbol_table(), this->layout());
+
+ // 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();
+
+}
+
// Read the relocs and local symbols from the object file and store
// the information in RD.
@@ -595,7 +618,9 @@ Sized_relobj_file<size, big_endian>::do_relocate(const Symbol_table* symtab,
shnum * This::shdr_size,
true, true);
- Views views;
+ // "Views" created here is either discarded at the end of "Relocate_task" or
+ // "Relocstub_task", depending whether the latter ones are needed.
+ Views& views = *(this->create_views());
views.resize(shnum);
// Make two passes over the sections. The first one copies the
@@ -608,24 +633,6 @@ Sized_relobj_file<size, big_endian>::do_relocate(const Symbol_table* symtab,
// input offsets to output addresses.
this->initialize_input_to_output_maps();
- // Make the views available through get_output_view() for the duration
- // of this routine. This RAII class will reset output_views_ to NULL
- // when the views go out of scope.
- struct Set_output_views
- {
- Set_output_views(const Views** ppviews, const Views* pviews)
- {
- ppviews_ = ppviews;
- *ppviews = pviews;
- }
-
- ~Set_output_views()
- { *ppviews_ = NULL; }
-
- const Views** ppviews_;
- };
- Set_output_views set_output_views(&this->output_views_, &views);
-
// Apply relocations.
this->relocate_sections(symtab, layout, pshdrs, of, &views);
@@ -1728,6 +1735,7 @@ void
Sized_relobj_file<64, true>::do_relocate(const Symbol_table* symtab,
const Layout* layout,
Output_file* of);
+
#endif
#ifdef HAVE_TARGET_32_LITTLE
diff --git a/gold/reloc.h b/gold/reloc.h
index 7ec247411d..9e5ebf06b9 100644
--- a/gold/reloc.h
+++ b/gold/reloc.h
@@ -183,27 +183,38 @@ class Relocate_task : public Task
Relocate_task(const Symbol_table* symtab, const Layout* layout,
Relobj* object, Output_file* of,
Task_token* input_sections_blocker,
- Task_token* output_sections_blocker, Task_token* final_blocker)
+ Task_token* output_sections_blocker, Task_token* final_blocker,
+ bool defer_object_cleanup)
: symtab_(symtab), layout_(layout), object_(object), of_(of),
input_sections_blocker_(input_sections_blocker),
output_sections_blocker_(output_sections_blocker),
- final_blocker_(final_blocker)
+ final_blocker_(final_blocker),
+ defer_object_cleanup_(defer_object_cleanup)
{ }
// The standard Task methods.
- 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;
+ const Symbol_table* symbol_table() const
+ { return this->symtab_; }
+
+ const Layout* layout() const
+ { return this->layout_; }
+
+ Relobj* object() const
+ { return this->object_; }
+
private:
const Symbol_table* symtab_;
const Layout* layout_;
@@ -212,6 +223,39 @@ class Relocate_task : public Task
Task_token* input_sections_blocker_;
Task_token* output_sections_blocker_;
Task_token* final_blocker_;
+ // When this is true, do not do object cleanup, because later, Relocstub_task
+ // will do the chores. Only use this in Relocate_task. Not accessible in
+ // subclasses.
+ bool defer_object_cleanup_;
+};
+
+// This task relocates stub_tables. It does similar things as a
+// Relocate_task. The reason why we have this is that we have to wait for all
+// instructions to be relocated before we can copy the instruction into
+// stub_tables. More details here - pr/21491.
+
+class Relocstub_task : public Relocate_task
+{
+ public:
+
+ Relocstub_task(const Symbol_table* symtab,
+ const Layout* layout,
+ Relobj* object,
+ Output_file* of,
+ Task_token* input_sections_blocker,
+ Task_token* output_sections_blocker,
+ Task_token* final_blocker)
+ : Relocate_task(symtab, layout, object, of,
+ input_sections_blocker,
+ output_sections_blocker,
+ final_blocker, true)
+ {}
+
+ virtual void
+ run(Workqueue*);
+
+ virtual std::string
+ get_name() const;
};
// During a relocatable link, this class records how relocations