PL286/external/toolchain-utils/binary_search_tool/MAINTENANCE

This document is for future maintainers of the binary search/bisection tools.

Authors:
  * Original Tool: asharif@, llozano@, cmtice@
  * Updates after May 2016: cburden@
  * chromeos-toolchain@

The following are good reference materials on how the tool works:
  * Ahmad's original presentation:
    https://goto.google.com/zxdfyi

  * Bisection tool update design doc:
    https://goto.google.com/zcwei

  * Bisection tool webpage:
    https://goto.google.com/ruwpyi

  * Compiler wrapper webpage:
    https://goto.google.com/xossn


TESTING:
All unit tests live under the ./test directory. However, these tests
specifically test binary_search_state.py, binary_search_perforce.py, bisect.py.
These unit tests will not test the specific logic for ChromeOS/Android
bisection. To test the ChromeOS/Android bisectors, use the common/hash_test.sh
test. This is a simple test case that just checks the hashes of files on your
file system. This means you won't have to find a specific compiler error for
the bisector to triage in order to test each bisector.

TODO:
The bisection tool (I believe) is in a fairly good state. So these are mostly
wishlist items and things that could use some improvement.

  1. Get rid of binary_search_perforce.py. This file is mostly legacy code and
     the majority of it isn't even used to bisect object files. The file was
     originally intended to bisect CLs, and binary_search_state.py just reused
     the binary searching logic from it. Maybe just extract the binary searching
     logic from binary_search_perforce.py and put it in its own module in
     cros_utils?

  2. Cleanup unit tests in ./test. These tests are a little hacked together,
     and are all under one test suite. Maybe consider organizing them across
     multiple directories.

  3. Create a "checkout setup" system for bisection. Currently if you want to
     bisect, you have to run scripts/edit sources in this repo. Ideally these
     scripts would be static, and if you wanted to bisect/make changes you would
     "checkout" or copy all the scripts to a working directory and have a unique
     working directory for each bisection. Credits to Luis for this idea =)

  4. Make all scripts relative to each other. Currently all scripts enforce the
     idea that their cwd will be ./binary_search_tool/. But it would be less
     confusing to have each script relative to each other. There's quite a few
     stackoverflow topics on how to do this best, but each one has some sort of
     downside or flaw.

  5. Overall modularize code more, especially in binary_search_state.py

DESIGN EXPLANATIONS:
Some of the design decisions are a bit difficult to understand from just reading
the code unfortunately. I will attempt to clear up the major offenders of this:

  1. common.py's argument dictionary:
     binary_search_state.py and bisect.py both have to have near identical
     arguments in order to support argument overriding in bisect.py. However
     they do have to be slightly different. Mainly, bisect.py needs to have no
     default values for arguments (so it can determine what's being overriden).

     In order to reduce huge amounts of code duplication for the argument
     building, we put argument building in common.py. That way both modules
     can reference the arguments, and they can have different configurations
     across both.

  2. Compiler wrapper:
     The compiler wrapper is called before all compiler calls. It exists to
     trick whatever build system (make, emerge, etc.) into thinking our
     bisection is just a normal build, when really we're doing some tricks.

     The biggest benefit the compiler wrapper gives is: knowing for sure which
     files are actually generated by the compiler during bisection setup, and
     potentially being able to skip compilations while triaging (speeding up the
     triaging process significantly).

  3. The weird options for the --verify, --verbose, --file_args, etc. arguments:
     Some of the arguments for the bisection tool have a weird set of options
     for the AddArgument method (nargs, const, default, StrToBool). This is so
     we can make argument overriding workable. These options allow the following
     functionality for a boolean argument (using --prune as an example):
       * --prune (prune set to True)
       * <not given> (prune set to False)
       * --prune=True (prune set to True)
       * --prune=False (prune set to False)

     The first two are easy to implement (action='store_true'), but the last two
     are why the extra weird arguments are required. Now, why would we want the
     last two? Imagine if the Android bisector set --prune=True as a default
     argument. With just the first two options above it would be impossible for
     the user to override prune and set it to False. So the user needs the
     --prune=False option. See the argparse documentation for more details.

  4. General binary searching logic/pruning logic:
     binary_search_state.py will enumerate all items into a list. The binary
     search will find the *first* bad item (starting with lowest index).
     Everything to the left of the "current" index is switched to good,
     everything to right of the "current" index is switched to bad. Once a bad
     item is found, it's put at the very end of the list.

     If prune is set, the tool will continuing searching until all bad items are
     found (instead of stopping after the first one). If the tool finds the same
     item twice, that means no more bad items exist. This is because the item
     was found, said item was put at the end of the list, and it was found
     again. Because the binary search logic finds the bad item with the lowest
     index, this means nothing in between the start of the list and the end of
     the list is bad (thus no more bad items remain).