Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Mon, Oct 5, 2015 at 2:58 PM, Sriraman Tallam  wrote:
> On Wed, Sep 9, 2015 at 4:01 PM, Sriraman Tallam  wrote:
>> On Wed, Sep 2, 2015 at 4:32 PM, Sriraman Tallam  wrote:
>>>
>>> On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant  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.
>>
>>
>> 
>> 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
* 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= Convert all wide strings and character 
constants to character set 
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  

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Wed, Sep 9, 2015 at 4:01 PM, Sriraman Tallam  wrote:
> On Wed, Sep 2, 2015 at 4:32 PM, Sriraman Tallam  wrote:
>>
>> On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant  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.
>
>
> 
> 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
* 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= Convert all wide strings and character 
constants to character set 
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

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Wed, Sep 2, 2015 at 4:32 PM, Sriraman Tallam  wrote:
>
> On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant  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.



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
* 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= Convert all wide strings and character 
constants to character set 
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 

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Tue, Aug 18, 2015 at 9:46 PM, Cary Coutant  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
* 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= Convert all wide strings and character 
constants to character set 
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

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Wed, Aug 12, 2015 at 10:36 PM, Sriraman Tallam tmsri...@google.com wrote:
 On Tue, Aug 4, 2015 at 11:43 AM, Sriraman Tallam tmsri...@google.com wrote:
 On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam tmsri...@google.com wrote:
 On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li davi...@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 -misa 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
* 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.);

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

[Sriraman Tallam]

On Tue, Aug 18, 2015 at 2:14 PM, Cary Coutant ccout...@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

[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 4, 2015 at 11:43 AM, Sriraman Tallam tmsri...@google.com wrote:
 On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam tmsri...@google.com wrote:
 On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li davi...@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 -misa 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
* 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)

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam tmsri...@google.com wrote:
 On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li davi...@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 -misa 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
* 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
 

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li davi...@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 -misa 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
* 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++ 

[RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam]

We have the following problem with selectively compiling modules with
-misa 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 -misa 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:

templatetypename 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
-misa.  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 tmsri...@google.com wrote:
 We have the following problem with selectively compiling modules with
 -misa 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 -misa 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:

 templatetypename 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
 -misa.  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

[Sriraman Tallam]

On Tue, May 19, 2015 at 2:39 AM, Richard Biener
richard.guent...@gmail.com wrote:
 On Tue, May 19, 2015 at 8:16 AM, Sriraman Tallam tmsri...@google.com wrote:
 We have the following problem with selectively compiling modules with
 -misa 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 -misa 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:

 templatetypename 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
 -misa.  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

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

[Yury Gribov]

On 05/19/2015 09:16 AM, Sriraman Tallam wrote:

We have the following problem with selectively compiling modules with
-misa 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 -misa 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:

templatetypename 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
-misa.  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.gri...@samsung.com wrote:
 On 05/19/2015 09:16 AM, Sriraman Tallam wrote:

 We have the following problem with selectively compiling modules with
 -misa 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 -misa 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:

 templatetypename 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
 -misa.  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