Discussion:
[PATCH] oprofile-tests update error handling around ocount calls
(too old to reply)
Will Schmidt
2017-04-27 18:35:13 UTC
Permalink
Raw Message
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 <***@vnet.ibm.com>
---
testsuite/lib/ocount_util.exp | 9 +++++
testsuite/oprofile-ocount/ocount-run.exp | 42 +++++++++++++++++++----
testsuite/oprofile-operf/oprofile-operf-run.exp | 8 ++--
3 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/testsuite/lib/ocount_util.exp b/testsuite/lib/ocount_util.exp
index 28df1a4..292fad0 100644
--- a/testsuite/lib/ocount_util.exp
+++ b/testsuite/lib/ocount_util.exp
@@ -68,6 +68,15 @@ proc check_nonzero_count {count workload event} {
}
}

+proc check_ocount_valid_cpu_type {} {
+ set ocount_result [ local_exec "ocount " "" "" 5 ]
+ if { [ regexp "Unable to obtain cpu_type" $ocount_result ] } {
+ send "\nERROR: Ocount failed to obtain a valid cpu_type.\n"
+ return 0
+ }
+ return 1
+}
+
proc get_event_count { result symbol} {

set words [regexp -all -inline {\S+} $result]
diff --git a/testsuite/oprofile-ocount/ocount-run.exp b/testsuite/oprofile-ocount/ocount-run.exp
index 047e0ff..95e9f7a 100644
--- a/testsuite/oprofile-ocount/ocount-run.exp
+++ b/testsuite/oprofile-ocount/ocount-run.exp
@@ -90,7 +90,7 @@ proc do_test_ocount_scaling {workload_exec} {
# Version 1.0 and beyond time interval is in units of milliseconds
set run_time_interval1 [expr $interval1 * 1000]
set run_time_interval2 [expr $interval2 * 1000]
- set run_time_interval4 [expr $interval4 * 1000]
+ set run_time_interval4 [expr $interval4 * 1000]
}
}

@@ -155,9 +155,18 @@ proc do_test_ocount_scaling {workload_exec} {
set count4 [get_event_count $result4 $event]

# 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 <***@us.ibm.com>
-# 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
@@ -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
William Cohen
2017-04-28 14:51:42 UTC
Permalink
Raw Message
Post by Will Schmidt
Update the expect script to handle (and return with an error
message) when ocount returns an
"Unable to obtain cpu_type" error message/string.
ERROR: tcl error sourcing ./oprofile-ocount/ocount-run.exp.
ERROR: invalid bareword "to"
in expression "to * 2";
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.
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?
Post by Will Schmidt
# 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
#
-# 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".
Post by Will Schmidt
@@ -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
Will Schmidt
2017-04-28 15:49:23 UTC
Permalink
Raw Message
Post by William Cohen
Post by Will Schmidt
Update the expect script to handle (and return with an error
message) when ocount returns an
"Unable to obtain cpu_type" error message/string.
ERROR: tcl error sourcing ./oprofile-ocount/ocount-run.exp.
ERROR: invalid bareword "to"
in expression "to * 2";
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.
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?
I noticed the mismatched brace on one of the checks, and assumed they
were initially intended to be early exit checks.
Post by William Cohen
- check_nonzero_count $k_count $workload_exec $event]
But I think you are right that these shouldn't be, and this is just me
getting a little carried away with making changes. :-)
Post by William Cohen
The following error messages would read better if "not able find" was
changed to "not able to find".

Yup, noted. I'll make some changes and resubmit an updated version
shortly.

Thanks,
-Will
Post by William Cohen
Post by Will Schmidt
# 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
#
-# 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".
Post by Will Schmidt
@@ -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
Will Schmidt
2017-04-28 16:18:37 UTC
Permalink
Raw Message
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, also did a few cosmetic touch-ups.

Tested on ppc64/ppc64le/x86_64.

Signed-off-by: Will Schmidt <***@vnet.ibm.com>

---
lib/ocount_util.exp | 9 +++++++++
oprofile-ocount/ocount-run.exp | 11 ++++++++---
oprofile-operf/oprofile-operf-run.exp | 8 ++++----
3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/testsuite/lib/ocount_util.exp b/testsuite/lib/ocount_util.exp
index 28df1a4..292fad0 100644
--- a/testsuite/lib/ocount_util.exp
+++ b/testsuite/lib/ocount_util.exp
@@ -68,6 +68,15 @@ proc check_nonzero_count {count workload event} {
}
}

+proc check_ocount_valid_cpu_type {} {
+ set ocount_result [ local_exec "ocount " "" "" 5 ]
+ if { [ regexp "Unable to obtain cpu_type" $ocount_result ] } {
+ send "\nERROR: Ocount failed to obtain a valid cpu_type.\n"
+ return 0
+ }
+ return 1
+}
+
proc get_event_count { result symbol} {

set words [regexp -all -inline {\S+} $result]
diff --git a/testsuite/oprofile-ocount/ocount-run.exp b/testsuite/oprofile-ocoun
index 047e0ff..a0abcea 100644
--- a/testsuite/oprofile-ocount/ocount-run.exp
+++ b/testsuite/oprofile-ocount/ocount-run.exp
@@ -90,7 +90,7 @@ proc do_test_ocount_scaling {workload_exec} {
# Version 1.0 and beyond time interval is in units of milliseconds
set run_time_interval1 [expr $interval1 * 1000]
set run_time_interval2 [expr $interval2 * 1000]
- set run_time_interval4 [expr $interval4 * 1000]
+ set run_time_interval4 [expr $interval4 * 1000]
}
}

@@ -245,7 +245,7 @@ proc do_test_ocount_modes {workload_exec} {

# 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 $k_count $workload_exec $event
check_nonzero_count $u_k_count $workload_exec $event

# Check the scaling of the results
@@ -267,7 +267,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/oprofil
index 0d846cf..2409a9c 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 <***@us.ibm.com>
-# 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
@@ -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 to 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 to 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 to find a valid cpu type. Exiting.\n"
} else {
# kallsyms output check
set output_check 2
William Cohen
2017-04-28 18:20:10 UTC
Permalink
Raw Message
Post by Will Schmidt
Update the expect script to handle (and return with an error
message) when ocount returns an
"Unable to obtain cpu_type" error message/string.
ERROR: tcl error sourcing ./oprofile-ocount/ocount-run.exp.
ERROR: invalid bareword "to"
in expression "to * 2";
ERROR: Ocount failed to obtain a valid cpu_type.
While in the neighborhood, also did a few cosmetic touch-ups.
Tested on ppc64/ppc64le/x86_64.
Hi Will,

The patch looks good with the exception that the following hunk didn't apply from the saved email. The oprofile-ocount.exp has a tabs in the line being modified and the surrounding context, but the patch has it as spaces. I am not sure where the corruption of the patch occurred. Maybe your email client is converting tabs to spaces in the body of the email. Note that the previous version of the patch did have tabs and applied cleanly. I corrected whitespace and applied the patch. It should be in the upstream oprofile-testsuite now. Thanks,

-Will Cohen
Post by Will Schmidt
diff --git a/testsuite/oprofile-ocount/ocount-run.exp b/testsuite/oprofile-ocoun
index 047e0ff..a0abcea 100644
--- a/testsuite/oprofile-ocount/ocount-run.exp
+++ b/testsuite/oprofile-ocount/ocount-run.exp
@@ -90,7 +90,7 @@ proc do_test_ocount_scaling {workload_exec} {
# Version 1.0 and beyond time interval is in units of milliseconds
set run_time_interval1 [expr $interval1 * 1000]
set run_time_interval2 [expr $interval2 * 1000]
- set run_time_interval4 [expr $interval4 * 1000]
+ set run_time_interval4 [expr $interval4 * 1000]
}
}
Loading...