Script checking that the observed function order matches an orderfile.

tools/cygprofile/check_orderfile.py

Index: tools/cygprofile/check_orderfile.py
diff --git a/tools/cygprofile/check_orderfile.py b/tools/cygprofile/check_orderfile.py
new file mode 100755
index 0000000000000000000000000000000000000000..3a7f4d89c3ac8e377fbda1b1c0594eea7fef4817
--- /dev/null
+++ b/tools/cygprofile/check_orderfile.py
@@ -0,0 +1,80 @@
+#!/usr/bin/python
+# Copyright 2015 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Check that symbols are ordered into a binary as they appear in the orderfile.
+"""
+
+import logging
+import sys
+
+import symbol_extractor
+import patch_orderfile

[pasko, 2015/02/02 10:00:30]

nit: alpha

[Benoit L, 2015/02/02 13:11:45]

Done.

+
+
+_MAX_WARNINGS_TO_PRINT = 200
+
+
+def _CountMisorderedSymbols(symbols, symbol_infos):
+  """Count the number of misordered symbols, and log them.
+
+  Args:
+    symbols: ordered sequence of symbols from the orderfile
+    symbol_infos: list of SymbolInfo from the binary

[pasko, 2015/02/02 10:00:30]

nit:
s/list/ordered list/
(seems tiny-ish confusing if ordering mentioned in the 1st argument, but omitted
on the 2nd)

[Benoit L, 2015/02/02 13:11:45]

Done.

+
+  Returns:
+    (misordered_pairs_count, matched_symbols, unmatched_symbols)

[pasko, 2015/02/02 10:00:30]

matched_symbols_count and unmatched_symbols_count? (yay for consistency)

[Benoit L, 2015/02/02 13:11:45]

Done.

+  """
+  name_to_symbol_info = symbol_extractor.CreateNameToSymbolInfo(symbol_infos)

[pasko, 2015/02/02 10:00:30]

here we are creating a mapping [name -> info], but we need only offsets from the
infos. So, with less abstraction, I think the code would be more straightforward
to read. Once you create a 'name_to_offset', it's pretty obvious how it's going
to be used.

You might argue that we need to print 'Unordered pair: info2, info1', but I
think (symbol, offset, previous_symbol, previous_offset) would be enough
information to be printed.

[Benoit L, 2015/02/02 13:11:45]

Yes, I think it is better to keep the whole SymbolInfo here, as it is used
elsewhere, and also for the time where we'll find something weird (for instance,
a symbol being a subset of the offsets of another one, or being 0-sized).

But I can change it. wdyt ?

[pasko, 2015/02/02 14:32:37]

OK, given your consideration, seems like a good enough balance. Thanks!

+  matched_symbol_infos = []
+  missing_count = 0
+
+  # Find the SymbolInfo matching the orderfile symbols in the binary.
+  for symbol in symbols:
+    if symbol in name_to_symbol_info:
+      matched_symbol_infos.append(name_to_symbol_info[symbol])
+    else:
+      missing_count += 1
+      if missing_count < _MAX_WARNINGS_TO_PRINT:
+        logging.warning('Symbol "%s" is in the orderfile, not in the binary' %
+                        symbol)
+  logging.warning('%d matched symbols, %d un-matched (Only the first %d '

[pasko, 2015/02/02 10:00:30]

this warning should be under:
if missing_count > 0:

[Benoit L, 2015/02/02 13:11:45]

I think that printing the number of matched symbol in all cases is still useful.
wdyt ?

[pasko, 2015/02/02 14:32:37]

ok, then let's make it log.info to avoid the sea of red herrings in the output
from the bots.

+                  'unmatched symbols are shown)' % (
+                      len(matched_symbol_infos), missing_count,
+                      _MAX_WARNINGS_TO_PRINT))
+
+  # In the order of the orderfile, find all the symbols that are at an offset
+  # smaller than their immediate predecessor, and record the pair.
+  misordered_symbol_infos = []
+  previous_symbol_info = symbol_extractor.SymbolInfo(
+      name='', offset=-1, size=0, section='')
+  for symbol_info in matched_symbol_infos:
+    if symbol_info.offset < previous_symbol_info.offset:
+      misordered_symbol_infos.append((symbol_info, previous_symbol_info))
+    previous_symbol_info = symbol_info
+
+  if len(misordered_symbol_infos) > 0:
+    for (first, second) in misordered_symbol_infos:

[pasko, 2015/02/02 10:00:30]

Iterating for it second time makes it harder to read: one has to remember how
the pairs are laid out. If it were printed as part of the previous loop, it
would have been obvious.

also, it's a little 'strange' to see (previous_symbol_info == second)

And the message could be clearer:
The symbol B (offset=X) follows symbol A (offset=Y) in the orderfile.

[Benoit L, 2015/02/02 13:11:45]

Done, but still printing the whole SymbolInfo (for now).

[pasko, 2015/02/02 14:32:37]

Acknowledged.

+      logging.warning("Unordered pair: %s - %s" % (str(first), str(second)))
+  return (len(misordered_symbol_infos), len(matched_symbol_infos),
+          missing_count)
+
+
+def main():
+  if len(sys.argv) != 4:
+    logging.error('Usage: check_orderfile.py binary orderfile threshold')

[pasko, 2015/02/02 10:00:30]

I'm about polishing again: can threshold be optional with a default==0? It does
sound slightly more convenient to _not_ worry about the threshold for triggering
an error when you run the script locally to repro a bot issue.

[Benoit L, 2015/02/02 13:11:45]

Done.

+    return 1
+  (binary_filename, orderfile_filename, threshold) = sys.argv[1:]
+  threshold = int(threshold)
+
+  symbols = patch_orderfile._GetSymbolsFromOrderfile(orderfile_filename)
+  symbol_infos = symbol_extractor.SymbolInfosFromBinary(binary_filename)
+  (misordered_pairs_count, _, _) = _CountMisorderedSymbols(

[pasko, 2015/02/02 10:00:30]

Please mention in the comment the non-obvious observation that the symbols not
being in the binary is not considered being an error because dead code could
have been eliminated at link time.

[Benoit L, 2015/02/02 13:11:45]

Done.

+      symbols, symbol_infos)
+  return misordered_pairs_count > threshold
+
+
+if __name__ == '__main__':
+  logging.basicConfig(level=logging.INFO)
+  sys.exit(main())