From patchwork Tue Aug 18 18:38:26 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sriraman Tallam X-Patchwork-Id: 508422 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 77AB614032E for ; Wed, 19 Aug 2015 04:38:41 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=yz1ClGYG; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=xrDflWpDdQoDu7KjwH l6TAKJzWL99Y5VscF8AF1tP9FJ5G+alvC7xdMQw71up01OVq6gp7h+PI5hnnFami z5FmJ+1kGqZPi44sc+1g4GWsn31wGKBdmoR7IsXpMvkWl2YkrO+N3T2lq+GULAEV H91M+sJ9V6Bskw51ee+zm8nXg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=CBQSK79Ls8ihfCaF0RSKyYGK 7nE=; b=yz1ClGYGnHGau+OgwG/gin9yzr2X7RZsnI8Leky1gfEr4oOJ/Q8Ovlj5 lNvBhpI9D1jMgVCFNBYWjt93vf6qlT2LjcnEGR6FE9oHHVIeBrlzPGu8Ns5NbF3+ b7zgg1QmOIi9I1jpVhl3fMv4mEROmR0MlwZ5cMBh7veGVeZnRFs= Received: (qmail 16523 invoked by alias); 18 Aug 2015 18:38:32 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 16506 invoked by uid 89); 18 Aug 2015 18:38:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f169.google.com Received: from mail-wi0-f169.google.com (HELO mail-wi0-f169.google.com) (209.85.212.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 18 Aug 2015 18:38:30 +0000 Received: by wibhh20 with SMTP id hh20so116474870wib.0 for ; Tue, 18 Aug 2015 11:38:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=Haqb+GA0CeUiFaQEkoHNpEZpg8KcN9j9BcAUATeqN7I=; b=J4Ch1X027HTBD2a0YcTD05mlo1o0VAv9+QEMxFSc8Mcs/rqmDjgNnM027AMoaRqPXX BGnI7C2PonMK5pQI1uWkjsWAekWAKjMzcCyF/YHMJFAsE7py9EZ+w6ZKGxFdBBGU25lP dF83suCo4YxbSDjFHj2HWzlRDv9nnZBg+V3k0NdjQMcOJC44FqHrc/BNxLpFNWYXW29i +vCgccusWQabW90f+MaA0o8Z6ZA2zkO1QAIeDhv8+k5k945x+1onUphrJUpGApEHKn2C WR7gsy5oeDgXLQOgHZcPf4JVmJDTKZtxwtbi7yuQn8CkxoT1AWej51RDfoQCT1WuP6Jf ESkg== X-Gm-Message-State: ALoCoQkthM2CO+f9lN5D7shQ9RC3CCO4pGzlXcgfsP4M9ROGDpa/92ENS/5bQwyiFJrIhAg9NAye MIME-Version: 1.0 X-Received: by 10.180.205.148 with SMTP id lg20mr47029778wic.60.1439923106864; Tue, 18 Aug 2015 11:38:26 -0700 (PDT) Received: by 10.28.46.3 with HTTP; Tue, 18 Aug 2015 11:38:26 -0700 (PDT) In-Reply-To: References: Date: Tue, 18 Aug 2015 11:38:26 -0700 Message-ID: Subject: Re: [RFC] COMDAT Safe Module Level Multi versioning From: Sriraman Tallam To: Xinliang David Li Cc: Richard Biener , "H.J. Lu" , GCC Patches , binutils , Cary Coutant , Yury Gribov X-IsSubscribed: yes On Wed, Aug 12, 2015 at 10:36 PM, Sriraman Tallam wrote: > On Tue, Aug 4, 2015 at 11:43 AM, Sriraman Tallam wrote: >> On Tue, Jun 16, 2015 at 4:22 PM, Sriraman Tallam wrote: >>> On Tue, May 19, 2015 at 9:11 AM, Xinliang David Li 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 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. 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= Convert all wide strings and character constants to character set 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" } } */