[PATCH] Two level comdat priorities in gold
Sriraman Tallam
tmsriram@google.com
Mon Jul 13 23:17:00 GMT 2015
Hi,
This patch implements a two level comdat function priority. Comdats defined
in those objects with a named section ".gnu.comdat.low" are treated with a
lower priority. If this patch is suitable, I could add compiler option
-fcomdat-priority-low which creates this named section in the object file. For
testing, I have used the asm directive to create this named section. Comdat
priority checking is enabled and honored with new option
--honor-comdat-priority.
Context: I have created this patch to support Module Level Multiversioning.
Multi versioning at module granularity is done by compiling a subset of modules
with advanced ISA instructions, supported on later generations of the target
architecture, via -m<isa> options and invoking the functions defined in these
modules with explicit checks for the ISA support via builtin functions,
__builtin_cpu_supports. This mechanism has the unfortunate side-effect that
generated COMDAT candidates from these modules can contain these advanced
instructions and potentially “violate” ODR assumptions. Choosing such a COMDAT
candidate over a generic one from a different module can cause SIGILL on
platforms where the advanced ISA is not supported.
Here is a slightly contrived example to illustrate:
matrixdouble.h
--------------------
// Template (Comdat) function definition in a header:
template<typename T>
__attribute__((noinline))
void matrixDouble (T *a) {
for (int i = 0 ; i < 16; ++i) //Vectorizable Loop
a[i] = a[i] * 2;
}
avx.cc (Compile with -mavx -O2)
---------
#include "matrixdouble.h"
void getDoubleAVX(int *a) {
matrixDouble(a); // Instantiated with vectorized AVX instructions
}
non_avx.cc (Compile with -mno-avx -O2)
---------------
#include “matrixdouble.h”
void
getDouble(int *a) {
matrixDouble(a); // Instantiated with non-AVX instructions
}
main.cc
-----------
void getDoubleAVX(int *a);
void getDouble(int *a);
int a[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
int main () {
// The AVX call is appropriately guarded.
if (__builtin_cpu_supports(“avx”))
getDoubleAVX(a);
else
getDouble(a);
return a[0];
}
In the above code, function “getDoubleAVX” is only called when the
run-time CPU supports AVX instructions. This code looks clean but
suffers from the COMDAT ODR violation. Two copies of COMDAT function
“matrixDouble” are generated. One copy is generated in object file
“avx.o” with AVX instructions and another copy exists in “non_avx.o”
without AVX instruction. At link time, in a link order where object
file avx.o is seen ahead of non_avx.o, the COMDAT copy of function
“matrixDouble” that contains AVX instructions is kept leading to
SIGILL on unsupported platforms. To reproduce the SIGILL,
$ g++ -c -O2 -mavx avx.cc
$ g++ -c -O2 -mno-avx non_avx.cc
$ g++ main.cc avx.o non_avx.o
$ ./a.out # on a non-AVX machine
Illegal Instruction
Comdat priorities can solve the above problem by tagging the avx.cc file as
having lower priority comdats.
Patch implementation details:
Thanks to the two-pass approach available in do_layout (object.cc), implementing
two level comdat priority is easy. The first pass processes all default
priority comdat groups and the second pass then processes the lower priority
comdat groups. This ensures that a lower priority candidate is never picked
when a default priority substitute is available.
* gold.cc (queue_middle_tasks): Check if comdat priority is enabled.
* object.cc (do_layout): Do it as two pass if comdat priority is
enabled. Check for low priority comdats. Do not process low priority
comdat groups in the first pass.
* object.h (Object::Object): Initialize has_low_priority_comdats_
(Object::has_low_priority_comdats): New function.
(Object::set_has_low_priority_comdats): New function
(Object::has_low_priority_comdats_): New member.
* options.h (--honor-comdat-priority): New option.
* resolve.cc (Symbol_table::higher_priority_comdat_symbol): New function.
(should_override): Check for a higher priority comdat symbol.
* symtab.h (Symbol_table::higher_priority_comdat_symbol): New function.
* testsuite/Makefile.am (comdat_priority_test): New test.
* testsuite/comdat_priority_test_default.cc: New file.
* testsuite/comdat_priority_test_low.cc: New file.
Is this reasonable?
Patch attached.
Thanks
Sri
-------------- next part --------------
This patch implements a two level comdat function priority. Comdats defined
in those objects with a named section ".gnu.comdat.low" are treated with a
lower priority. If this patch is suitable, I could add compiler option
-fcomdat-priority-low which creates this named section in the object file. For
testing, I have used the asm directive to create this named section. Comdat
priority checking is enabled and honored with new option
--honor-comdat-priority.
Context: I have created this patch to support Module Level Multiversioning.
Multi versioning at module granularity is done by compiling a subset of modules
with advanced ISA instructions, supported on later generations of the target
architecture, via -m<isa> options and invoking the functions defined in these
modules with explicit checks for the ISA support via builtin functions,
__builtin_cpu_supports. This mechanism has the unfortunate side-effect that
generated COMDAT candidates from these modules can contain these advanced
instructions and potentially “violate” ODR assumptions. Choosing such a COMDAT
candidate over a generic one from a different module can cause SIGILL on
platforms where the advanced ISA is not supported.
Here is a slightly contrived example to illustrate:
matrixdouble.h
--------------------
// Template (Comdat) function definition in a header:
template<typename T>
__attribute__((noinline))
void matrixDouble (T *a) {
for (int i = 0 ; i < 16; ++i) //Vectorizable Loop
a[i] = a[i] * 2;
}
avx.cc (Compile with -mavx -O2)
---------
#include "matrixdouble.h"
void getDoubleAVX(int *a) {
matrixDouble(a); // Instantiated with vectorized AVX instructions
}
non_avx.cc (Compile with -mno-avx -O2)
---------------
#include “matrixdouble.h”
void
getDouble(int *a) {
matrixDouble(a); // Instantiated with non-AVX instructions
}
main.cc
-----------
void getDoubleAVX(int *a);
void getDouble(int *a);
int a[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
int main () {
// The AVX call is appropriately guarded.
if (__builtin_cpu_supports(“avx”))
getDoubleAVX(a);
else
getDouble(a);
return a[0];
}
In the above code, function “getDoubleAVX” is only called when the
run-time CPU supports AVX instructions. This code looks clean but
suffers from the COMDAT ODR violation. Two copies of COMDAT function
“matrixDouble” are generated. One copy is generated in object file
“avx.o” with AVX instructions and another copy exists in “non_avx.o”
without AVX instruction. At link time, in a link order where object
file avx.o is seen ahead of non_avx.o, the COMDAT copy of function
“matrixDouble” that contains AVX instructions is kept leading to
SIGILL on unsupported platforms. To reproduce the SIGILL,
$ g++ -c -O2 -mavx avx.cc
$ g++ -c -O2 -mno-avx non_avx.cc
$ g++ main.cc avx.o non_avx.o
$ ./a.out # on a non-AVX machine
Illegal Instruction
Comdat priorities can solve the above problem by tagging the avx.cc file as
having lower priority comdats.
Patch implementation details:
Thanks to the two-pass approach available in do_layout (object.cc), implementing
two level comdat priority is easy. The first pass processes all default
priority comdat groups and the second pass then processes the lower priority
comdat groups. This ensures that a lower priority candidate is never picked
when a higher priority substitute is available.
* gold.cc (queue_middle_tasks): Check if comdat priority is enabled.
* object.cc (do_layout): Do it as two pass if comdat priority is
enabled. Check for low priority comdats. Do not process low priority
comdat groups in the first pass.
* object.h (Object::Object): Initialize has_low_priority_comdats_
(Object::has_low_priority_comdats): New function.
(Object::set_has_low_priority_comdats): New function
(Object::has_low_priority_comdats_): New member.
* options.h (--honor-comdat-priority): New option.
* resolve.cc (Symbol_table::higher_priority_comdat_symbol): New function.
(should_override): Check for a higher priority comdat symbol.
* symtab.h (Symbol_table::higher_priority_comdat_symbol): New function.
* testsuite/Makefile.am (comdat_priority_test): New test.
* testsuite/comdat_priority_test_default.cc: New file.
* testsuite/comdat_priority_test_low.cc: New file.
diff --git a/gold/gold.cc b/gold/gold.cc
index 18b06b9..1157fe0 100644
--- a/gold/gold.cc
+++ b/gold/gold.cc
@@ -533,7 +533,8 @@ queue_middle_tasks(const General_options& options,
// are added.
if (parameters->options().gc_sections()
|| parameters->options().icf_enabled()
- || layout->is_unique_segment_for_sections_specified())
+ || layout->is_unique_segment_for_sections_specified()
+ || parameters->options().honor_comdat_priority())
{
for (Input_objects::Relobj_iterator p = input_objects->relobj_begin();
p != input_objects->relobj_end();
diff --git a/gold/object.cc b/gold/object.cc
index 316f8d4..f0d9538 100644
--- a/gold/object.cc
+++ b/gold/object.cc
@@ -1347,7 +1347,8 @@ Sized_relobj_file<size, big_endian>::do_layout(Symbol_table* symtab,
/* Should this function be called twice? */
bool is_two_pass = (parameters->options().gc_sections()
|| parameters->options().icf_enabled()
- || layout->is_unique_segment_for_sections_specified());
+ || layout->is_unique_segment_for_sections_specified()
+ || parameters->options().honor_comdat_priority());
/* Only one of is_pass_one and is_pass_two is true. Both are false when
a two-pass approach is not needed. */
@@ -1469,6 +1470,16 @@ Sized_relobj_file<size, big_endian>::do_layout(Symbol_table* symtab,
reloc_type[target_shndx] = sh_type;
}
}
+
+ // Find named section ".gnu.comdat.low". If found, this makes all
+ // comdats in this object of lower priority than the default.
+ if (parameters->options().honor_comdat_priority()
+ && shdr.get_sh_name() < section_names_size)
+ {
+ const char *name = pnames + shdr.get_sh_name();
+ if (strcmp (name, ".gnu.comdat.low") == 0)
+ this->set_has_low_priority_comdats();
+ }
}
Output_sections& out_sections(this->output_sections());
@@ -1575,10 +1586,11 @@ Sized_relobj_file<size, big_endian>::do_layout(Symbol_table* symtab,
{
if (shdr.get_sh_type() == elfcpp::SHT_GROUP)
{
- if (!this->include_section_group(symtab, layout, i, name,
- shdrs, pnames,
- section_names_size,
- &omit))
+ if (!this->has_low_priority_comdats()
+ && !this->include_section_group(symtab, layout, i, name,
+ shdrs, pnames,
+ section_names_size,
+ &omit))
discard = true;
}
else if ((shdr.get_sh_flags() & elfcpp::SHF_GROUP) == 0
@@ -1611,6 +1623,34 @@ Sized_relobj_file<size, big_endian>::do_layout(Symbol_table* symtab,
}
}
+ // In the second pass allow low priority comdat candidates to get picked.
+ // The first pass picked all the default priority comdat candidates so it
+ // is fine to pick low priority ones if necessary as that only means
+ // there weren't any default priority comdat candidates with the same
+ // comdat signature.
+ if (is_pass_two && this->has_low_priority_comdats())
+ {
+ bool discard = omit[i];
+ if (!discard)
+ {
+ if (shdr.get_sh_type() == elfcpp::SHT_GROUP)
+ {
+ if (!this->include_section_group(symtab, layout, i, name,
+ shdrs, pnames,
+ section_names_size,
+ &omit))
+ discard = true;
+ }
+ }
+ if (discard)
+ {
+ out_sections[i] = NULL;
+ out_section_offsets[i] = invalid_address;
+ continue;
+ }
+ }
+
+
if (is_pass_one && parameters->options().gc_sections())
{
if (this->is_section_name_included(name)
diff --git a/gold/object.h b/gold/object.h
index e8f74ac..a54d583 100644
--- a/gold/object.h
+++ b/gold/object.h
@@ -347,7 +347,8 @@ class Object
Object(const std::string& name, Input_file* input_file, bool is_dynamic,
off_t offset = 0)
: name_(name), input_file_(input_file), offset_(offset), shnum_(-1U),
- is_dynamic_(is_dynamic), is_needed_(false), uses_split_stack_(false),
+ is_dynamic_(is_dynamic), is_needed_(false),
+ has_low_priority_comdats_(false), uses_split_stack_(false),
has_no_split_stack_(false), no_export_(false),
is_in_system_directory_(false), as_needed_(false), xindex_(NULL),
compressed_sections_(NULL)
@@ -400,6 +401,16 @@ class Object
set_is_needed()
{ this->is_needed_ = true; }
+ // Return true if this object's comdats are of lower priority than the
+ // default.
+ bool
+ has_low_priority_comdats() const
+ { return this->has_low_priority_comdats_; }
+
+ void
+ set_has_low_priority_comdats()
+ { this->has_low_priority_comdats_ = true; }
+
// Return whether this object was compiled with -fsplit-stack.
bool
uses_split_stack() const
@@ -1001,6 +1012,9 @@ class Object
// objects, and means that the object defined a symbol which was
// used by a reference from a regular object.
bool is_needed_ : 1;
+ // Whether this object defines comdat functions that are of lower priority
+ // than the default.
+ bool has_low_priority_comdats_ : 1;
// Whether this object was compiled with -fsplit-stack.
bool uses_split_stack_ : 1;
// Whether this object contains any functions compiled with the
diff --git a/gold/options.h b/gold/options.h
index f4da128..3e0307c 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -1133,6 +1133,14 @@ class General_options
N_("List removed unused sections on stderr"),
N_("Do not list removed unused sections"));
+ // Two level comdat priority, default and low. Comdat functions in objects
+ // with named section ".gnu.comdat.low" have lower than default priority.
+ // Option --honor-comdat-priority picks default priority comdat candidates
+ // over low priority candidates when possible.
+ DEFINE_bool(honor_comdat_priority, options::TWO_DASHES, '\0', false,
+ N_("Choose default priority comdats over low priority comdats"),
+ N_("Pick the first seen comdat candidate (default)"));
+
DEFINE_bool(stats, options::TWO_DASHES, '\0', false,
N_("Print resource usage statistics"), NULL);
diff --git a/gold/resolve.cc b/gold/resolve.cc
index fdae0ba..bd6f00a 100644
--- a/gold/resolve.cc
+++ b/gold/resolve.cc
@@ -443,6 +443,31 @@ Symbol_table::resolve(Sized_symbol<size>* to,
}
}
+// Check for a higher priority comdat symbol. If symbol TO points to a low
+// priority comdat section and a symbol in OBJECT points to a default priority
+// comdat section, return true. This is called by function should_override.
+
+bool
+Symbol_table::is_higher_priority_comdat_symbol(const Symbol* to,
+ Object* object,
+ elfcpp::STT fromtype)
+{
+ bool is_ordinary = false;
+
+ if (parameters->options().honor_comdat_priority()
+ && to->source() == Symbol::FROM_OBJECT
+ && to->object() != object
+ && to->shndx(&is_ordinary) != -1U
+ && is_ordinary
+ && to->type() == fromtype
+ && (to->object()->section_flags(to->shndx(&is_ordinary))
+ & elfcpp::SHF_GROUP) != 0
+ && to->object()->has_low_priority_comdats()
+ && !object->has_low_priority_comdats())
+ return true;
+ return false;
+}
+
// Handle the core of symbol resolution. This is called with the
// existing symbol, TO, and a bitflag describing the new symbol. This
// returns true if we should override the existing symbol with the new
@@ -518,6 +543,9 @@ Symbol_table::should_override(const Symbol* to, unsigned int frombits,
|| (object != NULL && object->just_symbols()))
return false;
+ if (is_higher_priority_comdat_symbol(to, object, fromtype))
+ return true;
+
if (!parameters->options().muldefs())
Symbol_table::report_resolve_problem(true,
_("multiple definition of '%s'"),
@@ -566,6 +594,8 @@ Symbol_table::should_override(const Symbol* to, unsigned int frombits,
case DEF * 16 + WEAK_DEF:
case WEAK_DEF * 16 + WEAK_DEF:
+ if (is_higher_priority_comdat_symbol(to, object, fromtype))
+ return true;
// We've seen a definition and now we see a weak definition. We
// ignore the new weak definition.
return false;
diff --git a/gold/symtab.h b/gold/symtab.h
index 2c9aa32..265132a 100644
--- a/gold/symtab.h
+++ b/gold/symtab.h
@@ -1721,6 +1721,10 @@ class Symbol_table
const char*
wrap_symbol(const char* name, Stringpool::Key* name_key);
+ // Returns true if a higher priority comdat symbol is found.
+ static bool
+ is_higher_priority_comdat_symbol(const Symbol*, Object*, elfcpp::STT);
+
// Whether we should override a symbol, based on flags in
// resolve.cc.
static bool
diff --git a/gold/testsuite/comdat_priority_test_default.cc b/gold/testsuite/comdat_priority_test_default.cc
new file mode 100644
index 0000000..604d50f
--- /dev/null
+++ b/gold/testsuite/comdat_priority_test_default.cc
@@ -0,0 +1,29 @@
+// comdat_priority_test_default.cc -- a test case for gold
+
+// Copyright (C) 2015 Free Software Foundation, Inc.
+// Written by Sriraman Tallam <tmsriram@google.com>.
+
+// This file is part of gold.
+
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+// MA 02110-1301, USA.
+
+// The goal of this program is to verify if --honor-comdat-priority works
+// as intended.
+
+inline int foo() {
+ static int a = 0;
+ return a;
+}
diff --git a/gold/testsuite/comdat_priority_test_low.cc b/gold/testsuite/comdat_priority_test_low.cc
new file mode 100644
index 0000000..999415f
--- /dev/null
+++ b/gold/testsuite/comdat_priority_test_low.cc
@@ -0,0 +1,43 @@
+// comdat_priority_test_low.cc -- a test case for gold
+
+// Copyright (C) 2015 Free Software Foundation, Inc.
+// Written by Sriraman Tallam <tmsriram@google.com>.
+
+// This file is part of gold.
+
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+// MA 02110-1301, USA.
+
+// The goal of this program is to verify if --honor-comdat-priority works
+// as intended. This file contains low priority comdats because of named
+// section ".gnu.comdat.low". A default priority comdat exists in file
+// comdat_priority_test_default.cc for foo which must be picked over the one
+// defined in this file. If foo defined in this file is picked, it will
+// abort.
+
+#include <stdlib.h>
+
+inline int foo() {
+ static int a = 0;
+ if (a == 0)
+ abort();
+ return a;
+}
+
+int main() {
+ return foo();
+}
+
+asm(".section .gnu.comdat.low");
More information about the Binutils
mailing list