[PATCH] Two level comdat priorities in gold

[Sriraman Tallam <tmsriram at google dot com>]

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

Attachment: gold_comdat_priority_patch.txt
Description: Text document