Bisection debugging helper script

pydir/bisection-tool.py

Index: pydir/bisection-tool.py
diff --git a/pydir/bisection-tool.py b/pydir/bisection-tool.py
new file mode 100755
index 0000000000000000000000000000000000000000..91853f129a7a596e49fcf26e00d1b4fb09078162
--- /dev/null
+++ b/pydir/bisection-tool.py
@@ -0,0 +1,116 @@
+#!/usr/bin/env python2
+import argparse
+import os
+import signal
+import subprocess

[John, 2016/07/20 14:50:08]

add an empty line

[manasijm, 2016/07/20 21:15:24]

Done.

+num_tries = 0 # Counts no. of iterations
+find_all = True
+def build_command(input_cmd, include_ranges):

[John, 2016/07/20 16:11:58]

why not have build command return an object:

def Runner(object):
  def __init__(input_cmd, timeout):
    self._input_cmd = input_cmd
    self._timeout = timeout
    self._num_tries = 0

  def Run(included_ranges):
    def timeout_handler(signum, frame):
      raise Exception("timeout")

    self._num_tries += 1
    cmd = self.input_cmd
    for i in included_ranges:
      cmd += "-i {start}:{end}".format(start=i[0], end=i[1] + 1)

    p = subprocess.Popen(cmd, shell=True, cwd=None, ...)
    if self._timeout != -1:
      install signal handlers

    try:
      _, _ = p.communicate()
      if self._timeout != -1:
        remove signal handlers
    catch Exception:
      cancel p

With this interface, find_crashes' signature becomes:

def find_crashes(runner, include_ranges):

Which I personally prefer. The current signature has two parameters (cmd, and
timeout) that are closely related to each other, and would be better off
"bundled" together. It also gets rid of the num_tries global.

[manasijm, 2016/07/20 21:15:24]

Done. Thanks!

+  result = input_cmd
+  for i in include_ranges:
+    result = result + " -i " + str(i[0])+":"+str(i[1] + 1)

[Jim Stichnoth, 2016/07/20 18:32:02]

For more awesomeness, I would really like it if one could somehow control on the
command line how 'result' is formed based on include_ranges.

For example.  If you're doing szbuild.py bisection, you do as above:
  -i a1:b1 -i a2:b2 ... -i an:bn
But Subzero itself has its own bisection range syntax, e.g.:
  -verbose-focus a1:b1,a2:b2,...,an:bn

Furthermore, these ranges might need to be injected into the middle of the
command line instead of appended to the end:
  ./pydir/szbuild.py -o ./a.out -i a1:b1 ; ./a.out

[manasijm, 2016/07/20 21:49:31]

Done.

+  return result
+
+def run(cmd, include_ranges, timeout):
+  #TODO(manasijm): Figure out how to parallelize, not sure how important.
+  run_str = build_command(cmd, include_ranges)
+  print run_str
+  global num_tries
+  num_tries = num_tries + 1
+
+  class Timeout(Exception):

[John, 2016/07/20 14:50:08]

No need for this class, just

raise Exception("timeout")

[manasijm, 2016/07/20 21:15:24]

Done.

+    pass
+  def timeout_handler(signum, frame):
+    raise Timeout
+  p = subprocess.Popen(run_str, shell = True, cwd = None,
+      stdout = subprocess.PIPE, stderr = subprocess.PIPE, env = None)
+  if timeout != -1:
+    signal.signal(signal.SIGALRM, timeout_handler)
+    signal.alarm(timeout)
+  try:
+    stdout, stderr = p.communicate()
+    if timeout != -1:
+      signal.alarm(0)
+  except Timeout:
+    try:
+      os.kill(p.pid, signal.SIGKILL)
+    except OSError:
+      pass
+    print "Timeout"
+    return -9
+  print p.returncode

[John, 2016/07/20 14:50:08]

remove this print, it will get lost in the script's output.

[manasijm, 2016/07/20 21:15:24]

It seems helpful to determine how close the script is to ending.
And eyeballing which ranges are failing.
Making it harder to miss !

[John, 2016/07/20 22:23:05]

Maybe add a string describing what's going on, then? just printing the range
might be a bit confusing...

+  return p.returncode
+
+def find_crashes(cmd, include_ranges, timeout) :
+  main_range = include_ranges[0]
+  if main_range[0] == main_range[1]:

[John, 2016/07/20 14:50:08]

Are you sure this is what you want to do here? I would assume you want to run
the test program even for single-element ranges...

[manasijm, 2016/07/20 21:15:24]

Acknowledged.

+    return [main_range[0]]
+  mid = (main_range[0] + main_range[1])/2
+
+  first_half = (main_range[0], mid)
+  second_half = (mid + 1, main_range[1])
+
+  exit_code_2 = 0
+  exit_code_1 = run(cmd, [first_half] + include_ranges[1:], timeout)

[John, 2016/07/20 14:50:08]

I personally would discourage using recursion for this. I would keep a stack of
ranges, and I would keep run()ing until that stack becomes empty.

+  if find_all or exit_code_1 == 0 :
+    exit_code_2 = run(cmd, [second_half]+ include_ranges[1:], timeout)
+
+  if exit_code_1 == 0 and exit_code_2 == 0:

[John, 2016/07/20 14:50:08]

I understand what you're trying to accomplish, but I do not understand how it
accomplishes that...

[John, 2016/07/20 16:11:58]

Now I understand...

This method's signature is too complicated. You're convoluting the concepts of
"include_ranges", and "current_interval".

You probably want a signature like

def find_crashes(cmd, interval, include_ranges, timeout)

then the code would naturally show what is your bisect interval.

Then, later on, when you're trying to figure out the "a:b crashes, but a:mid and
mid:b don't" case below, you would invoke the method as follows:

find_crashes(cmd, first_half, [second_half] + include_ranges, timeout)
find_crashes(cmd, second_half, [first_half] + include_ranges, timeout)

+    # Whole range fails but both halves pass
+    # So, some conjunction of funtions cause a failure, but none individually.
+    partial_1 = find_crashes(cmd, [first_half] + [second_half] + include_ranges[1:], timeout)

[John, 2016/07/20 14:50:08]

80-col, as per the Google python style guide.

[manasijm, 2016/07/20 21:49:31]

Done.

+    # Heavy list concatenation, but this is insignificant compared to the process run times
+    if isinstance(partial_1[0], list) :

[John, 2016/07/20 14:50:08]

This is python. You should be careful about the values you return!

This is only happening because of line 68, when you concatenate two lists and
wrap the result around another list.

I propose that you modify this code so that you always ensure find_crashes
returns a list of "crashes." And you ensure that by using list's super
convenient "extend" method.

[manasijm, 2016/07/20 21:49:30]

Done.

+      partial_1 = partial_1[0] # Hack to convert [[x]] to [x] !
+    partial_2 = find_crashes(cmd, [second_half] + [first_half] + include_ranges[1:], timeout)

[John, 2016/07/20 14:50:08]

80-col

[manasijm, 2016/07/20 21:15:24]

Done.

+    if isinstance(partial_2[0], list) :
+      partial_2 = partial_2[0]
+    return [partial_1 + partial_2]
+  else :
+    result = []
+    if exit_code_1 != 0 :
+      result = find_crashes(cmd, [first_half] + include_ranges[1:], timeout)
+    if exit_code_2 != 0 :
+      result = result + find_crashes(cmd, [second_half] + include_ranges[1:], timeout)

[John, 2016/07/20 14:50:08]

80-col

[manasijm, 2016/07/20 21:15:24]

Done.

+    return result
+
+def main():
+  desc = 'Bisection debugging helper script'
+  argparser = argparse.ArgumentParser(description=desc)
+  argparser.add_argument('--cmd', required=True,
+                           dest='cmd',
+                           help='Runnable command')
+  argparser.add_argument('--start', dest='start', default=0,
+                           help='Start of initial range')
+  argparser.add_argument('--end', dest='end', default=50000,
+                           help='End of initial range')
+  argparser.add_argument('--timeout', dest='timeout', default=60,
+                           help='Timeout for each invocation of the input command')

[John, 2016/07/20 14:50:08]

80-col

[manasijm, 2016/07/20 21:15:24]

Done.

+
+  argparser.add_argument('--all', dest='all', action='store_true')
+  argparser.add_argument('--no-all', dest='all', action='store_false')
+  argparser.set_defaults(all=True)
+
+  args = argparser.parse_args()
+
+  global find_all
+  find_all = args.all

[John, 2016/07/20 16:11:58]

this is only ever use in find_crashes. I strongly suggest you pass this as a
parameter to that method instead of using a global.

Also, as the Google Python Style guide is fairly specific about which types of
data are candidates for globals, ans find_all is definitively not one of them.

[manasijm, 2016/07/20 21:15:24]

Done.

+
+  fail_list = []
+
+  initial_range = (int(args.start), int(args.end))
+  timeout = int(args.timeout)
+
+  if run(args.cmd, [initial_range], timeout) != 0 :
+    fail_list = find_crashes(args.cmd, [initial_range], timeout)
+  else :
+    print "Pass" # The whole range works, maybe check subzero build flags?
+
+  if len(fail_list) > 0 :

[John, 2016/07/20 14:50:08]

this is python. you can just

if fail_list:
  print "Failing Functions"
  for fail in fail_list:
    print fail

[manasijm, 2016/07/20 21:15:24]

Done.

+    print "Failing Functions:"
+    for fail in fail_list:
+      print fail
+  print "Number of tries : " + str(num_tries)
+  #TODO(manasijm) : Pretty print, assocoiate these numbers with filenames

[John, 2016/07/20 14:50:08]

# TODO - note the space

Also, add an empty line between this and the if below.

[manasijm, 2016/07/20 21:15:24]

Done.

+if __name__ == '__main__':
+    main()