diff mbox

[RFC] COMDAT Safe Module Level Multi versioning

Message ID CAAs8HmyVjF+cG10euUFNMZoLP6nV4V8nvqmCbNwSMgJr926C-Q@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam Aug. 18, 2015, 6:38 p.m. UTC 
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
* 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.

Comments

Cary Coutant Aug. 18, 2015, 9:14 p.m. UTC | #1 
>>>> 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
Cary Coutant Aug. 18, 2015, 9:24 p.m. UTC | #2 
> +@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
Sriraman Tallam Aug. 18, 2015, 9:33 p.m. UTC | #3 
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
Cary Coutant Aug. 19, 2015, 4:46 a.m. UTC | #4 
> 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
diff mbox

Patch

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