Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam <tmsriram@google.com>]

[-- 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" } } */