[PATCH] Two level comdat priorities in gold

[Sriraman Tallam]

Ping.

On Mon, Jul 13, 2015 at 4:17 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> 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