Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam <tmsriram@google.com>]

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