Re: [PATCH] oprofile-tests update error handling around ocount calls

[William C. <wc...@re...>]

On 04/27/2017 02:35 PM, Will Schmidt wrote:
> Update the expect script to handle (and return with an error
> message)  when ocount returns an
>  "Unable to obtain cpu_type" error message/string.
> 
> Subsequently turns this confusing output:
>   ERROR: tcl error sourcing ./oprofile-ocount/ocount-run.exp.
>   ERROR: invalid bareword "to"
>   in expression "to * 2";
> into this easier to understand blurb:
>   ERROR: Ocount failed to obtain a valid cpu_type.
> 
> While in the neighborhood, this also fixes up some nearby check-for-zero code.
> And does a few cosmetic touch-ups.
> 
> Tested on ppc64/ppc64le/x86_64.
> 
> Signed-off-by:  Will Schmidt <wil...@vn...

Hi Will,

A couple comments on the patch.

What is the purpose of changing the behavior below to stop the rest of the testing in this function if any of these three checks return 0?  There are checks a little later that will pass/fail the results.  Wouldn't be be better to explicitly fail these tests?

 
>      # Check the results are not zero
> -    check_nonzero_count $count1 $workload_exec $event
> -    check_nonzero_count $count2 $workload_exec $event
> -    check_nonzero_count $count4 $workload_exec $event
> +    if {[check_nonzero_count $count1 $workload_exec $event] == 0} {
> +	send "count from ocount run (1) was zero."
> +	return
> +    }
> +    if {[check_nonzero_count $count2 $workload_exec $event] == 0} {
> +	send "count from ocount run (2) was zero."
> +	return
> +    }
> +    if {[check_nonzero_count $count4 $workload_exec $event] == 0} {
> +	send "count from ocount run (4) was zero."
> +	return
> +    }
>  
>      # Check the scaling of the results
>      set test "Event $event count scales by 2, workload $workload_exec"
> @@ -244,9 +253,21 @@ proc do_test_ocount_modes {workload_exec} {
>      set u_k_count [get_event_count $user_kernel_result $event]
>  
>      # Check the counts are not zero
> -    check_nonzero_count $u_count $workload_exec $event
> -    check_nonzero_count $k_count $workload_exec $event]
> -    check_nonzero_count $u_k_count $workload_exec $event
> +    # check_nonzero_count $u_count $workload_exec $event
> +    if {[check_nonzero_count $u_count $workload_exec $event] == 0} {
> +	send "count from ocount run (user) was zero."
> +	return
> +    }
> +    # check_nonzero_count $k_count $workload_exec $event]
> +    if {[check_nonzero_count $k_count $workload_exec $event] == 0} {
> +	send "count from ocount run (kernel) was zero."
> +	return
> +    }
> +    #check_nonzero_count $u_k_count $workload_exec $event
> +    if {[check_nonzero_count $u_k_count $workload_exec $event] == 0} {
> +	send "count from ocount run (user+kernel) was zero."
> +	return
> +    }
>  
>      # Check the scaling of the results
>      set test "Event $event, user mode count plus kernel mode count matches user and kernel mode count, workload $workload_exec"
> @@ -267,7 +288,12 @@ proc ocount_run_tests {} {
>  
>      set workload_exec [compile_ocount_workload "workload_ocount/load.c"]
>  
> -send "ocount_run_tests\n"
> +    send "ocount_run_tests\n"
> +
> +    # if ocount can't determine the cpu_type properly, just return.
> +    if {[check_ocount_valid_cpu_type] == 0 } {
> +       return 0
> +    }
>  
>      # Check the brief format counting cycles gives non-zero count
>      do_test_ocount_cycle_test $workload_exec "--brief-format"
> diff --git a/testsuite/oprofile-operf/oprofile-operf-run.exp b/testsuite/oprofile-operf/oprofile-operf-run.exp
> index 0d846cf..4811072 100644
> --- a/testsuite/oprofile-operf/oprofile-operf-run.exp
> +++ b/testsuite/oprofile-operf/oprofile-operf-run.exp
> @@ -2,7 +2,7 @@
>  #   Copyright (C) 2012 Carl Love, IBM
>  #
>  # Modified by Carl Love <ca...@us...>
> -#   Copyright (C) 2013 Carl Love, IBM Corporation
> +#   Copyright (C) 2013-2017 Carl Love, IBM Corporation
>  #
>  # This file is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by

The following error messages would read better if "not able find" was changed to "not able to find".

> @@ -33,7 +33,7 @@ proc oprofile-operf_run_system_wide_tests {} {
>      set cpu [operf_cpu_type]
>  
>      if {$cpu == "bogus"} {
> -        send "Error, not able find cpu type exiting.\n"
> +        send "Error, not able find a valid cpu type. Exiting.\n"
>      } else {
>          set symbol_check 1
>          foreach spec $op_event_table($cpu) {
> @@ -54,7 +54,7 @@ proc oprofile-callgraph_run_tests {} {
>  
>      set cpu [operf_cpu_type]
>      if {$cpu == "bogus"} {
> -        send "Error, not able find cpu type exiting.\n"
> +        send "Error, not able find a valid cpu type. Exiting.\n"
>      } else {
>          # test callgraph output
>          set output_check 3
> @@ -79,7 +79,7 @@ proc oprofile-kallsyms-readable_run_tests {} {
>      set kptr_restrict [ lindex [split [local_exec $cmd "" "" 10 ] "\{\}" ] 1 ]
>  
>      if {$cpu == "bogus"} {
> -        send "Error, not able find cpu type exiting.\n"
> +        send "Error, not able find a valid cpu type. Exiting.\n"
>      } else {
>          # kallsyms output check
>          set output_check 2
> 
> 

-Will Cohen