From patchwork Mon Nov 17 13:34:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Enkovich X-Patchwork-Id: 411680 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 8ABDF14012B for ; Tue, 18 Nov 2014 00:34:40 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=GXZc1KkEPMDL6hmCk Q+R9MtX56j4lIWh8INLPhAqKcjLY9u8Xs6J5/VsGAXxp7fcM7Z7MdznlWMHfVEbA WlazIqJGIaV0c6kPYgnfVYMsTLq5IiSvOENFdJUdcYb+bSEzkgkKlNmtr6uMZJPb T74246Ago9/MP0lk7qM9pQrWe4= 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:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=nkxRTjRoqvzfju4RyhmT+13 FPwM=; b=enHJ8BBCrSjON8ed8/yYJPefRsgllqXF2E9A3+4+xAPQIdpXMj81soe IT8oYq8X1tYTs7PlqKsr1ZhqKP40VQm1l1e76kpkgwehpYCS2QKK3Z7TcZoqtNqV jzFOpD9rIJr8YO5kA+0LI7WwQ7U7lFqzRdJrFcaaNGOxn09ZZMNU= Received: (qmail 20735 invoked by alias); 17 Nov 2014 13:34:33 -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 20724 invoked by uid 89); 17 Nov 2014 13:34:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f48.google.com Received: from mail-pa0-f48.google.com (HELO mail-pa0-f48.google.com) (209.85.220.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 17 Nov 2014 13:34:31 +0000 Received: by mail-pa0-f48.google.com with SMTP id rd3so8639970pab.35 for ; Mon, 17 Nov 2014 05:34:29 -0800 (PST) X-Received: by 10.70.138.37 with SMTP id qn5mr3091311pdb.118.1416231269441; Mon, 17 Nov 2014 05:34:29 -0800 (PST) Received: from msticlxl57.ims.intel.com ([192.55.54.40]) by mx.google.com with ESMTPSA id ba7sm35152935pdb.87.2014.11.17.05.34.26 for (version=TLSv1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 17 Nov 2014 05:34:28 -0800 (PST) Date: Mon, 17 Nov 2014 16:34:22 +0300 From: Ilya Enkovich To: Jeff Law Cc: gcc-patches Subject: Re: [PATCH, Pointer Bounds Checker, Builtins instrumentation 2/5] Instrument builtin calls Message-ID: <20141117133422.GB5422@msticlxl57.ims.intel.com> References: <20141106121041.GA44122@msticlxl57.ims.intel.com> <5465A614.2040707@redhat.com> <5466F99E.4040702@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5466F99E.4040702@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes On 14 Nov 23:58, Jeff Law wrote: > On 11/14/14 01:06, Ilya Enkovich wrote: > > >>>- /* Avoid instrumented builtin functions for now. Due to IPA > >>>- it also means we have to avoid instrumentation of indirect > >>>- calls. */ > >>>- if (fndecl && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN) > >>>- return; > >>>+ /* We instrument only some subset of builtins. We also instrument > >>>+ builtin calls to be inlined. */ > >>>+ if (fndecl > >>>+ && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL > >>>+ && !chkp_instrument_normal_builtin (fndecl)) > >>>+ { > >>>+ struct cgraph_node *clone = chkp_maybe_create_clone (fndecl); > >>>+ if (!clone > >>>+ || !gimple_has_body_p (clone->decl) > >>>+ || !lookup_attribute ("always_inline", DECL_ATTRIBUTES > >>>(fndecl))) > >>>+ return; > >>>+ } > >> > >>Is that outer conditional right? If we have a fndecl and it's a normal > >>builtin, but it's _not_ one of hte builtins we're instrumenting, then call > >>chkp_maybe_create_clone? > > > >Some builtin functions (especially their *_chk version) are defined as > >always_inline functions in headers. In this case we handle them as > >regular functions (clone and instrument) because they will be inlined > >anyway. Seems gimple_has_body_p should be applied to fndecl and moved > >into outer if-statement along with attribute check. Thus unneeded > >clones would be avoided. > So the outer condition is, essentially checking for a _chk builtin > and if we find it, try to create a clone for instrumentation > purposes. > > I think the always_inline attribute lookup can move to the outer-if. > Less sure about the gimple_has_body check though. Presumably > clone->decl is going to refer to fndecl? If so, then that can move > to the outer if as well. As you say, that may cut down on the > number of clones that get created. > > OK as-is, or with those two conditions moved out of level as long as > all prerequisites are approved and it passes a bootstrap & > regression test. > > JEff I moved always_inline check up. Here is a final version. Thanks, Ilya diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c index 3e343d4..46b2139 100644 --- a/gcc/ipa-chkp.c +++ b/gcc/ipa-chkp.c @@ -581,8 +581,9 @@ chkp_versioning (void) && (!flag_chkp_instrument_marked_only || lookup_attribute ("bnd_instrument", DECL_ATTRIBUTES (node->decl))) - /* No builtins instrumentation for now. */ - && DECL_BUILT_IN_CLASS (node->decl) == NOT_BUILT_IN) + && (!DECL_BUILT_IN (node->decl) + || (DECL_BUILT_IN_CLASS (node->decl) == BUILT_IN_NORMAL + && DECL_FUNCTION_CODE (node->decl) < BEGIN_CHKP_BUILTINS))) chkp_maybe_create_clone (node->decl); } diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index df7d425..0fb78cc 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -1586,6 +1586,50 @@ chkp_find_bound_slots (const_tree type, bitmap res) chkp_find_bound_slots_1 (type, res, 0); } +/* Return 1 if call to FNDECL should be instrumented + and 0 otherwise. */ + +static bool +chkp_instrument_normal_builtin (tree fndecl) +{ + switch (DECL_FUNCTION_CODE (fndecl)) + { + case BUILT_IN_STRLEN: + case BUILT_IN_STRCPY: + case BUILT_IN_STRNCPY: + case BUILT_IN_STPCPY: + case BUILT_IN_STPNCPY: + case BUILT_IN_STRCAT: + case BUILT_IN_STRNCAT: + case BUILT_IN_MEMCPY: + case BUILT_IN_MEMPCPY: + case BUILT_IN_MEMSET: + case BUILT_IN_MEMMOVE: + case BUILT_IN_BZERO: + case BUILT_IN_STRCMP: + case BUILT_IN_STRNCMP: + case BUILT_IN_BCMP: + case BUILT_IN_MEMCMP: + case BUILT_IN_MEMCPY_CHK: + case BUILT_IN_MEMPCPY_CHK: + case BUILT_IN_MEMMOVE_CHK: + case BUILT_IN_MEMSET_CHK: + case BUILT_IN_STRCPY_CHK: + case BUILT_IN_STRNCPY_CHK: + case BUILT_IN_STPCPY_CHK: + case BUILT_IN_STPNCPY_CHK: + case BUILT_IN_STRCAT_CHK: + case BUILT_IN_STRNCAT_CHK: + case BUILT_IN_MALLOC: + case BUILT_IN_CALLOC: + case BUILT_IN_REALLOC: + return 1; + + default: + return 0; + } +} + /* Add bound arguments to call statement pointed by GSI. Also performs a replacement of user checker builtins calls with internal ones. */ @@ -1619,7 +1663,7 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_OBJECT_SIZE) return; - /* Donothing for calls to legacy functions. */ + /* Do nothing for calls to legacy functions. */ if (fndecl && lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (fndecl))) return; @@ -1686,11 +1730,20 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) if (!flag_chkp_instrument_calls) return; - /* Avoid instrumented builtin functions for now. Due to IPA - it also means we have to avoid instrumentation of indirect - calls. */ - if (fndecl && DECL_BUILT_IN_CLASS (fndecl) != NOT_BUILT_IN) - return; + /* We instrument only some subset of builtins. We also instrument + builtin calls to be inlined. */ + if (fndecl + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL + && !chkp_instrument_normal_builtin (fndecl)) + { + if (!lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl))) + return; + + struct cgraph_node *clone = chkp_maybe_create_clone (fndecl); + if (!clone + || !gimple_has_body_p (clone->decl)) + return; + } /* If function decl is available then use it for formal arguments list. Otherwise use function type. */ @@ -1764,14 +1817,6 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi) } new_args.release (); - /* If we call built-in function and pass no bounds then - we do not need to change anything. */ - if (new_call == call - && fndecl - && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL - && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE (fndecl))) - return; - /* For direct calls fndecl is replaced with instrumented version. */ if (fndecl) { @@ -3905,15 +3950,21 @@ chkp_replace_function_pointer (tree *op, int *walk_subtrees, { if (TREE_CODE (*op) == FUNCTION_DECL && !lookup_attribute ("bnd_legacy", DECL_ATTRIBUTES (*op)) - /* Do not replace builtins for now. */ - && DECL_BUILT_IN_CLASS (*op) == NOT_BUILT_IN) + && (DECL_BUILT_IN_CLASS (*op) == NOT_BUILT_IN + /* For builtins we replace pointers only for selected + function and functions having definitions. */ + || (DECL_BUILT_IN_CLASS (*op) == BUILT_IN_NORMAL + && (chkp_instrument_normal_builtin (*op) + || gimple_has_body_p (*op))))) { struct cgraph_node *node = cgraph_node::get_create (*op); + struct cgraph_node *clone = NULL; if (!node->instrumentation_clone) - chkp_maybe_create_clone (*op); + clone = chkp_maybe_create_clone (*op); - *op = node->instrumented_version->decl; + if (clone) + *op = clone->decl; *walk_subtrees = 0; }