On Tue, Apr 19, 2011 at 2:38 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> Hi, Insane value profile data may contain indirect call targets with
>> wrong (corrupted) pids. r172276 solves the problem when the pid
>> refers to a bogus target that is still 'alive'. This patch addresses
>> the issue when the bogus target is already eliminated or pid is too
>> large.
>>
>> OK after testing? (SPEC FDO testing is currently blocked by some
>> regressions).
>
> I will look into the FDO inliner ICEs. I added sanity check for profile
> updates
> and it seems that insane profiles are mishandled somewhere.
>
>> Index: cgraph.c
>> ===================================================================
>> --- cgraph.c (revision 172721)
>> +++ cgraph.c (working copy)
>> @@ -1478,6 +1478,7 @@ cgraph_remove_node (struct cgraph_node *
>> cgraph_call_node_removal_hooks (node);
>> cgraph_node_remove_callers (node);
>> cgraph_node_remove_callees (node);
>> + cgraph_remove_pid (node);
>> ipa_remove_all_references (&node->ref_list);
>> ipa_remove_all_refering (&node->ref_list);
>> VEC_free (ipa_opt_pass, heap,
>
> profiling is now run as IPA pass. This means that we compute pid map and
> nothing can remove nodes before we read in the profile. So it this is
> unnecesary to hook PID array updating into cgraph_remove_node.
Right -- with the current phase ordering it should be safe -- the pid
map is created on the fly when tree-profiling is invoked and call
graph will be rebuilt immediately after that which will force the
bogus target to be reachable.
Deleting the pid table after tree profiling is the way to go.
>
>> Index: value-prof.c
>> ===================================================================
>> --- value-prof.c (revision 172721)
>> +++ value-prof.c (working copy)
>> @@ -1083,13 +1083,35 @@ init_pid_map (void)
>> /* Return cgraph node for function with pid */
>>
>> static inline struct cgraph_node*
>> -find_func_by_pid (int pid)
>> +find_func_by_pid (int pid)
>> {
>> init_pid_map ();
>>
>> + if (pid >= cgraph_max_pid)
>> + {
>> + if (flag_profile_correction)
>> + inform (DECL_SOURCE_LOCATION (current_function_decl),
>> + "Inconsistent profile: indirect call target (%d) does not
>> exist", pid);
>> + else
>> + error ("Inconsistent profile: indirect call target (%d) does not
>> exist", pid);
>> +
>> + return NULL;
>> + }
>> +
>> return pid_map [pid];
>
> PIDs are not dense, because functions might've been removed from before we get
> to ipa-profile. I would suggest making pid_map VECtor to be more consistent
> with rest of GCC sourc
Why is VEC any better in terms of density ? Are you suggesting using a
hash table?
, making init_pid_map to allocate it cleared and add also
> test that VEC_index (pid_map, pid) is not NULL in addition to the bound check
> above.
>
> Also we ought to free pids once the IPA profiling pass is done. I.e. at the
> end of tree_profling
> function.
Yes.
Thanks,
David
>
> Honza
>
