Re: [RFC] COMDAT Safe Module Level Multi versioning

[Yury Gribov <y.gribov@samsung.com>]

On 05/19/2015 09:16 AM, Sriraman Tallam wrote:
> We have the following problem with selectively compiling modules with
> -m<isa> options and I have provided a solution to solve this.  I would
> like to hear what you think.
>
> 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
>
>
> To solve this, I propose introducing a new compiler option, say
> -fodr-unsafe-comdats, to let the user tag objects that use specialized
> options and let the linker choose the comdat candidate to be linked
> wisely.  The root cause of the above problem is that comdat functions
> in common headers may not be properly guarded and the linker picks the
> first candidate it sees.  A link order where the object with the
> specialized comdat functions appear first causes these comdats to be
> picked leading to SIGILL on unsupported arches.  With the objects
> tagged, the linker can be made to pick other comdat candidates when
> possible.
>
> More details:
>
> This option is user specified when using arch specific options like
> -m<isa>.  It is an indicator to the compiler that any comdat bodies
> generated are potentially unsafe for execution.  Note that the COMDAT
> bodies however have to be generated as there are no guarantees that
> other modules will do so.  The compiler then emits a specially named
> section, like “.gnu.odr.unsafe”, in the object file.  When the linker
> tries to pick a COMDAT candidate from several choices, it must avoid
> COMDAT copies from objects with sections named “.gnu.odr.unsafe” when
> presented with a choice to pick a candidate from an object that does
> not have the “.gnu.odr.unsafe” section.  Note that it may not be
> possible to do that in which case the linker must pick the unsafe
> copy, it could explicitly warn when this happens.
>
> Alternately,  the compiler can bind locally any emitted comdat version
> from a specialized module, which could also be guarded by an option.
> This will solve the problem but this may not be always possible
> especially when addresses of any such comdat version is taken.

Can IFUNC relocations be used to properly select optimal version of code 
at runtime?

-Y