Message ID | CAAs8HmyVjF+cG10euUFNMZoLP6nV4V8nvqmCbNwSMgJr926C-Q@mail.gmail.com |
---|---|
State | New |
Headers | show |
>>>> 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
> +@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
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
> 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
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" } } */