[RFC] COMDAT Safe Module Level Multi versioning

[RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

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.


Thanks
Sri

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Richard Biener]

On Tue, May 19, 2015 at 8:16 AM, Sriraman Tallam <tmsriram@google.com> 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.

Hm.  But which options are unsafe?  Also wouldn't it be better to simply
_not_ have unsafe options produce comdats but always make local clones
for them (thus emit the comdat with "unsafe" flags dropped)?

Richard.

>
> Thanks
> Sri

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Xinliang David Li]

>
> Hm.  But which options are unsafe?  Also wouldn't it be better to simply
> _not_ have unsafe options produce comdats but always make local clones
> for them (thus emit the comdat with "unsafe" flags dropped)?

Always localize comdat functions may lead to text size increase. It
does not work if the comdat function is a virtual function for
instance.

David


>
> Richard.
>
>>
>> Thanks
>> Sri

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Tue, May 19, 2015 at 2:39 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, May 19, 2015 at 8:16 AM, Sriraman Tallam <tmsriram@google.com> 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.
>
> Hm.  But which options are unsafe?  Also wouldn't it be better to simp

In general, should that be any option that affects code gen and is
only *applied to a subset of modules* is potentially unsafe as the
comdat copies generated from those modules are not identical to the
copies from other modules.  Tagging such modules with
-fodr-unsafe-comdats, even conservatively, is fine.  In the worst
case, the comdat candidate from that module is the only available
candidate and the linker uses that and emits a non-fatal message that
this comdat was used.  Shouldn't that be alright?

Thanks
Sri


> _not_ have unsafe options produce comdats but always make local clones
> for them (thus emit the comdat with "unsafe" flags dropped)?
>
> Richard.
>
>>
>> Thanks
>> Sri

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Yury Gribov]

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

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

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
>

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li <davidxl@google.com> wrote:
>>
>> Hm.  But which options are unsafe?  Also wouldn't it be better to simply
>> _not_ have unsafe options produce comdats but always make local clones
>> for them (thus emit the comdat with "unsafe" flags dropped)?
>
> Always localize comdat functions may lead to text size increase. It
> does not work if the comdat function is a virtual function for
> instance.

Based on Richard's suggestion, I have a patch to localize comdat
functions which seems like a very effective solution to this problem.
The text size increase is limited to the extra comdat copies generated
for the specialized modules (modules with unsafe options) which is
usually only a few.   Since -fweak does something similar for
functions,  I have called the new option -fweak-comdat-functions.
This does not apply to virtual comdat functions as their addresses can
always be leaked via the vtable. Using this flag with virtual
functions generates a warning.

To summarize, this is the intended usage of this option. Modules which
use unsafe code options, like -m<isa> for multiversioning, to generate
code that is meant to run only on a subset of CPUs can generate
comdats with specialized instructions which when picked by the linker
can get run unconditionally causing SIGILL on unsupported platforms.
This flag hides these comdats to be local to these modules and not
make them available publicly,  with the caveat that it does not apply
to virtual comdats.

Could you please review?

* c-family/c.opt (fweak-comdat-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.


Thanks
Sri


>
> David
>
>
>>
>> Richard.
>>
>>>
>>> Thanks
>>> Sri

[-- Attachment #2: no_weak_comdat_functions.txt --]
[-- Type: text/plain, Size: 4095 bytes --]

	* c-family/c.opt (fweak-comdat-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 224486)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,14 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions.  With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 224486)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,19 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak)
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_weak_comdat_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_weak_comdat_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-weak-comdat-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.");
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 224486)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-weak-comdat-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2445,6 +2445,16 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker.  By default, G++ uses weak symbols if they are
+available.  This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally.  This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables.  This can cause unintended behavior if
+the addresses of comdat functions are used.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: testsuite/g++.dg/no-weak-comdat-functions-1.C
===================================================================
--- testsuite/g++.dg/no-weak-comdat-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-weak-comdat-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-weak-comdat-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>
>>> Hm.  But which options are unsafe?  Also wouldn't it be better to simply
>>> _not_ have unsafe options produce comdats but always make local clones
>>> for them (thus emit the comdat with "unsafe" flags dropped)?
>>
>> Always localize comdat functions may lead to text size increase. It
>> does not work if the comdat function is a virtual function for
>> instance.
>
> Based on Richard's suggestion, I have a patch to localize comdat
> functions which seems like a very effective solution to this problem.
> The text size increase is limited to the extra comdat copies generated
> for the specialized modules (modules with unsafe options) which is
> usually only a few.   Since -fweak does something similar for
> functions,  I have called the new option -fweak-comdat-functions.
> This does not apply to virtual comdat functions as their addresses can
> always be leaked via the vtable. Using this flag with virtual
> functions generates a warning.
>
> To summarize, this is the intended usage of this option. Modules which
> use unsafe code options, like -m<isa> for multiversioning, to generate
> code that is meant to run only on a subset of CPUs can generate
> comdats with specialized instructions which when picked by the linker
> can get run unconditionally causing SIGILL on unsupported platforms.
> This flag hides these comdats to be local to these modules and not
> make them available publicly,  with the caveat that it does not apply
> to virtual comdats.
>
> Could you please review?

Ping.  This patch uses Richard's suggestion to localize comdat
functions with option -fno-weak-comdat-functions.  Comments?

* c-family/c.opt (fweak-comdat-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.


>
> * c-family/c.opt (fweak-comdat-functions): New option.
> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
> virtual comdat functions are seen.
> * doc/invoke.texi: Document new option.
> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>
>
> Thanks
> Sri
>
>
>>
>> David
>>
>>
>>>
>>> Richard.
>>>
>>>>
>>>> Thanks
>>>> Sri

[-- Attachment #2: no_weak_comdat_functions.txt --]
[-- Type: text/plain, Size: 4095 bytes --]

	* c-family/c.opt (fweak-comdat-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 224486)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,14 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions.  With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 224486)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,19 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak)
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_weak_comdat_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_weak_comdat_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-weak-comdat-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.");
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 224486)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-weak-comdat-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2445,6 +2445,16 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker.  By default, G++ uses weak symbols if they are
+available.  This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally.  This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables.  This can cause unintended behavior if
+the addresses of comdat functions are used.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: testsuite/g++.dg/no-weak-comdat-functions-1.C
===================================================================
--- testsuite/g++.dg/no-weak-comdat-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-weak-comdat-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-weak-comdat-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]

On Tue, Aug 4, 2015 at 11:43 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>
>>>> Hm.  But which options are unsafe?  Also wouldn't it be better to simply
>>>> _not_ have unsafe options produce comdats but always make local clones
>>>> for them (thus emit the comdat with "unsafe" flags dropped)?
>>>
>>> Always localize comdat functions may lead to text size increase. It
>>> does not work if the comdat function is a virtual function for
>>> instance.
>>
>> Based on Richard's suggestion, I have a patch to localize comdat
>> functions which seems like a very effective solution to this problem.
>> The text size increase is limited to the extra comdat copies generated
>> for the specialized modules (modules with unsafe options) which is
>> usually only a few.   Since -fweak does something similar for
>> functions,  I have called the new option -fweak-comdat-functions.
>> This does not apply to virtual comdat functions as their addresses can
>> always be leaked via the vtable. Using this flag with virtual
>> functions generates a warning.
>>
>> To summarize, this is the intended usage of this option. Modules which
>> use unsafe code options, like -m<isa> for multiversioning, to generate
>> code that is meant to run only on a subset of CPUs can generate
>> comdats with specialized instructions which when picked by the linker
>> can get run unconditionally causing SIGILL on unsupported platforms.
>> This flag hides these comdats to be local to these modules and not
>> make them available publicly,  with the caveat that it does not apply
>> to virtual comdats.
>>
>> Could you please review?
>
> Ping.  This patch uses Richard's suggestion to localize comdat
> functions with option -fno-weak-comdat-functions.  Comments?


Ping.

* c-family/c.opt (fweak-comdat-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.

>
> * c-family/c.opt (fweak-comdat-functions): New option.
> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
> virtual comdat functions are seen.
> * doc/invoke.texi: Document new option.
> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>
>
>>
>> * c-family/c.opt (fweak-comdat-functions): New option.
>> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
>> virtual comdat functions are seen.
>> * doc/invoke.texi: Document new option.
>> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>>
>>
>> Thanks
>> Sri
>>
>>
>>>
>>> David
>>>
>>>
>>>>
>>>> Richard.
>>>>
>>>>>
>>>>> Thanks
>>>>> Sri

[-- Attachment #2: no_weak_comdat_functions.txt --]
[-- Type: text/plain, Size: 4095 bytes --]

	* c-family/c.opt (fweak-comdat-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 224486)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,14 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions.  With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 224486)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,19 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak)
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_weak_comdat_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_weak_comdat_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-weak-comdat-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.");
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 224486)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-weak-comdat-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2445,6 +2445,16 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker.  By default, G++ uses weak symbols if they are
+available.  This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally.  This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables.  This can cause unintended behavior if
+the addresses of comdat functions are used.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: testsuite/g++.dg/no-weak-comdat-functions-1.C
===================================================================
--- testsuite/g++.dg/no-weak-comdat-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-weak-comdat-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-weak-comdat-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 3237 bytes --]

On Wed, Aug 12, 2015 at 10:36 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Aug 4, 2015 at 11:43 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>> On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>
>>>>> Hm.  But which options are unsafe?  Also wouldn't it be better to simply
>>>>> _not_ have unsafe options produce comdats but always make local clones
>>>>> for them (thus emit the comdat with "unsafe" flags dropped)?
>>>>
>>>> Always localize comdat functions may lead to text size increase. It
>>>> does not work if the comdat function is a virtual function for
>>>> instance.
>>>
>>> Based on Richard's suggestion, I have a patch to localize comdat
>>> functions which seems like a very effective solution to this problem.
>>> The text size increase is limited to the extra comdat copies generated
>>> for the specialized modules (modules with unsafe options) which is
>>> usually only a few.   Since -fweak does something similar for
>>> functions,  I have called the new option -fweak-comdat-functions.
>>> This does not apply to virtual comdat functions as their addresses can
>>> always be leaked via the vtable. Using this flag with virtual
>>> functions generates a warning.
>>>
>>> To summarize, this is the intended usage of this option. Modules which
>>> use unsafe code options, like -m<isa> for multiversioning, to generate
>>> code that is meant to run only on a subset of CPUs can generate
>>> comdats with specialized instructions which when picked by the linker
>>> can get run unconditionally causing SIGILL on unsupported platforms.
>>> This flag hides these comdats to be local to these modules and not
>>> make them available publicly,  with the caveat that it does not apply
>>> to virtual comdats.
>>>
>>> Could you please review?
>>
>> Ping.  This patch uses Richard's suggestion to localize comdat
>> functions with option -fno-weak-comdat-functions.  Comments?

Ping.

* c-family/c.opt (fweak-comdat-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.

>
>
> Ping.
>
> * c-family/c.opt (fweak-comdat-functions): New option.
> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
> virtual comdat functions are seen.
> * doc/invoke.texi: Document new option.
> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>
>>
>> * c-family/c.opt (fweak-comdat-functions): New option.
>> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
>> virtual comdat functions are seen.
>> * doc/invoke.texi: Document new option.
>> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>>
>>
>>>
>>> * c-family/c.opt (fweak-comdat-functions): New option.
>>> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
>>> virtual comdat functions are seen.
>>> * doc/invoke.texi: Document new option.
>>> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>>>
>>>
>>> Thanks
>>> Sri
>>>
>>>
>>>>
>>>> David
>>>>
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Sri

[-- Attachment #2: no_weak_comdat_functions.txt --]
[-- Type: text/plain, Size: 4095 bytes --]

	* c-family/c.opt (fweak-comdat-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 224486)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,14 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions.  With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 224486)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,19 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak)
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_weak_comdat_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_weak_comdat_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-weak-comdat-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.");
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 224486)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-weak-comdat-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2445,6 +2445,16 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker.  By default, G++ uses weak symbols if they are
+available.  This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally.  This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables.  This can cause unintended behavior if
+the addresses of comdat functions are used.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: testsuite/g++.dg/no-weak-comdat-functions-1.C
===================================================================
--- testsuite/g++.dg/no-weak-comdat-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-weak-comdat-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-weak-comdat-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Cary Coutant]

>>>> Based on Richard's suggestion, I have a patch to localize comdat
>>>> functions which seems like a very effective solution to this problem.
>>>> The text size increase is limited to the extra comdat copies generated
>>>> for the specialized modules (modules with unsafe options) which is
>>>> usually only a few.   Since -fweak does something similar for
>>>> functions,  I have called the new option -fweak-comdat-functions.
>>>> This does not apply to virtual comdat functions as their addresses can
>>>> always be leaked via the vtable. Using this flag with virtual
>>>> functions generates a warning.

+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize
Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions.  With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.

Is one of those "With -fno-weak-comdat-functions" supposed to be "With
-fweak-comdat-functions"? This description doesn't really say what the
flag (without the "no") does, and doesn't explain what "localize"
means.

+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker.  By default, G++ uses weak symbols if they are
+available.  This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally.  This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables.  This can cause unintended behavior if
+the addresses of comdat functions are used.

It's not really the "weak" that is causing the problem -- it's the
"comdat". What the option really is doing is making the functions
static rather than comdat. (It's all gated under flag_weak because
weak symbols are the fall-back to link-once and comdat symbols.) I'd
suggest phrasing this more in terms of static vs. comdat.

This looks like the right way to go, though.

-cary

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Cary Coutant]

> +@item -fno-weak-comdat-functions
> +@opindex fno-weak-comdat-functions
> +Do not use weak symbol support for comdat non-virtual functions, even if it
> +is provided by the linker.  By default, G++ uses weak symbols if they are
> +available.  This option is useful when comdat functions generated in certain
> +compilation units need to be kept local to the respective units and not exposed
> +globally.  This does not apply to virtual comdat functions as their pointers
> +may be taken via virtual tables.  This can cause unintended behavior if
> +the addresses of comdat functions are used.
>
> It's not really the "weak" that is causing the problem -- it's the
> "comdat". What the option really is doing is making the functions
> static rather than comdat. (It's all gated under flag_weak because
> weak symbols are the fall-back to link-once and comdat symbols.) I'd
> suggest phrasing this more in terms of static vs. comdat.

Oh, also, I'd suggest clarifying what you mean by "if the addresses of
comdat functions are used". I think what you really mean here is "if
pointers to the [now non-comdat] functions are compared for equality."

-cary

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Tue, Aug 18, 2015 at 2:14 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>>>> Based on Richard's suggestion, I have a patch to localize comdat
>>>>> functions which seems like a very effective solution to this problem.
>>>>> The text size increase is limited to the extra comdat copies generated
>>>>> for the specialized modules (modules with unsafe options) which is
>>>>> usually only a few.   Since -fweak does something similar for
>>>>> functions,  I have called the new option -fweak-comdat-functions.
>>>>> This does not apply to virtual comdat functions as their addresses can
>>>>> always be leaked via the vtable. Using this flag with virtual
>>>>> functions generates a warning.
>
> +fweak-comdat-functions
> +C++ Var(flag_weak_comdat_functions) Init(1)
> +Specific to comdat functions(-fno-weak-comdat-functions : Localize
> Comdat Functions).
> +With -fno-weak-comdat-functions, virtual comdat functions are still linked as
> +weak functions.  With -fno-weak-comdat-functions, the address of the comdat
> +functions that are localized will be unique and this can cause unintended
> +behavior when addresses of comdat functions are used.
>
> Is one of those "With -fno-weak-comdat-functions" supposed to be "With
> -fweak-comdat-functions"? This description doesn't really say what the
> flag (without the "no") does, and doesn't explain what "localize"
> means.
>
> +@item -fno-weak-comdat-functions
> +@opindex fno-weak-comdat-functions
> +Do not use weak symbol support for comdat non-virtual functions, even if it
> +is provided by the linker.  By default, G++ uses weak symbols if they are
> +available.  This option is useful when comdat functions generated in certain
> +compilation units need to be kept local to the respective units and not exposed
> +globally.  This does not apply to virtual comdat functions as their pointers
> +may be taken via virtual tables.  This can cause unintended behavior if
> +the addresses of comdat functions are used.
>
> It's not really the "weak" that is causing the problem -- it's the
> "comdat". What the option really is doing is making the functions
> static rather than comdat. (It's all gated under flag_weak because
> weak symbols are the fall-back to link-once and comdat symbols.) I'd
> suggest phrasing this more in terms of static vs. comdat.

Thanks, will make those changes.  Do you recommend a different name
for this flag like -fmake-comdat-functions-static?

Sri

>
> This looks like the right way to go, though.
>
> -cary

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Cary Coutant]

> Thanks, will make those changes.  Do you recommend a different name
> for this flag like -fmake-comdat-functions-static?

Well, the C++ ABI refers to this as "vague linkage." It may be a bit
too long or too ABI-specific, but maybe something like
-f[no-]use-vague-linkage-for-functions or
-f[no-]functions-vague-linkage?

And perhaps note in the doc that using this option may technically
break the C++ ODR, so it should be used only when you know what you're
doing.

-cary

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> Thanks, will make those changes.  Do you recommend a different name
>> for this flag like -fmake-comdat-functions-static?
>
> Well, the C++ ABI refers to this as "vague linkage." It may be a bit
> too long or too ABI-specific, but maybe something like
> -f[no-]use-vague-linkage-for-functions or
> -f[no-]functions-vague-linkage?

Done and patch attached.

* c-family/c.opt (fvague-linkage-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* ipa.c (function_and_variable_visibility): Check for no vague
linkage.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.




>
> And perhaps note in the doc that using this option may technically
> break the C++ ODR, so it should be used only when you know what you're
> doing.

Done.

Thanks
Sri

>
> -cary

[-- Attachment #2: no_vague_linkage_functions.txt --]
[-- Type: text/plain, Size: 5028 bytes --]

	* c-family/c.opt (fvague-linkage-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* ipa.c (function_and_variable_visibility): Check for no vague
	linkage.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 227383)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,16 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fvague-linkage-functions
+C++ Var(flag_vague_linkage_functions) Init(1)
+Option -fno-vague-linkage-functions makes comdat functions static and local
+to the module. With -fno-vague-linkage-functions, virtual comdat functions
+still use vague linkage.  With -fno-vague-linkage-functions, the address of
+the comdat functions that are made local will be unique and this can cause
+unintended behavior when addresses of these comdat functions are used in
+comparisons.  This option may technically break the C++ ODR and users of
+this flag should know what they are doing.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 227383)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,22 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_vague_linkage_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      /* Warn when -fno-vague-linkage-functions is used and we found virtual
+	 comdat functions.  Virtual comdat functions must still use vague
+	 linkage.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_vague_linkage_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-vague-linkage-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.", decl);
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 227383)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-vague-linkage-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2448,6 +2448,18 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-vague-linkage-functions
+@opindex fno-vague-linkage-functions
+Do not use vague linkage for comdat non-virtual functions, even if it
+is provided by the linker.  This option is useful when comdat functions
+generated in certain compilation units need to be kept local to the
+respective units and not exposed globally.  This does not apply to virtual
+comdat functions as their pointers may be taken via virtual tables.
+This can cause unintended behavior if the addresses of the comdat functions
+that are made local are used in comparisons, which are not warned about.
+This option may technically break the C++ ODR and users of this flag should
+know what they are doing.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: ipa.c
===================================================================
--- ipa.c	(revision 227383)
+++ ipa.c	(working copy)
@@ -1037,6 +1037,7 @@ function_and_variable_visibility (bool whole_progr
 	}
       gcc_assert ((!DECL_WEAK (node->decl)
 		  && !DECL_COMDAT (node->decl))
+		  || !flag_vague_linkage_functions
       	          || TREE_PUBLIC (node->decl)
 		  || node->weakref
 		  || DECL_EXTERNAL (node->decl));
Index: testsuite/g++.dg/no-vague-linkage-functions-1.C
===================================================================
--- testsuite/g++.dg/no-vague-linkage-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-vague-linkage-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-vague-linkage-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On Wed, Sep 2, 2015 at 4:32 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>
> On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> >> Thanks, will make those changes.  Do you recommend a different name
> >> for this flag like -fmake-comdat-functions-static?
> >
> > Well, the C++ ABI refers to this as "vague linkage." It may be a bit
> > too long or too ABI-specific, but maybe something like
> > -f[no-]use-vague-linkage-for-functions or
> > -f[no-]functions-vague-linkage?
>
> Done and patch attached.


<Re-sending as plain text>
Ping.

* c-family/c.opt (fvague-linkage-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* ipa.c (function_and_variable_visibility): Check for no vague
linkage.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.

>
>
> * c-family/c.opt (fvague-linkage-functions): New option.
> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
> virtual comdat functions are seen.
> * ipa.c (function_and_variable_visibility): Check for no vague
> linkage.
> * doc/invoke.texi: Document new option.
> * testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.
>
>
>
>
> >
> > And perhaps note in the doc that using this option may technically
> > break the C++ ODR, so it should be used only when you know what you're
> > doing.
>
> Done.
>
> Thanks
> Sri
>
> >
> > -cary

[-- Attachment #2: no_vague_linkage_functions.txt --]
[-- Type: text/plain, Size: 5028 bytes --]

	* c-family/c.opt (fvague-linkage-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* ipa.c (function_and_variable_visibility): Check for no vague
	linkage.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 227383)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,16 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fvague-linkage-functions
+C++ Var(flag_vague_linkage_functions) Init(1)
+Option -fno-vague-linkage-functions makes comdat functions static and local
+to the module. With -fno-vague-linkage-functions, virtual comdat functions
+still use vague linkage.  With -fno-vague-linkage-functions, the address of
+the comdat functions that are made local will be unique and this can cause
+unintended behavior when addresses of these comdat functions are used in
+comparisons.  This option may technically break the C++ ODR and users of
+this flag should know what they are doing.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 227383)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,22 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_vague_linkage_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      /* Warn when -fno-vague-linkage-functions is used and we found virtual
+	 comdat functions.  Virtual comdat functions must still use vague
+	 linkage.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_vague_linkage_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-vague-linkage-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.", decl);
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 227383)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-vague-linkage-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2448,6 +2448,18 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-vague-linkage-functions
+@opindex fno-vague-linkage-functions
+Do not use vague linkage for comdat non-virtual functions, even if it
+is provided by the linker.  This option is useful when comdat functions
+generated in certain compilation units need to be kept local to the
+respective units and not exposed globally.  This does not apply to virtual
+comdat functions as their pointers may be taken via virtual tables.
+This can cause unintended behavior if the addresses of the comdat functions
+that are made local are used in comparisons, which are not warned about.
+This option may technically break the C++ ODR and users of this flag should
+know what they are doing.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: ipa.c
===================================================================
--- ipa.c	(revision 227383)
+++ ipa.c	(working copy)
@@ -1037,6 +1037,7 @@ function_and_variable_visibility (bool whole_progr
 	}
       gcc_assert ((!DECL_WEAK (node->decl)
 		  && !DECL_COMDAT (node->decl))
+		  || !flag_vague_linkage_functions
       	          || TREE_PUBLIC (node->decl)
 		  || node->weakref
 		  || DECL_EXTERNAL (node->decl));
Index: testsuite/g++.dg/no-vague-linkage-functions-1.C
===================================================================
--- testsuite/g++.dg/no-vague-linkage-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-vague-linkage-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-vague-linkage-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Wed, Sep 9, 2015 at 4:01 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Wed, Sep 2, 2015 at 4:32 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>
>> On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> >> Thanks, will make those changes.  Do you recommend a different name
>> >> for this flag like -fmake-comdat-functions-static?
>> >
>> > Well, the C++ ABI refers to this as "vague linkage." It may be a bit
>> > too long or too ABI-specific, but maybe something like
>> > -f[no-]use-vague-linkage-for-functions or
>> > -f[no-]functions-vague-linkage?
>>
>> Done and patch attached.
>
>
> <Re-sending as plain text>
> Ping.

Ping.


>
> * c-family/c.opt (fvague-linkage-functions): New option.
> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
> virtual comdat functions are seen.
> * ipa.c (function_and_variable_visibility): Check for no vague
> linkage.
> * doc/invoke.texi: Document new option.
> * testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.
>
>>
>>
>> * c-family/c.opt (fvague-linkage-functions): New option.
>> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
>> virtual comdat functions are seen.
>> * ipa.c (function_and_variable_visibility): Check for no vague
>> linkage.
>> * doc/invoke.texi: Document new option.
>> * testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.
>>
>>
>>
>>
>> >
>> > And perhaps note in the doc that using this option may technically
>> > break the C++ ODR, so it should be used only when you know what you're
>> > doing.
>>
>> Done.
>>
>> Thanks
>> Sri
>>
>> >
>> > -cary

[-- Attachment #2: no_vague_linkage_functions.txt --]
[-- Type: text/plain, Size: 5028 bytes --]

	* c-family/c.opt (fvague-linkage-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* ipa.c (function_and_variable_visibility): Check for no vague
	linkage.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 227383)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,16 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fvague-linkage-functions
+C++ Var(flag_vague_linkage_functions) Init(1)
+Option -fno-vague-linkage-functions makes comdat functions static and local
+to the module. With -fno-vague-linkage-functions, virtual comdat functions
+still use vague linkage.  With -fno-vague-linkage-functions, the address of
+the comdat functions that are made local will be unique and this can cause
+unintended behavior when addresses of these comdat functions are used in
+comparisons.  This option may technically break the C++ ODR and users of
+this flag should know what they are doing.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 227383)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,22 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_vague_linkage_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      /* Warn when -fno-vague-linkage-functions is used and we found virtual
+	 comdat functions.  Virtual comdat functions must still use vague
+	 linkage.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_vague_linkage_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-vague-linkage-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.", decl);
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 227383)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-vague-linkage-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2448,6 +2448,18 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-vague-linkage-functions
+@opindex fno-vague-linkage-functions
+Do not use vague linkage for comdat non-virtual functions, even if it
+is provided by the linker.  This option is useful when comdat functions
+generated in certain compilation units need to be kept local to the
+respective units and not exposed globally.  This does not apply to virtual
+comdat functions as their pointers may be taken via virtual tables.
+This can cause unintended behavior if the addresses of the comdat functions
+that are made local are used in comparisons, which are not warned about.
+This option may technically break the C++ ODR and users of this flag should
+know what they are doing.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: ipa.c
===================================================================
--- ipa.c	(revision 227383)
+++ ipa.c	(working copy)
@@ -1037,6 +1037,7 @@ function_and_variable_visibility (bool whole_progr
 	}
       gcc_assert ((!DECL_WEAK (node->decl)
 		  && !DECL_COMDAT (node->decl))
+		  || !flag_vague_linkage_functions
       	          || TREE_PUBLIC (node->decl)
 		  || node->weakref
 		  || DECL_EXTERNAL (node->decl));
Index: testsuite/g++.dg/no-vague-linkage-functions-1.C
===================================================================
--- testsuite/g++.dg/no-vague-linkage-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-vague-linkage-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-vague-linkage-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

On Mon, Oct 5, 2015 at 2:58 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Wed, Sep 9, 2015 at 4:01 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> On Wed, Sep 2, 2015 at 4:32 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>
>>> On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>> >> Thanks, will make those changes.  Do you recommend a different name
>>> >> for this flag like -fmake-comdat-functions-static?
>>> >
>>> > Well, the C++ ABI refers to this as "vague linkage." It may be a bit
>>> > too long or too ABI-specific, but maybe something like
>>> > -f[no-]use-vague-linkage-for-functions or
>>> > -f[no-]functions-vague-linkage?
>>>
>>> Done and patch attached.
>>
>>
>> <Re-sending as plain text>
>> Ping.

Forgot to follow up on this one but is patch of for trunk?

* c-family/c.opt (fvague-linkage-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
virtual comdat functions are seen.
* ipa.c (function_and_variable_visibility): Check for no vague
linkage.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.


Thanks
Sri

>
> Ping.
>
>
>>
>> * c-family/c.opt (fvague-linkage-functions): New option.
>> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
>> virtual comdat functions are seen.
>> * ipa.c (function_and_variable_visibility): Check for no vague
>> linkage.
>> * doc/invoke.texi: Document new option.
>> * testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.
>>
>>>
>>>
>>> * c-family/c.opt (fvague-linkage-functions): New option.
>>> * cp/decl2.c (comdat_linkage): Implement new option.  Warn when
>>> virtual comdat functions are seen.
>>> * ipa.c (function_and_variable_visibility): Check for no vague
>>> linkage.
>>> * doc/invoke.texi: Document new option.
>>> * testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.
>>>
>>>
>>>
>>>
>>> >
>>> > And perhaps note in the doc that using this option may technically
>>> > break the C++ ODR, so it should be used only when you know what you're
>>> > doing.
>>>
>>> Done.
>>>
>>> Thanks
>>> Sri
>>>
>>> >
>>> > -cary

[-- Attachment #2: no_vague_linkage_functions.txt --]
[-- Type: text/plain, Size: 5028 bytes --]

	* c-family/c.opt (fvague-linkage-functions): New option.
	* cp/decl2.c (comdat_linkage): Implement new option.  Warn when
	virtual comdat functions are seen.
	* ipa.c (function_and_variable_visibility): Check for no vague
	linkage.
	* doc/invoke.texi: Document new option.
	* testsuite/g++.dg/no-vague-linkage-functions-1.C: New test.

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 227383)
+++ c-family/c.opt	(working copy)
@@ -1236,6 +1236,16 @@ fweak
 C++ ObjC++ Var(flag_weak) Init(1)
 Emit common-like symbols as weak symbols
 
+fvague-linkage-functions
+C++ Var(flag_vague_linkage_functions) Init(1)
+Option -fno-vague-linkage-functions makes comdat functions static and local
+to the module. With -fno-vague-linkage-functions, virtual comdat functions
+still use vague linkage.  With -fno-vague-linkage-functions, the address of
+the comdat functions that are made local will be unique and this can cause
+unintended behavior when addresses of these comdat functions are used in
+comparisons.  This option may technically break the C++ ODR and users of
+this flag should know what they are doing.
+
 fwide-exec-charset=
 C ObjC C++ ObjC++ Joined RejectNegative
 -fwide-exec-charset=<cset>	Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 227383)
+++ cp/decl2.c	(working copy)
@@ -1702,8 +1702,22 @@ mark_vtable_entries (tree decl)
 void
 comdat_linkage (tree decl)
 {
-  if (flag_weak
-    make_decl_one_only (decl, cxx_comdat_group (decl));
+  if (flag_weak
+      && (flag_vague_linkage_functions
+	  || TREE_CODE (decl) != FUNCTION_DECL
+	  || DECL_VIRTUAL_P (decl)))
+    {
+      make_decl_one_only (decl, cxx_comdat_group (decl));
+      /* Warn when -fno-vague-linkage-functions is used and we found virtual
+	 comdat functions.  Virtual comdat functions must still use vague
+	 linkage.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_VIRTUAL_P (decl)
+	  && !flag_vague_linkage_functions)
+	warning_at (DECL_SOURCE_LOCATION (decl), 0,
+		    "fno-vague-linkage-functions: Comdat linkage of virtual "
+		    "function %q#D preserved.", decl);
+    }
   else if (TREE_CODE (decl) == FUNCTION_DECL
 	   || (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
     /* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 227383)
+++ doc/invoke.texi	(working copy)
@@ -189,7 +189,7 @@ in the following sections.
 -fno-pretty-templates @gol
 -frepo  -fno-rtti  -fstats  -ftemplate-backtrace-limit=@var{n} @gol
 -ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak  -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit  -fno-weak -fno-vague-linkage-functions -nostdinc++ @gol
 -fvisibility-inlines-hidden @gol
 -fvtable-verify=@var{std|preinit|none} @gol
 -fvtv-counts -fvtv-debug @gol
@@ -2448,6 +2448,18 @@ option exists only for testing, and should not be
 it results in inferior code and has no benefits.  This option may
 be removed in a future release of G++.
 
+@item -fno-vague-linkage-functions
+@opindex fno-vague-linkage-functions
+Do not use vague linkage for comdat non-virtual functions, even if it
+is provided by the linker.  This option is useful when comdat functions
+generated in certain compilation units need to be kept local to the
+respective units and not exposed globally.  This does not apply to virtual
+comdat functions as their pointers may be taken via virtual tables.
+This can cause unintended behavior if the addresses of the comdat functions
+that are made local are used in comparisons, which are not warned about.
+This option may technically break the C++ ODR and users of this flag should
+know what they are doing.
+
 @item -nostdinc++
 @opindex nostdinc++
 Do not search for header files in the standard directories specific to
Index: ipa.c
===================================================================
--- ipa.c	(revision 227383)
+++ ipa.c	(working copy)
@@ -1037,6 +1037,7 @@ function_and_variable_visibility (bool whole_progr
 	}
       gcc_assert ((!DECL_WEAK (node->decl)
 		  && !DECL_COMDAT (node->decl))
+		  || !flag_vague_linkage_functions
       	          || TREE_PUBLIC (node->decl)
 		  || node->weakref
 		  || DECL_EXTERNAL (node->decl));
Index: testsuite/g++.dg/no-vague-linkage-functions-1.C
===================================================================
--- testsuite/g++.dg/no-vague-linkage-functions-1.C	(revision 0)
+++ testsuite/g++.dg/no-vague-linkage-functions-1.C	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-vague-linkage-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+  return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */