From: Sriraman Tallam <tmsriram@google.com>
To: Xinliang David Li <davidxl@google.com>
Cc: Richard Biener <richard.guenther@gmail.com>,
"H.J. Lu" <hjl.tools@gmail.com>,
GCC Patches <gcc-patches@gcc.gnu.org>,
binutils <binutils@sourceware.org>,
Cary Coutant <ccoutant@gmail.com>,
Yury Gribov <y.gribov@samsung.com>
Subject: Re: [RFC] COMDAT Safe Module Level Multi versioning
Date: Tue, 04 Aug 2015 18:43:00 -0000 [thread overview]
Message-ID: <CAAs8HmwfrRfVuSU4iorW7ETscxxf4WqGx1qr+9+VeZ0Fe7xQKQ@mail.gmail.com> (raw)
In-Reply-To: <CAAs8HmzY-YPt2bE6ce7S68Uh42KvfOZfLyMzT-N8JTFMbu9yPg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]
On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>
>>> Hm. But which options are unsafe? Also wouldn't it be better to simply
>>> _not_ have unsafe options produce comdats but always make local clones
>>> for them (thus emit the comdat with "unsafe" flags dropped)?
>>
>> Always localize comdat functions may lead to text size increase. It
>> does not work if the comdat function is a virtual function for
>> instance.
>
> Based on Richard's suggestion, I have a patch to localize comdat
> functions which seems like a very effective solution to this problem.
> The text size increase is limited to the extra comdat copies generated
> for the specialized modules (modules with unsafe options) which is
> usually only a few. Since -fweak does something similar for
> functions, I have called the new option -fweak-comdat-functions.
> This does not apply to virtual comdat functions as their addresses can
> always be leaked via the vtable. Using this flag with virtual
> functions generates a warning.
>
> To summarize, this is the intended usage of this option. Modules which
> use unsafe code options, like -m<isa> for multiversioning, to generate
> code that is meant to run only on a subset of CPUs can generate
> comdats with specialized instructions which when picked by the linker
> can get run unconditionally causing SIGILL on unsupported platforms.
> This flag hides these comdats to be local to these modules and not
> make them available publicly, with the caveat that it does not apply
> to virtual comdats.
>
> Could you please review?
Ping. This patch uses Richard's suggestion to localize comdat
functions with option -fno-weak-comdat-functions. Comments?
* c-family/c.opt (fweak-comdat-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option. Warn when
virtual comdat functions are seen.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>
> * c-family/c.opt (fweak-comdat-functions): New option.
> * cp/decl2.c (comdat_linkage): Implement new option. Warn when
> virtual comdat functions are seen.
> * doc/invoke.texi: Document new option.
> * testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
>
>
> Thanks
> Sri
>
>
>>
>> David
>>
>>
>>>
>>> Richard.
>>>
>>>>
>>>> Thanks
>>>> Sri
[-- Attachment #2: no_weak_comdat_functions.txt --]
[-- Type: text/plain, Size: 4095 bytes --]
* c-family/c.opt (fweak-comdat-functions): New option.
* cp/decl2.c (comdat_linkage): Implement new option. Warn when
virtual comdat functions are seen.
* doc/invoke.texi: Document new option.
* testsuite/g++.dg/no-weak-comdat-functions-1.C: New test.
Index: c-family/c.opt
===================================================================
--- c-family/c.opt (revision 224486)
+++ c-family/c.opt (working copy)
@@ -1236,6 +1236,14 @@ fweak
C++ ObjC++ Var(flag_weak) Init(1)
Emit common-like symbols as weak symbols
+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions. With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.
+
fwide-exec-charset=
C ObjC C++ ObjC++ Joined RejectNegative
-fwide-exec-charset=<cset> Convert all wide strings and character constants to character set <cset>
Index: cp/decl2.c
===================================================================
--- cp/decl2.c (revision 224486)
+++ cp/decl2.c (working copy)
@@ -1702,8 +1702,19 @@ mark_vtable_entries (tree decl)
void
comdat_linkage (tree decl)
{
- if (flag_weak)
- make_decl_one_only (decl, cxx_comdat_group (decl));
+ if (flag_weak
+ && (flag_weak_comdat_functions
+ || TREE_CODE (decl) != FUNCTION_DECL
+ || DECL_VIRTUAL_P (decl)))
+ {
+ make_decl_one_only (decl, cxx_comdat_group (decl));
+ if (TREE_CODE (decl) == FUNCTION_DECL
+ && DECL_VIRTUAL_P (decl)
+ && !flag_weak_comdat_functions)
+ warning_at (DECL_SOURCE_LOCATION (decl), 0,
+ "fno-weak-comdat-functions: Comdat linkage of virtual "
+ "function %q#D preserved.");
+ }
else if (TREE_CODE (decl) == FUNCTION_DECL
|| (VAR_P (decl) && DECL_ARTIFICIAL (decl)))
/* We can just emit function and compiler-generated variables
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 224486)
+++ doc/invoke.texi (working copy)
@@ -189,7 +189,7 @@ in the following sections.
-fno-pretty-templates @gol
-frepo -fno-rtti -fstats -ftemplate-backtrace-limit=@var{n} @gol
-ftemplate-depth=@var{n} @gol
--fno-threadsafe-statics -fuse-cxa-atexit -fno-weak -nostdinc++ @gol
+-fno-threadsafe-statics -fuse-cxa-atexit -fno-weak -fno-weak-comdat-functions -nostdinc++ @gol
-fvisibility-inlines-hidden @gol
-fvtable-verify=@var{std|preinit|none} @gol
-fvtv-counts -fvtv-debug @gol
@@ -2445,6 +2445,16 @@ option exists only for testing, and should not be
it results in inferior code and has no benefits. This option may
be removed in a future release of G++.
+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker. By default, G++ uses weak symbols if they are
+available. This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally. This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables. This can cause unintended behavior if
+the addresses of comdat functions are used.
+
@item -nostdinc++
@opindex nostdinc++
Do not search for header files in the standard directories specific to
Index: testsuite/g++.dg/no-weak-comdat-functions-1.C
===================================================================
--- testsuite/g++.dg/no-weak-comdat-functions-1.C (revision 0)
+++ testsuite/g++.dg/no-weak-comdat-functions-1.C (working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-weak-comdat-functions -fkeep-inline-functions -ffunction-sections" } */
+
+inline int foo () {
+ return 0;
+}
+
+/* { dg-final { scan-assembler "_Z3foov" } } */
+/* { dg-final { scan-assembler-not "_Z3foov.*comdat" } } */
next prev parent reply other threads:[~2015-08-04 18:43 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-19 6:30 Sriraman Tallam
2015-05-19 9:39 ` Richard Biener
2015-05-19 16:12 ` Xinliang David Li
2015-06-17 2:18 ` Sriraman Tallam
2015-08-04 18:43 ` Sriraman Tallam [this message]
2015-08-13 6:17 ` Sriraman Tallam
2015-08-18 18:46 ` Sriraman Tallam
2015-08-18 21:19 ` Cary Coutant
2015-08-18 21:25 ` Cary Coutant
2015-08-18 21:44 ` Sriraman Tallam
2015-08-19 4:53 ` Cary Coutant
2015-09-03 0:51 ` Sriraman Tallam
2015-09-09 23:27 ` Sriraman Tallam
2015-10-05 21:58 ` Sriraman Tallam
2016-09-12 19:30 ` Sriraman Tallam
2015-05-19 17:22 ` Sriraman Tallam
2015-05-19 17:28 ` Yury Gribov
2015-05-19 18:18 ` Sriraman Tallam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAs8HmwfrRfVuSU4iorW7ETscxxf4WqGx1qr+9+VeZ0Fe7xQKQ@mail.gmail.com \
--to=tmsriram@google.com \
--cc=binutils@sourceware.org \
--cc=ccoutant@gmail.com \
--cc=davidxl@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
--cc=richard.guenther@gmail.com \
--cc=y.gribov@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).