From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9696 invoked by alias); 24 Jul 2014 13:17:24 -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 9659 invoked by uid 89); 24 Jul 2014 13:17:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f175.google.com Received: from mail-ie0-f175.google.com (HELO mail-ie0-f175.google.com) (209.85.223.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 24 Jul 2014 13:17:21 +0000 Received: by mail-ie0-f175.google.com with SMTP id x19so2185135ier.6 for ; Thu, 24 Jul 2014 06:17:19 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.50.2.42 with SMTP id 10mr14423156igr.33.1406207838937; Thu, 24 Jul 2014 06:17:18 -0700 (PDT) Received: by 10.64.229.17 with HTTP; Thu, 24 Jul 2014 06:17:18 -0700 (PDT) In-Reply-To: <20140724113841.GB9836@atrey.karlin.mff.cuni.cz> References: <20140416140313.GC41722@msticlxl57.ims.intel.com> <53CF3267.8080309@redhat.com> <20140724095907.GA15208@msticlxl57.ims.intel.com> <20140724113841.GB9836@atrey.karlin.mff.cuni.cz> Date: Thu, 24 Jul 2014 13:19:00 -0000 Message-ID: Subject: Re: [PATCH, Pointer Bounds Checker 9/x] Cgraph extension From: Ilya Enkovich To: Jan Hubicka Cc: Jeff Law , gcc-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg01662.txt.bz2 2014-07-24 15:38 GMT+04:00 Jan Hubicka : > Hello, > >> diff --git a/gcc/cgraph.h b/gcc/cgraph.h >> index a6a51cf..5e702a7 100644 >> --- a/gcc/cgraph.h >> +++ b/gcc/cgraph.h >> @@ -191,6 +191,7 @@ struct GTY(()) cgraph_thunk_info { >> tree alias; >> bool this_adjusting; >> bool virtual_offset_p; >> + bool add_pointer_bounds_args; >> /* Set to true when alias node is thunk. */ >> bool thunk_p; >> }; >> @@ -373,6 +374,13 @@ public: >> struct cgraph_node *prev_sibling_clone; >> struct cgraph_node *clones; >> struct cgraph_node *clone_of; >> + /* If instrumentation_clone is 1 then instrumented_version points >> + to the original function used to make instrumented version. >> + Otherwise points to instrumented version of the function. */ >> + struct cgraph_node *instrumented_version; >> + /* If instrumentation_clone is 1 then orig_decl is the original >> + function declaration. */ >> + tree orig_decl; > > So the patch is introducing yet another notion of clone (in addition to existing virtual clones > and function versions used by ifun) and you add a new type of reference (CHKP) to link the > original and the clone. > > Why do you need to link things in 3 different ways? (i.e. instrumented_version points to the > same place as CHKP and as orig_decl, right?). CHKP reference is required to have reachability algorithms working correctly and not removing required instrumented nodes. References are rebuilt time to time and instrumented_version is used to rebuild CHKP reference. orig_decl is required because original function node may be removed as unreachable. > > I would preffer if this can be put into the existing clone mechanizm. The virtual clones can > have quite generic transformations done on them and the do perform all the necessary links > back and forth. I suppose virtual clones are useful when we may delay their materialization, i.e. for IPA passes. For checker we have instrumentation almost immediately following clone creation. Instrumentation is a GIMPLE pass and we have to materialize clones to have bodies to instrument. After materialization there is no link to original node anymore and it means we would still require all new fields in cgraph_node structure. > > I will look into the rest of changes, is there some overview? I have a short overview of how it works on a wiki page: https://gcc.gnu.org/wiki/Intel%20MPX%20support%20in%20the%20GCC%20compiler#Instrumentation_clones Thanks, Ilya > > Honza > > >> /* For functions with many calls sites it holds map from call expression >> to the edge to speed up cgraph_edge function. */ >> htab_t GTY((param_is (struct cgraph_edge))) call_site_hash; >> @@ -433,6 +441,9 @@ public: >> /* True if this decl calls a COMDAT-local function. This is set up in >> compute_inline_parameters and inline_call. */ >> unsigned calls_comdat_local : 1; >> + /* True when function is clone created for Pointer Bounds Checker >> + instrumentation. */ >> + unsigned instrumentation_clone : 1; >> }; >> >> >> @@ -1412,6 +1423,8 @@ symtab_alias_target (symtab_node *n) >> { >> struct ipa_ref *ref; >> ipa_ref_list_reference_iterate (&n->ref_list, 0, ref); >> + if (ref->use == IPA_REF_CHKP) >> + ipa_ref_list_reference_iterate (&n->ref_list, 1, ref); >> gcc_checking_assert (ref->use == IPA_REF_ALIAS); >> return ref->referred; >> } >> diff --git a/gcc/cgraphbuild.c b/gcc/cgraphbuild.c >> index 19961e2..a2b2106 100644 >> --- a/gcc/cgraphbuild.c >> +++ b/gcc/cgraphbuild.c >> @@ -481,6 +481,10 @@ rebuild_cgraph_edges (void) >> record_eh_tables (node, cfun); >> gcc_assert (!node->global.inlined_to); >> >> + if (node->instrumented_version >> + && !node->instrumentation_clone) >> + ipa_record_reference (node, node->instrumented_version, IPA_REF_CHKP, NULL); >> + >> return 0; >> } >> >> @@ -513,6 +517,11 @@ cgraph_rebuild_references (void) >> ipa_record_stmt_references (node, gsi_stmt (gsi)); >> } >> record_eh_tables (node, cfun); >> + >> + >> + if (node->instrumented_version >> + && !node->instrumentation_clone) >> + ipa_record_reference (node, node->instrumented_version, IPA_REF_CHKP, NULL); >> } >> >> namespace { >> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c >> index 06283fc..ceb4060 100644 >> --- a/gcc/cgraphunit.c >> +++ b/gcc/cgraphunit.c >> @@ -1702,7 +1702,8 @@ assemble_thunks_and_aliases (struct cgraph_node *node) >> struct ipa_ref *ref; >> >> for (e = node->callers; e;) >> - if (e->caller->thunk.thunk_p) >> + if (e->caller->thunk.thunk_p >> + && !e->caller->thunk.add_pointer_bounds_args) >> { >> struct cgraph_node *thunk = e->caller; >> >> diff --git a/gcc/ipa-ref.c b/gcc/ipa-ref.c >> index 6aa41e6..3a055d9 100644 >> --- a/gcc/ipa-ref.c >> +++ b/gcc/ipa-ref.c >> @@ -27,7 +27,7 @@ along with GCC; see the file COPYING3. If not see >> #include "cgraph.h" >> #include "ipa-utils.h" >> >> -static const char *ipa_ref_use_name[] = {"read","write","addr","alias"}; >> +static const char *ipa_ref_use_name[] = {"read","write","addr","alias","chkp"}; >> >> /* Return ipa reference from REFERING_NODE or REFERING_VARPOOL_NODE >> to REFERED_NODE or REFERED_VARPOOL_NODE. USE_TYPE specify type >> diff --git a/gcc/ipa-ref.h b/gcc/ipa-ref.h >> index 4ce5f8d..d0df0bf 100644 >> --- a/gcc/ipa-ref.h >> +++ b/gcc/ipa-ref.h >> @@ -29,7 +29,8 @@ enum GTY(()) ipa_ref_use >> IPA_REF_LOAD, >> IPA_REF_STORE, >> IPA_REF_ADDR, >> - IPA_REF_ALIAS >> + IPA_REF_ALIAS, >> + IPA_REF_CHKP >> }; >> >> /* Record of reference in callgraph or varpool. */ >> @@ -40,7 +41,7 @@ struct GTY(()) ipa_ref >> gimple stmt; >> unsigned int lto_stmt_uid; >> unsigned int referred_index; >> - ENUM_BITFIELD (ipa_ref_use) use:2; >> + ENUM_BITFIELD (ipa_ref_use) use:3; >> unsigned int speculative:1; >> }; >> >> diff --git a/gcc/ipa.c b/gcc/ipa.c >> index 5ab3aed..1d7fa35 100644 >> --- a/gcc/ipa.c >> +++ b/gcc/ipa.c >> @@ -508,6 +508,12 @@ symtab_remove_unreachable_nodes (bool before_inlining_p, FILE *file) >> cgraph_node_remove_callees (node); >> ipa_remove_all_references (&node->ref_list); >> changed = true; >> + if (node->thunk.thunk_p >> + && node->thunk.add_pointer_bounds_args) >> + { >> + node->thunk.thunk_p = false; >> + node->thunk.add_pointer_bounds_args = false; >> + } >> } >> } >> else >> @@ -583,7 +589,10 @@ symtab_remove_unreachable_nodes (bool before_inlining_p, FILE *file) >> if (node->address_taken >> && !node->used_from_other_partition) >> { >> - if (!cgraph_for_node_and_aliases (node, has_addr_references_p, NULL, true)) >> + if (!cgraph_for_node_and_aliases (node, has_addr_references_p, NULL, true) >> + && (!node->instrumentation_clone >> + || !node->instrumented_version >> + || !node->instrumented_version->address_taken)) >> { >> if (file) >> fprintf (file, " %s", node->name ()); >> @@ -814,6 +823,10 @@ cgraph_externally_visible_p (struct cgraph_node *node, >> if (MAIN_NAME_P (DECL_NAME (node->decl))) >> return true; >> >> + if (node->instrumentation_clone >> + && MAIN_NAME_P (DECL_NAME (node->orig_decl))) >> + return true; >> + >> return false; >> } >> >> @@ -1016,6 +1029,7 @@ function_and_variable_visibility (bool whole_program) >> } >> >> if (node->thunk.thunk_p >> + && !node->thunk.add_pointer_bounds_args >> && TREE_PUBLIC (node->decl)) >> { >> struct cgraph_node *decl_node = node; >> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c >> index 999ce3d..58105f0 100644 >> --- a/gcc/lto-cgraph.c >> +++ b/gcc/lto-cgraph.c >> @@ -526,6 +526,7 @@ lto_output_node (struct lto_simple_output_block *ob, struct cgraph_node *node, >> bp_pack_value (&bp, node->thunk.thunk_p && !boundary_p, 1); >> bp_pack_enum (&bp, ld_plugin_symbol_resolution, >> LDPR_NUM_KNOWN, node->resolution); >> + bp_pack_value (&bp, node->instrumentation_clone, 1); >> streamer_write_bitpack (&bp); >> >> if (node->thunk.thunk_p && !boundary_p) >> @@ -533,11 +534,15 @@ lto_output_node (struct lto_simple_output_block *ob, struct cgraph_node *node, >> streamer_write_uhwi_stream >> (ob->main_stream, >> 1 + (node->thunk.this_adjusting != 0) * 2 >> - + (node->thunk.virtual_offset_p != 0) * 4); >> + + (node->thunk.virtual_offset_p != 0) * 4 >> + + (node->thunk.add_pointer_bounds_args != 0) * 8); >> streamer_write_uhwi_stream (ob->main_stream, node->thunk.fixed_offset); >> streamer_write_uhwi_stream (ob->main_stream, node->thunk.virtual_value); >> } >> streamer_write_hwi_stream (ob->main_stream, node->profile_id); >> + >> + if (node->instrumentation_clone) >> + lto_output_fn_decl_index (ob->decl_state, ob->main_stream, node->orig_decl); >> } >> >> /* Output the varpool NODE to OB. >> @@ -613,7 +618,7 @@ lto_output_ref (struct lto_simple_output_block *ob, struct ipa_ref *ref, >> struct cgraph_node *node; >> >> bp = bitpack_create (ob->main_stream); >> - bp_pack_value (&bp, ref->use, 2); >> + bp_pack_value (&bp, ref->use, 3); >> bp_pack_value (&bp, ref->speculative, 1); >> streamer_write_bitpack (&bp); >> nref = lto_symtab_encoder_lookup (encoder, ref->referred); >> @@ -1002,6 +1007,7 @@ input_overwrite_node (struct lto_file_decl_data *file_data, >> node->thunk.thunk_p = bp_unpack_value (bp, 1); >> node->resolution = bp_unpack_enum (bp, ld_plugin_symbol_resolution, >> LDPR_NUM_KNOWN); >> + node->instrumentation_clone = bp_unpack_value (bp, 1); >> gcc_assert (flag_ltrans >> || (!node->in_other_partition >> && !node->used_from_other_partition)); >> @@ -1112,10 +1118,19 @@ input_node (struct lto_file_decl_data *file_data, >> node->thunk.this_adjusting = (type & 2); >> node->thunk.virtual_value = virtual_value; >> node->thunk.virtual_offset_p = (type & 4); >> + node->thunk.add_pointer_bounds_args = (type & 8); >> } >> if (node->alias && !node->analyzed && node->weakref) >> node->alias_target = get_alias_symbol (node->decl); >> node->profile_id = streamer_read_hwi (ib); >> + >> + if (node->instrumentation_clone) >> + { >> + decl_index = streamer_read_uhwi (ib); >> + fn_decl = lto_file_decl_data_get_fn_decl (file_data, decl_index); >> + node->orig_decl = fn_decl; >> + } >> + >> return node; >> } >> >> @@ -1196,7 +1211,7 @@ input_ref (struct lto_input_block *ib, >> struct ipa_ref *ref; >> >> bp = streamer_read_bitpack (ib); >> - use = (enum ipa_ref_use) bp_unpack_value (&bp, 2); >> + use = (enum ipa_ref_use) bp_unpack_value (&bp, 3); >> speculative = (enum ipa_ref_use) bp_unpack_value (&bp, 1); >> node = nodes[streamer_read_hwi (ib)]; >> ref = ipa_record_reference (referring_node, node, use, NULL); >> @@ -1337,6 +1352,22 @@ input_cgraph_1 (struct lto_file_decl_data *file_data, >> cgraph (node)->global.inlined_to = cgraph (nodes[ref]); >> else >> cnode->global.inlined_to = NULL; >> + >> + /* Compute instrumented_version. */ >> + if (cnode->instrumentation_clone) >> + { >> + gcc_assert (cnode->orig_decl); >> + >> + cnode->instrumented_version = cgraph_get_node (cnode->orig_decl); >> + if (cnode->instrumented_version) >> + cnode->instrumented_version->instrumented_version = cnode; >> + >> + /* Restore decl names reference. */ >> + if (IDENTIFIER_TRANSPARENT_ALIAS (DECL_ASSEMBLER_NAME (cnode->decl)) >> + && !TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl))) >> + TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl)) >> + = DECL_ASSEMBLER_NAME (cnode->orig_decl); >> + } >> } >> >> ref = (int) (intptr_t) node->same_comdat_group; >> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h >> index 51b1903..62a5fe0 100644 >> --- a/gcc/lto-streamer.h >> +++ b/gcc/lto-streamer.h >> @@ -141,7 +141,7 @@ along with GCC; see the file COPYING3. If not see >> #define LTO_SECTION_NAME_PREFIX ".gnu.lto_" >> >> #define LTO_major_version 3 >> -#define LTO_minor_version 0 >> +#define LTO_minor_version 1 >> >> typedef unsigned char lto_decl_flags_t; >>