From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5303 invoked by alias); 7 Nov 2012 01:18:18 -0000 Received: (qmail 5294 invoked by uid 22791); 7 Nov 2012 01:18:17 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-qc0-f175.google.com (HELO mail-qc0-f175.google.com) (209.85.216.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Nov 2012 01:18:11 +0000 Received: by mail-qc0-f175.google.com with SMTP id j3so756873qcs.20 for ; Tue, 06 Nov 2012 17:18:10 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding:x-gm-message-state; bh=hN/9zKY00f60mCmTnf67YW1P1ufWGWAl0gx/FNzx8Oc=; b=fHtRcB+Z4xsh1W1guDGNxZAjTjwi7TSKR5D1CmorLTiFPB28bxawny1j6ZGpzshz/d lYIRziXWg+PB3kc5ADNlSNWliEVCThUj1ILHMB1xsAPP4Fx73MTH9T0bZddAG2azcZjy /c2K9PoA4U3CDcE/swwfZoqYvH4gw15EQCf9lB7Jvuksy3dUnzhdDgbg9lbOpaWPgYiY 1/3QiMSDo1sdqJtHVTLFcqZvm8ZqAc3hhLxy9Vt9jDPc8cONCuvuPsOhndi2hGVHsfPH Lq8BvVlgvXSDvIlJaX9NB14xl7eFdyHr/7gwuzkeFsF3eCQG8qbBi7B6OR70gvl/3mB4 VD1Q== Received: by 10.224.109.199 with SMTP id k7mr4591169qap.66.1352251090756; Tue, 06 Nov 2012 17:18:10 -0800 (PST) MIME-Version: 1.0 Received: by 10.229.186.6 with HTTP; Tue, 6 Nov 2012 17:17:49 -0800 (PST) In-Reply-To: References: <20121031001531.5D197121AAA@hchopra.mtv.corp.google.com> From: Harshit Chopra Date: Wed, 07 Nov 2012 01:18:00 -0000 Message-ID: Subject: Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051) To: Xinliang David Li Cc: GCC Patches , reply@codereview.appspotmail.com Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-Gm-Message-State: ALoCoQnD/OaGSFNypKFZmXievsUVRyflLAoZ9e88gaKfPb5726T3EmLuhcfgwhL3oJeSbTbTJVTCNqqOAgCzST+Hdu5+KJWyiQ4Zvs6ee+CuHiHQd3aYpNuTOYBJZOD+pDH4nzTSYkIl+GiM5kkm4Lpcww5yi7tCeCLYDDfhNsc/jsXKtrKU7MGMS6eqbS/9SQxJL0QrXG/1 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-11/txt/msg00605.txt.bz2 Yes, will do, but probably not so soon. Once I have some spare time to prepare my case for this being useful to public. Meanwhile, this patch is just for google-main and then I will port it to google_4-7 and adds to the already existing functionality of -mpatch-function-for-instrumentation. Thanks, Harshit On Mon, Nov 5, 2012 at 12:29 PM, Xinliang David Li wro= te: > It does not hurt to submit the patch for review -- you need to provide > more background and motivation for this work > 1) comparison with -finstrument-functions (runtime overhead etc) > 2) use model difference (production binary ..) > 3) Interesting examples of use cases (with graphs). > > thanks, > > David > > On Mon, Nov 5, 2012 at 12:20 PM, Harshit Chopra wrot= e: >> Thanks David for the review. My comments are inline. >> >> >> On Sat, Nov 3, 2012 at 12:38 PM, Xinliang David Li = wrote: >>> >>> Harshit, Nov 5 is the gcc48 cutoff date. If you want to have the x-ray >>> instrumentation feature into this release, you will need to port your >>> patch and submit for trunk review now. >> >> >> I am a bit too late now, I guess. If I target for the next release, >> will it create any issues for the gcc48 release? >> >>> >>> >>> >>> On Tue, Oct 30, 2012 at 5:15 PM, Harshit Chopra wr= ote: >>> > Adding function attributes: 'always_patch_for_instrumentation' and 'n= ever_patch_for_instrumentation' to always patch a function or to never patc= h a function, respectively, when given the option -mpatch-functions-for-ins= trumentation. Additionally, the attribute always_patch_for_instrumentation = disables inlining of that function. >>> > >>> > Tested: >>> > Tested by 'crosstool-validate.py --crosstool_ver=3D16 --testers=3Dc= rosstool' >>> > >>> > ChangeLog: >>> > >>> > 2012-10-30 Harshit Chopra >>> > >>> > * gcc/c-family/c-common.c (handle_always_patch_for_instrument= ation_attribute): Handle >>> > always_patch_for_instrumentation attribute and turn inlining off fo= r the function. >>> > (handle_never_patch_for_instrumentation_attribute): Handle ne= ver_patch_for_instrumentation >>> > attribute of a function. >>> > * gcc/config/i386/i386.c (check_should_patch_current_function= ): Takes into account >>> > always_patch_for_instrumentation or never_patch_for_instrumentation= attribute when >>> > deciding that a function should be patched. >>> > * gcc/testsuite/gcc.target/i386/patch-functions-force-no-patc= hing.c: Test case >>> > to test for never_patch_for_instrumentation attribute. >>> > * gcc/testsuite/gcc.target/i386/patch-functions-force-patchin= g.c: Test case to >>> > test for always_patch_for_instrumentation attribute. >>> > * gcc/tree.h (struct GTY): Add fields for the two attributes = and macros to access >>> > the fields. >>> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c >>> > index ab416ff..998645d 100644 >>> > --- a/gcc/c-family/c-common.c >>> > +++ b/gcc/c-family/c-common.c >>> > @@ -396,6 +396,13 @@ static tree ignore_attribute (tree *, tree, tree= , int, bool *); >>> > static tree handle_no_split_stack_attribute (tree *, tree, tree, int= , bool *); >>> > static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *= ); >>> > >>> > +static tree handle_always_patch_for_instrumentation_attribute (tree = *, tree, >>> > + tree,= int, >>> > + bool = *); >>> >>> Move bool * to the previous line. >> >> >> If I do that, it goes beyond the 80 char boundary. >> >>> >>> >>> > +static tree handle_never_patch_for_instrumentation_attribute (tree *= , tree, >>> > + tree, = int, >>> > + bool *= ); >>> > + >>> >>> Same here. >> >> >> As above. >> >>> >>> >>> > static void check_function_nonnull (tree, int, tree *); >>> > static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT); >>> > static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT); >>> > @@ -804,6 +811,13 @@ const struct attribute_spec c_common_attribute_t= able[] =3D >>> > The name contains space to prevent its usage in source code. */ >>> > { "fn spec", 1, 1, false, true, true, >>> > handle_fnspec_attribute, false }, >>> > + { "always_patch_for_instrumentation", 0, 0, true, false, false, >>> > + handle_always_patch_for_instrumentatio= n_attribute, >>> > + false }, >>> > + { "never_patch_for_instrumentation", 0, 0, true, false, false, >>> > + handle_never_patch_for_instrumentation= _attribute, >>> > + false }, >>> > + >>> > { NULL, 0, 0, false, false, false, NULL, false= } >>> > }; >>> > >>> > @@ -9158,6 +9172,48 @@ handle_no_thread_safety_analysis_attribute (tr= ee *node, tree name, >>> > return NULL_TREE; >>> > } >>> > >>> > +/* Handle a "always_patch_for_instrumentation" attribute; arguments = as in >>> > + struct attribute_spec.handler. */ >>> >>> Add new line here >> >> >> Done. >> >>> >>> >>> > +static tree >>> > +handle_always_patch_for_instrumentation_attribute (tree *node, tree = name, >>> > + tree ARG_UNUSED (= args), >>> > + int ARG_UNUSED (f= lags), >>> > + bool *no_add_attr= s) >>> > +{ >>> > + if (TREE_CODE (*node) =3D=3D FUNCTION_DECL) >>> > + { >>> > + DECL_FORCE_PATCHING_FOR_INSTRUMENTATION (*node) =3D 1; >>> > + DECL_UNINLINABLE (*node) =3D 1; >>> > + } >>> > + else >>> > + { >>> > + warning (OPT_Wattributes, "%qE attribute ignored", name); >>> > + *no_add_attrs =3D true; >>> > + } >>> > + return NULL_TREE; >>> > +} >>> > + >>> > + >>> > +/* Handle a "never_patch_for_instrumentation" attribute; arguments a= s in >>> > + struct attribute_spec.handler. */ >>> >>> A new line here. >> >> >> Done >> >>> >>> >>> > +static tree >>> > +handle_never_patch_for_instrumentation_attribute (tree *node, tree n= ame, >>> > + tree ARG_UNUSED (a= rgs), >>> > + int ARG_UNUSED (fl= ags), >>> > + bool *no_add_attrs) >>> > +{ >>> > + if (TREE_CODE (*node) =3D=3D FUNCTION_DECL) >>> > + DECL_FORCE_NO_PATCHING_FOR_INSTRUMENTATION (*node) =3D 1; >>> >>> Probably no need for this. The attribute will be attached to the decl >>> node -- can be queried using lookup_attribute method. >> >> >> Done. >> >>> >>> >>> > + else >>> > + { >>> > + warning (OPT_Wattributes, "%qE attribute ignored", name); >>> > + *no_add_attrs =3D true; >>> > + } >>> > + return NULL_TREE; >>> > +} >>> > + >>> > + >>> > + >>> > /* Check for valid arguments being passed to a function with FNTYPE. >>> > There are NARGS arguments in the array ARGARRAY. */ >>> > void >>> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >>> > index 6972ae6..b1475bf 100644 >>> > --- a/gcc/config/i386/i386.c >>> > +++ b/gcc/config/i386/i386.c >>> > @@ -10983,6 +10983,13 @@ check_should_patch_current_function (void) >>> > int num_loops =3D 0; >>> > int min_functions_instructions; >>> > >>> > + /* If a function has an attribute forcing patching on or off, do a= s it >>> > + indicates. */ >>> > + if (DECL_FORCE_PATCHING_FOR_INSTRUMENTATION (current_function_decl= )) >>> > + return true; >>> > + else if (DECL_FORCE_NO_PATCHING_FOR_INSTRUMENTATION (current_funct= ion_decl)) >>> > + return false; >>> > + >>> > /* Patch the function if it has at least a loop. */ >>> > if (!patch_functions_ignore_loops) >>> > { >>> > diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-force-no-p= atching.c b/gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching= .c >>> > new file mode 100644 >>> > index 0000000..cad6f2d >>> > --- /dev/null >>> > +++ b/gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching= .c >>> > @@ -0,0 +1,27 @@ >>> > +/* { dg-do compile } */ >>> > +/* { dg-require-effective-target lp64 } */ >>> > +/* { dg-options "-mpatch-functions-for-instrumentation -mno-patch-fu= nctions-main-always" } */ >>> >>> > /* Nonzero if a FUNCTION_CODE is a TM load/store. */ >>> > #define BUILTIN_TM_LOAD_STORE_P(FN) \ >>> > ((FN) >=3D BUILT_IN_TM_STORE_1 && (FN) <=3D BUILT_IN_TM_LOAD_RFW_L= DOUBLE) >>> > @@ -3586,7 +3596,8 @@ struct GTY(()) tree_function_decl { >>> > unsigned has_debug_args_flag : 1; >>> > unsigned tm_clone_flag : 1; >>> > >>> > - /* 1 bit left */ >>> > + unsigned force_patching_for_instrumentation : 1; >>> > + unsigned force_no_patching_for_instrumentation : 1; >>> >>> >>> I don't think you should use precious bits here -- directly query the >>> attributes. >> >> >> Done. >> >>> >>> >>> thanks, >>> >>> David >>> >>> > }; >>> > >>> > /* The source language of the translation-unit. */ >>> > >>> > -- >>> > This patch is available for review at http://codereview.appspot.com/6= 821051