From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36575 invoked by alias); 10 Nov 2018 00:20:06 -0000 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 Received: (qmail 36555 invoked by uid 89); 10 Nov 2018 00:20:05 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=sight X-HELO: mail-qk1-f180.google.com Received: from mail-qk1-f180.google.com (HELO mail-qk1-f180.google.com) (209.85.222.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 10 Nov 2018 00:20:01 +0000 Received: by mail-qk1-f180.google.com with SMTP id a132so4794634qkg.1 for ; Fri, 09 Nov 2018 16:20:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=5IK5fBLkVINAzXmk/Vy3oZVhDNxbpGmfUDOx1QjpOCs=; b=JEDVZTPWECfQ9rJ8YQZ+tvYuH9PR3uK6HB1dct+EF4K1elxD4BQKb9spki6gXJXnHe LNknon2fMySSP9OYYTwCseo9f1xVI95w/7WO0e8x/3IdslAO3ViiwnjeywiCcWM1yu7P o8MAqtPB1aFIyokDVLFyvWEb0agOs3sxy/pHlxNIuDUNKwOl5HO3aB73UGucG+Xp0RmY Au/Gz3DRaWx/WQE4JJFHiFtgeOAmibqq1bNOkaRoj7TzGaVBGOa7iTO/EnnRwAxoYX1x +Npr0YIFWS5rIjXucseNdNgsoKv6q/sxD/L1hYXhF03FZhTfPEBxAZb86POvkdFRBYOS MOKw== Return-Path: Received: from localhost.localdomain (184-96-239-209.hlrn.qwest.net. [184.96.239.209]) by smtp.gmail.com with ESMTPSA id c77sm6729385qkh.82.2018.11.09.16.19.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 09 Nov 2018 16:19:58 -0800 (PST) Subject: Re: [PATCH 2/3] Add a pass to automatically add ptwrite instrumentation To: Andi Kleen , gcc-patches@gcc.gnu.org References: <20181104063235.6914-1-andi@firstfloor.org> <20181104063235.6914-2-andi@firstfloor.org> Cc: Andi Kleen From: Martin Sebor Message-ID: <899fc065-4a0b-9867-9daa-d92699c6c59d@gmail.com> Date: Sat, 10 Nov 2018 00:20:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20181104063235.6914-2-andi@firstfloor.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00808.txt.bz2 On 11/04/2018 12:32 AM, Andi Kleen wrote: > From: Andi Kleen > > Add a new pass to automatically instrument changes to variables > with the new PTWRITE instruction on x86. PTWRITE writes a 4 or 8 byte > field into an Processor Trace log, which allows log over head > logging of informatin. > > This allows to reconstruct how values later, which can be useful for > debugging or other analysis of the program behavior. With the compiler > support this can be done with without having to manually add instrumentation > to the code. > > Using dwarf information this can be later mapped back to the variables. > > There are new options to enable instrumentation for different types, > and also a new attribute to control analysis fine grained per > function or variable level. The attributes can be set on both > the variable and the type level, and also on structure fields. > This allows to enable tracing only for specific code in large > programs. > > The pass is generic, but only the x86 backend enables the necessary > hooks. When the backend enables the necessary hooks (with -mptwrite) > there is an additional pass that looks through the code for > attribute vartrace enabled functions or variables. > > The -fvartrace-locals options is experimental: it works, but it > generates redundant ptwrites because the pass doesn't use > the SSA information to minimize instrumentation. This could be optimized > later. > > Currently the code can be tested with SDE, or on a Intel > Gemini Lake system with a new enough Linux kernel (v4.10+) > that supports PTWRITE for PT. Linux perf can be used to > record the values > > perf record -e intel_pt/ptw=1,branch=0/ program > perf script --itrace=crw -F +synth ... > > I have an experimential version of perf that can also use > dwarf information to symbolize many[1] values back to their variable > names. So far it is not in standard perf, but available at > > https://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc.git/log/?h=perf/var-resolve-4 > > It is currently not able to decode all variable locations to names, > but a large subset. > > Longer term hopefully gdb will support this information too. > > The CPU can potentially generate very data high bandwidths when > code doing a lot of computation is heavily instrumented. > This can cause some data loss in both the CPU and also in perf > logging the data when the disk cannot keep up. > > Running some larger workloads most workloads do not cause > CPU level overflows, but I've seen it with -fvartrace > with crafty, and with more workloads with -fvartrace-locals. > > Recommendation is to not fully instrument programs, > but only areas of interest either at the file level or using > the attributes. > > The other thing is that perf and the disk often cannot keep up > with the data bandwidth for longer computations. In this case > it's possible to use perf snapshot mode (add --snapshot > to the command line above). The data will be only logged to > a memory ring buffer then, and only dump the buffers on events > of interest by sending SIGUSR2 to the perf binrary. > > In the future this will be hopefully better supported with > core files and gdb. > > Passes bootstrap and test suite on x86_64-linux, also > bootstrapped and tested gcc itself with full -fvartrace > and -fvartrace-locals instrumentation. (I initially meant to just suggest detecting and rejecting the two mutually exclusive attributes but as I read the rest of the patch to better understand what it's about I noticed a few other issues I thought would be useful to point out.) ... > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index 4416b5042f7..66bbd87921f 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > @@ -325,6 +327,12 @@ const struct attribute_spec c_common_attribute_table[] = > { "no_instrument_function", 0, 0, true, false, false, false, > handle_no_instrument_function_attribute, > NULL }, > + { "vartrace", 0, 0, false, false, false, false, > + handle_vartrace_attribute, > + NULL }, > + { "no_vartrace", 0, 0, false, false, false, false, > + handle_vartrace_attribute, > + NULL }, > { "no_profile_instrument_function", 0, 0, true, false, false, false, > handle_no_profile_instrument_function_attribute, > NULL }, Unless mixing these attributes on the same declaration makes sense I would suggest to either define the exclusions that should be automatically applied to the attributes (see attribute exclusions), or to enforce them in the handler. Judging only by the names it looks to me like vartrace should be mutually exclusive with no_vartrace. > @@ -767,6 +775,21 @@ handle_no_sanitize_undefined_attribute (tree *node, tree name, tree, int, > return NULL_TREE; > } > > +/* Handle "vartrace"/"no_vartrace" attributes; arguments as in > + struct attribute_spec.handler. */ > + > +static tree > +handle_vartrace_attribute (tree *node, tree, tree, int flags, > + bool *) > +{ > + if (TYPE_P (*node) && !(flags & (int) ATTR_FLAG_TYPE_IN_PLACE)) > + *node = build_variant_type_copy (*node); > + > + /* Can apply to types, functions, variables. */ I suspect the attribute shouldn't be applied to LABEL_DECLs but the code suggests it's accepted there. Does the patch reject it on labels? (I think it should with at least a warning.) Similarly, does it make sense to apply the attribute to CONST_DECLs such as enumerators? If not, then I would suggest to give a warning for those as well, mentioning the kind of symbols it either does or doesn't apply to. Ditto there any other DECLs that it doesn't apply to. Also, if I understand things correctly, it seems that the handler would benefit from making use of TARGET_VARTRACE_FUNC() to give a warning when a target doesn't support tracing at all, and perhaps also a different warning when it does but not for variables of the given type/precision, etc. This might need to be controlled by a different option than -Wattributes. > + /* We lookup it up later with lookup_attribute. */ > + return NULL_TREE; > +} > + > /* Handle an "asan odr indicator" attribute; arguments as in > struct attribute_spec.handler. */ > > diff --git a/gcc/common.opt b/gcc/common.opt > index 2971dc21b1f..930acf40588 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -2811,6 +2811,30 @@ ftree-scev-cprop > Common Report Var(flag_tree_scev_cprop) Init(1) Optimization > Enable copy propagation of scalar-evolution information. > > +fvartrace > +Common Report Var(flag_vartrace) > +Generate all variable tracking instrumentations, except for locals. > + > +fvartrace-returns > +Common Report Var(flag_vartrace_returns) > +Generate variable tracking instructions for function returns. > + > +fvartrace-args > +Common Report Var(flag_vartrace_args) > +Generate variable tracking instructions for function arguments. > + > +fvartrace-reads > +Common Report Var(flag_vartrace_reads) > +Generate variable tracking instructions for reads. > + > +fvartrace-writes > +Common Report Var(flag_vartrace_writes) > +Generate variable tracking instructions for writes. > + > +fvartrace-locals > +Common Report Var(flag_vartrace_locals) > +Generate variable tracking instructions for locals. > + > ; -fverbose-asm causes extra commentary information to be produced in > ; the generated assembly code (to make it more readable). This option > ; is generally only of use to those who actually need to read the > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index 490bb6292a8..4337121c239 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c > @@ -31873,6 +31873,19 @@ ix86_mangle_function_version_assembler_name (tree decl, tree id) > } > > > +static tree > +ix86_vartrace_func (tree type) > +{ > + if (!(ix86_isa_flags2 & OPTION_MASK_ISA_PTWRITE)) > + return NULL; > + if (TYPE_PRECISION (type) == 32) > + return ix86_builtins [(int) IX86_BUILTIN_PTWRITE32]; > + else if (TYPE_PRECISION (type) == 64) > + return ix86_builtins [(int) IX86_BUILTIN_PTWRITE64]; > + else > + return NULL; > +} Do I understand correctly that the tracing is limited to variables of 32-bit and 64-bit scalar types? If so, shouldn't the attribute handler detect when the attribute is applied to variables of other types/sizes/precisions and give a warning that tracing is not supported there? (I think it should though portability to other targets with support for other types/sizes might need to be considered.) I would suggest to certainly document this limitation for each target. > + > static tree > ix86_mangle_decl_assembler_name (tree decl, tree id) > { > @@ -50849,6 +50862,9 @@ ix86_run_selftests (void) > #undef TARGET_ASAN_SHADOW_OFFSET > #define TARGET_ASAN_SHADOW_OFFSET ix86_asan_shadow_offset > > +#undef TARGET_VARTRACE_FUNC > +#define TARGET_VARTRACE_FUNC ix86_vartrace_func > + > #undef TARGET_GIMPLIFY_VA_ARG_EXPR > #define TARGET_GIMPLIFY_VA_ARG_EXPR ix86_gimplify_va_arg > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index 1eca009e255..08286aa4591 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -3193,6 +3193,13 @@ the standard C library can be guaranteed not to throw an exception > with the notable exceptions of @code{qsort} and @code{bsearch} that > take function pointer arguments. > > +@item no_vartrace > +@cindex @code{no_vartrace} function or variable attribute If it also applies to types (as stated below) it's not just a "function or variable attribute." I think conventionally this text describes the kind of attribute that's discussed in the subsequent paragraphs, so it should probably read "function attribute" for a function attribute and "variable attribute" for a variable attribute, etc. > +Disable data tracing for the function or variable or structured field Typo: "structure field" or "structure member" (not structured). > +marked with this attribute. Applies to types. Currently implemented > +for x86 when the @option{ptwrite} target option is enabled for systems > +that support the @code{PTWRITE} instruction. I would suggest to use full sentences in the manual (as in other entries in this section). Such as: The @code{no_vartrace} attribute disables data tracing for the function [or variable or structure field] declared with the attribute. The attribute is currently implemented for x86 when the @option{ptwrite} target option is enabled for systems that support the @code{PTWRITE} instruction. See @pxref{Common Variable Attributes} and @pxref{Common Type Attributes}. There is no description of the effect of the attribute on a function. That seems important to discuss. Since the attribute applies to types it should also be listed in the Common Type Attributes section (if it isn't -- I only see it mentioned twice in the patch) and its effects on types explained there. (On further thought, if the sentence "Applies to types." means something else than that it can also be applied to type definitions it should be clarified.) > + > @item optimize (@var{level}, @dots{}) > @item optimize (@var{string}, @dots{}) > @cindex @code{optimize} function attribute > @@ -3454,6 +3461,12 @@ When applied to a member function of a C++ class template, the > attribute also means that the function is instantiated if the > class itself is instantiated. > > +@item vartrace > +@cindex @code{vartrace} function or variable attribute > +Enable data tracing for the function or variable or structure field > +marked with this attribute. Applies to types. Will not trace locals, > +but arguments, returns, globals, pointer references. Same as above. I'm guessing this text describes the effects of the attribute on a function. It feels like it could use quite a bit more detail. For instance, how do the function attribute interact with type or variable attributes specified on objects accessed by the function. What is the effect on functions inlined into a caller declared with the attribute? > + > @item visibility ("@var{visibility_type}") > @cindex @code{visibility} function attribute > This attribute affects the linkage of the declaration to which it is attached. > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index cb5bc7bafc5..2f10b3c1023 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -2720,6 +2720,35 @@ Don't use the @code{__cxa_get_exception_ptr} runtime routine. This > causes @code{std::uncaught_exception} to be incorrect, but is necessary > if the runtime routine is not available. > > +@item -fvartrace > +@opindex -fvartrace > +Insert trace instructions to trace variable values at runtime. > +Requires enabling a backend specific option, like @option{-mptwrite} to enable > +@code{PTWRITE} instruction generation on x86. @option{-fvartrace} traces > +arguments, return values, pointer references and globals, but no locals. Not to be too pedantic but presumably it traces also static locals. If so, suggest to refer to "objects with static storage duration" instead and perhaps also "objects with thread storage duration" if it applies to those as well. > + > +@item -fvartrace-args > +@opindex -fvartrace-args > +Trace arguments. Can be used independently or together with @option{-vartrace}, > +or as @option{-fno-vartrace-args} to disable. > + > +@item -fvartrace-returns > +@opindex -fvartrace-returns > +Trace return values. Can be used independently or together with @option{-vartrace}, > +or as @option{-fno-vartrace-return} to disable. > + > +@item -fvartrace-reads > +@opindex -fvartrace-reads > +Trace reads. > + > +@item -fvartrace-writes > +@opindex -fvartrace-writes > +Trace writes. > + > +@item -fvartrace-locals > +@opindex -fvartrace-locals > +Insert code to trace local variables. This can have high overhead. > + > @item -fvisibility-inlines-hidden > @opindex fvisibility-inlines-hidden > This switch declares that the user does not attempt to compare > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi > index f841527f971..6555cb122e9 100644 > --- a/gcc/doc/tm.texi > +++ b/gcc/doc/tm.texi > @@ -11933,6 +11933,10 @@ Address Sanitizer shadow memory address. NULL if Address Sanitizer is not > supported by the target. > @end deftypefn > > +@deftypefn {Target Hook} tree TARGET_VARTRACE_FUNC (tree @var{type}) > +Return a builtin to call to trace variables or NULL if not supported by the target. > +@end deftypefn So (IIUC) the hook returns true iff the target supports tracking of objects of the given type, correct? (If so, the documentation should state that. As it is, it doesn't mention type at all.) > + > @deftypefn {Target Hook} {unsigned HOST_WIDE_INT} TARGET_MEMMODEL_CHECK (unsigned HOST_WIDE_INT @var{val}) > Validate target specific memory model mask bits. When NULL no target specific > memory model bits are allowed. > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in > index 967ef3ad22f..7cce21bb26c 100644 > --- a/gcc/doc/tm.texi.in > +++ b/gcc/doc/tm.texi.in > @@ -8101,6 +8101,8 @@ and the associated definitions of those functions. > > @hook TARGET_ASAN_SHADOW_OFFSET > > +@hook TARGET_VARTRACE_FUNC > + > @hook TARGET_MEMMODEL_CHECK > > @hook TARGET_ATOMIC_TEST_AND_SET_TRUEVAL > diff --git a/gcc/passes.def b/gcc/passes.def > index 24f212c8e31..518cb4ef6f7 100644 > --- a/gcc/passes.def > +++ b/gcc/passes.def > @@ -179,6 +179,7 @@ along with GCC; see the file COPYING3. If not see > NEXT_PASS (pass_oacc_device_lower); > NEXT_PASS (pass_omp_device_lower); > NEXT_PASS (pass_omp_target_link); > + NEXT_PASS (pass_vartrace); > NEXT_PASS (pass_all_optimizations); > PUSH_INSERT_PASSES_WITHIN (pass_all_optimizations) > NEXT_PASS (pass_remove_cgraph_callee_edges); > diff --git a/gcc/target.def b/gcc/target.def > index ad27d352ca4..db5d88efb95 100644 > --- a/gcc/target.def > +++ b/gcc/target.def > @@ -4300,6 +4300,13 @@ supported by the target.", > unsigned HOST_WIDE_INT, (void), > NULL) > > +/* Defines the builtin to trace variables, or NULL. */ > +DEFHOOK > +(vartrace_func, > + "Return a builtin to call to trace variables or NULL if not supported by the target.", > + tree, (tree type), Same as above (how is TYPE used?) > + NULL) > + > /* Functions relating to calls - argument passing, returns, etc. */ > /* Members of struct call have no special macro prefix. */ > HOOK_VECTOR (TARGET_CALLS, calls) > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h > index af15adc8e0c..2cf31785a6f 100644 > --- a/gcc/tree-pass.h > +++ b/gcc/tree-pass.h > @@ -423,6 +423,7 @@ extern gimple_opt_pass *make_pass_strlen (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_fold_builtins (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_post_ipa_warn (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_stdarg (gcc::context *ctxt); > +extern gimple_opt_pass *make_pass_vartrace (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_early_warn_uninitialized (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_late_warn_uninitialized (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_cse_reciprocals (gcc::context *ctxt); > diff --git a/gcc/tree-vartrace.c b/gcc/tree-vartrace.c > new file mode 100644 > index 00000000000..07f5aa6bc8f > --- /dev/null > +++ b/gcc/tree-vartrace.c > @@ -0,0 +1,463 @@ > +/* Insert instructions for data value tracing. > + Copyright (C) 2017 Free Software Foundation, Inc. > + Contributed by Andi Kleen. > + > +This file is part of GCC. > + > +GCC is free software; you can redistribute it and/or modify > +it under the terms of the GNU General Public License as published by > +the Free Software Foundation; either version 3, or (at your option) > +any later version. > + > +GCC is distributed in the hope that it will be useful, > +but WITHOUT ANY WARRANTY; without even the implied warranty of > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +GNU General Public License for more details. > + > +You should have received a copy of the GNU General Public License > +along with GCC; see the file COPYING3. If not see > +. */ > + > +#include "config.h" > +#include "system.h" > +#include "coretypes.h" > +#include "backend.h" > +#include "target.h" > +#include "tree.h" > +#include "tree-iterator.h" > +#include "tree-pass.h" > +#include "basic-block.h" > +#include "gimple.h" > +#include "gimple-iterator.h" > +#include "gimplify.h" > +#include "gimplify-me.h" > +#include "gimple-ssa.h" > +#include "gimple-pretty-print.h" > +#include "cfghooks.h" > +#include "ssa.h" > +#include "tree-dfa.h" > +#include "attribs.h" > + > +enum attrstate { force_off, force_on, neutral }; > + > +/* Can we trace with attributes ATTR. */ > + > +static attrstate supported_attr (tree attr) > +{ > + if (lookup_attribute ("no_vartrace", attr)) > + return force_off; > + if (lookup_attribute ("vartrace", attr)) > + return force_on; Does this imply that no_vartrace overrides vartrace on the same declaration? Unless that's the intended design (in which case I would expect to see it mentioned in the manual and tested) I think it's preferable to have the attribute handler treat these consistently with similar either/or attributes such as cold/hot, or noreturn/returns_nonnull, etc. (i.e., warn and drop the conflicting one that's being added so back ends don't have to worry about ambiguous attribute combinations). > + return neutral; > +} > + > +/* Is ARG supported considering S, handling both decls and other trees. */ > + > +static attrstate supported_op (tree arg, attrstate s) > +{ > + if (s != neutral) > + return s; > + if (DECL_P (arg)) > + { > + s = supported_attr (DECL_ATTRIBUTES (arg)); > + if (s != neutral) > + return s; > + } > + return supported_attr (TYPE_ATTRIBUTES (TREE_TYPE (arg))); > +} > + > +/* Can we trace T. */ > + > +static attrstate supported_type (tree t) > +{ > + tree type = TREE_TYPE (t); > + > + if (!POINTER_TYPE_P (type) && !INTEGRAL_TYPE_P (type)) > + return force_off; This looks like tracing is supported only for integers and pointers but not other types (such as floats). If I'm reading that correctly then this restriction too should be exposed to the attribute handler so meaningless uses of the attribute could be detected (again, perhaps under an option other than -Wattributes). > + enum attrstate s = supported_op (t, neutral); > + if (TREE_CODE (t) == COMPONENT_REF > + || TREE_CODE (t) == ARRAY_REF) > + { > + s = supported_op (TREE_OPERAND (t, 0), s); > + s = supported_op (TREE_OPERAND (t, 1), s); This may be a naive question but what will this code do for: extern __attribute__((no_vartrace)) int a[]; extern __attribute__((vartrace)) int idx; int f (void) { return a[idx]; } Will it trace the read of IDX? (I would expect it to but I'm not sure the code will have that effect.) A more detailed comment might help. > + } > + return s; > +} > + > +/* Can we trace T, or if FORCE is set. */ > + > +static bool supported_type_or_force (tree t, bool force) > +{ > + enum attrstate s = supported_type (t); > + if (s == neutral) > + return force; > + return s == force_off ? false : true; > +} > + > +/* Return true if T refering to a local variable. Typo: "Returns true if T refers to a local variable." Though since T is assumed to be a DECL then naming it decl would make that obvious at the first sight. (I would suggest to use that naming convention for all such function arguments in this file.) I did not look at the rest of the patch. Thanks Martin