Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam <tmsriram at google dot com>]

On Tue, May 19, 2015 at 10:22 AM, Yury Gribov <y.gribov@samsung.com> wrote:
> 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?

Yes, we do want a solution like this but all the dispatching code for
IFUNC needs to be generated at link-time.   Here is an example header
file with target-specific functionalities :

https://bitbucket.org/eigen/eigen/src/6ed647a644b8e3924800f0916a4ce4addf9e7739/Eigen/Core?at=default

This header can be included in several modules and  the modules may be
specialized for SSE4.2, AVX, etc respectively when compiling for x86.
The calls to functions defined in each module are appropriately
guarded by target checks using builtins but each such  module may
generate specialized COMDAT candidates which needs to be carefully
handled by the linker.

Only the linker sees the various COMDAT choices, so we need to
generate the body of the IFUNC based dispatcher at link-time.  The
linker needs to do quite a bit here.  We need a mechanism to tell the
linker what extra options were used to generate each COMDAT copy and
what platform checks are needed before this copy can be executed.

Thanks
Sri



>
> -Y
>