Re: [PATCH 01/40] add the cleanup checker

Tom Tromey
2013-05-17T16:15:09Z

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This patch adds the cleanup checker. This is a Python plugin for GCC
Tom> that checks some rules for cleanup handling. In particular it tries
Tom> to notice when cleanups are left dangling at the end of a function.

After seeing Keith's cleanup fix, I wondered why the checker hadn't
caught the problem. So, I investigated and ended up changing the
checker. Now it catches the bug that Keith found, plus a few more
things.

This is the new version of the checker patch.

Tom

* contrib/cleanup_check.py: New file.
* contrib/gcc-with-excheck: Add option parsing.
---
gdb/contrib/cleanup_check.py | 335 +++++++++++++++++++++++++++++++++++++++++++
gdb/contrib/gcc-with-excheck | 32 ++++-
2 files changed, 366 insertions(+), 1 deletion(-)
create mode 100644 gdb/contrib/cleanup_check.py

diff --git a/gdb/contrib/cleanup_check.py b/gdb/contrib/cleanup_check.py
new file mode 100644
index 0000000..b182d8d
--- /dev/null
+++ b/gdb/contrib/cleanup_check.py
@@ -0,0 +1,335 @@
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# This is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
+# <http://www.gnu.org/licenses/>.
+
+import gcc
+import gccutils
+import sys
+
+want_raii_info = False
+
+logging = False
+show_cfg = False
+
+def log(msg, indent=0):
+ global logging
+ if logging:
+ sys.stderr.write('%s%s\n' % (' ' * indent, msg))
+ sys.stderr.flush()
+
+def is_cleanup_type(return_type):
+ if not isinstance(return_type, gcc.PointerType):
+ return False
+ if not isinstance(return_type.dereference, gcc.RecordType):
+ return False
+ if str(return_type.dereference.name) == 'cleanup':
+ return True
+ return False
+
+def is_constructor(decl):
+ "Return True if the function DECL is a cleanup constructor; False otherwise"
+ return is_cleanup_type(decl.type.type) and (not decl.name or str(decl.name) != 'make_final_cleanup')
+
+destructor_names = set(['do_cleanups', 'discard_cleanups'])
+
+def is_destructor(decl):
+ return decl.name in destructor_names
+
+# This list is just much too long... we should probably have an
+# attribute instead.
+special_names = set(['do_final_cleanups', 'discard_final_cleanups',
+ 'save_cleanups', 'save_final_cleanups',
+ 'restore_cleanups', 'restore_final_cleanups',
+ 'exceptions_state_mc_init',
+ 'make_my_cleanup2', 'make_final_cleanup', 'all_cleanups',
+ 'save_my_cleanups', 'quit_target'])
+
+def needs_special_treatment(decl):
+ return decl.name in special_names
+
+# Sometimes we need a new placeholder object that isn't the same as
+# anything else.
+class Dummy(object):
+ def __init__(self, location):
+ self.location = location
+
+# A wrapper for a cleanup which has been assigned to a variable.
+# This holds the variable and the location.
+class Cleanup(object):
+ def __init__(self, var, location):
+ self.var = var
+ self.location = location
+
+# A class representing a master cleanup. This holds a stack of
+# cleanup objects and supports a merging operation.
+class MasterCleanup(object):
+ # Create a new MasterCleanup object. OTHER, if given, is a
+ # MasterCleanup object to copy.
+ def __init__(self, other = None):
+ # 'cleanups' is a list of cleanups. Each element is either a
+ # Dummy, for an anonymous cleanup, or a Cleanup, for a cleanup
+ # which was assigned to a variable.
+ if other is None:
+ self.cleanups = []
+ self.aliases = {}
+ else:
+ self.cleanups = other.cleanups[:]
+ self.aliases = dict(other.aliases)
+
+ def compare_vars(self, definition, argument):
+ if definition == argument:
+ return True
+ if argument in self.aliases:
+ argument = self.aliases[argument]
+ if definition in self.aliases:
+ definition = self.aliases[definition]
+ return definition == argument
+
+ def note_assignment(self, lhs, rhs):
+ log('noting assignment %s = %s' % (lhs, rhs), 4)
+ self.aliases[lhs] = rhs
+
+ # Merge with another MasterCleanup.
+ # Returns True if this resulted in a change to our state.
+ def merge(self, other):
+ # We do explicit iteration like this so we can easily
+ # update the list after the loop.
+ counter = -1
+ found_named = False
+ for counter in range(len(self.cleanups) - 1, -1, -1):
+ var = self.cleanups[counter]
+ log('merge checking %s' % var, 4)
+ # Only interested in named cleanups.
+ if isinstance(var, Dummy):
+ log('=> merge dummy', 5)
+ continue
+ # Now see if VAR is found in OTHER.
+ if other._find_var(var.var) >= 0:
+ log ('=> merge found', 5)
+ break
+ log('=>merge not found', 5)
+ found_named = True
+ if found_named and counter < len(self.cleanups) - 1:
+ log ('merging to %d' % counter, 4)
+ if counter < 0:
+ self.cleanups = []
+ else:
+ self.cleanups = self.cleanups[0:counter]
+ return True
+ # If SELF is empty but OTHER has some cleanups, then consider
+ # that a change as well.
+ if len(self.cleanups) == 0 and len(other.cleanups) > 0:
+ log('merging non-empty other', 4)
+ self.cleanups = other.cleanups[:]
+ return True
+ return False
+
+ # Push a new constructor onto our stack. LHS is the
+ # left-hand-side of the GimpleCall statement. It may be None,
+ # meaning that this constructor's value wasn't used.
+ def push(self, location, lhs):
+ if lhs is None:
+ obj = Dummy(location)
+ else:
+ obj = Cleanup(lhs, location)
+ log('pushing %s' % lhs, 4)
+ idx = self._find_var(lhs)
+ if idx >= 0:
+ gcc.permerror(location, 'reassigning to known cleanup')
+ gcc.inform(self.cleanups[idx].location,
+ 'previous assignment is here')
+ self.cleanups.append(obj)
+
+ # A helper for merge and pop that finds BACK_TO in self.cleanups,
+ # and returns the index, or -1 if not found.
+ def _find_var(self, back_to):
+ for i in range(len(self.cleanups) - 1, -1, -1):
+ if isinstance(self.cleanups[i], Dummy):
+ continue
+ if self.compare_vars(self.cleanups[i].var, back_to):
+ return i
+ return -1
+
+ # Pop constructors until we find one matching BACK_TO.
+ # This is invoked when we see a do_cleanups call.
+ def pop(self, location, back_to):
+ log('pop:', 4)
+ i = self._find_var(back_to)
+ if i >= 0:
+ self.cleanups = self.cleanups[0:i]
+ else:
+ gcc.permerror(location, 'destructor call with unknown argument')
+
+ # Check whether ARG is the current master cleanup. Return True if
+ # all is well.
+ def verify(self, location, arg):
+ log('verify %s' % arg, 4)
+ return (len(self.cleanups) > 0
+ and not isinstance(self.cleanups[0], Dummy)
+ and self.compare_vars(self.cleanups[0].var, arg))
+
+ # Check whether SELF is empty.
+ def isempty(self):
+ log('isempty: len = %d' % len(self.cleanups), 4)
+ return len(self.cleanups) == 0
+
+ # Emit informational warnings about the cleanup stack.
+ def inform(self):
+ for item in reversed(self.cleanups):
+ gcc.inform(item.location, 'leaked cleanup')
+
+class CleanupChecker:
+ def __init__(self, fun):
+ self.fun = fun
+ self.seen_edges = set()
+ self.bad_returns = set()
+
+ # This maps BB indices to a list of master cleanups for the
+ # BB.
+ self.master_cleanups = {}
+
+ # Pick a reasonable location for the basic block BB.
+ def guess_bb_location(self, bb):
+ if isinstance(bb.gimple, list):
+ for stmt in bb.gimple:
+ if stmt.loc:
+ return stmt.loc
+ return self.fun.end
+
+ # Compute the master cleanup list for BB.
+ # Modifies MASTER_CLEANUP in place.
+ def compute_master(self, bb, bb_from, master_cleanup):
+ if not isinstance(bb.gimple, list):
+ return
+ curloc = self.fun.end
+ for stmt in bb.gimple:
+ if stmt.loc:
+ curloc = stmt.loc
+ if isinstance(stmt, gcc.GimpleCall) and stmt.fndecl:
+ if is_constructor(stmt.fndecl):
+ log('saw constructor %s in bb=%d' % (str(stmt.fndecl), bb.index), 2)
+ self.cleanup_aware = True
+ master_cleanup.push(curloc, stmt.lhs)
+ elif is_destructor(stmt.fndecl):
+ if str(stmt.fndecl.name) != 'do_cleanups':
+ self.only_do_cleanups_seen = False
+ log('saw destructor %s in bb=%d, bb_from=%d, argument=%s'
+ % (str(stmt.fndecl.name), bb.index, bb_from, str(stmt.args[0])),
+ 2)
+ master_cleanup.pop(curloc, stmt.args[0])
+ elif needs_special_treatment(stmt.fndecl):
+ pass
+ # gcc.permerror(curloc, 'function needs special treatment')
+ elif isinstance(stmt, gcc.GimpleAssign):
+ if isinstance(stmt.lhs, gcc.VarDecl) and isinstance(stmt.rhs[0], gcc.VarDecl):
+ master_cleanup.note_assignment(stmt.lhs, stmt.rhs[0])
+ elif isinstance(stmt, gcc.GimpleReturn):
+ if self.is_constructor:
+ if not master_cleanup.verify(curloc, stmt.retval):
+ gcc.permerror(curloc,
+ 'constructor does not return master cleanup')
+ elif not self.is_special_constructor:
+ if not master_cleanup.isempty():
+ if curloc not in self.bad_returns:
+ gcc.permerror(curloc, 'cleanup stack is not empty at return')
+ self.bad_returns.add(curloc)
+ master_cleanup.inform()
+
+ # Traverse a basic block, updating the master cleanup information
+ # and propagating to other blocks.
+ def traverse_bbs(self, edge, bb, bb_from, entry_master):
+ log('traverse_bbs %d from %d' % (bb.index, bb_from), 1)
+
+ # Propagate the entry MasterCleanup though this block.
+ master_cleanup = MasterCleanup(entry_master)
+ self.compute_master(bb, bb_from, master_cleanup)
+
+ modified = False
+ if bb.index in self.master_cleanups:
+ # Merge the newly-computed MasterCleanup into the one we
+ # have already computed. If this resulted in a
+ # significant change, then we need to re-propagate.
+ modified = self.master_cleanups[bb.index].merge(master_cleanup)
+ else:
+ self.master_cleanups[bb.index] = master_cleanup
+ modified = True
+
+ # EDGE is None for the entry BB.
+ if edge is not None:
+ # If merging cleanups caused a change, check to see if we
+ # have a bad loop.
+ if edge in self.seen_edges:
+ # This error doesn't really help.
+ # if modified:
+ # gcc.permerror(self.guess_bb_location(bb),
+ # 'invalid cleanup use in loop')
+ return
+ self.seen_edges.add(edge)
+
+ if not modified:
+ return
+
+ # Now propagate to successor nodes.
+ for edge in bb.succs:
+ self.traverse_bbs(edge, edge.dest, bb.index, master_cleanup)
+
+ def check_cleanups(self):
+ if not self.fun.cfg or not self.fun.decl:
+ return 'ignored'
+ if is_destructor(self.fun.decl):
+ return 'destructor'
+ if needs_special_treatment(self.fun.decl):
+ return 'special'
+
+ self.is_constructor = is_constructor(self.fun.decl)
+ self.is_special_constructor = not self.is_constructor and str(self.fun.decl.name).find('with_cleanup') > -1
+ # Yuck.
+ if str(self.fun.decl.name) == 'gdb_xml_create_parser_and_cleanup_1':
+ self.is_special_constructor = True
+
+ if self.is_special_constructor:
+ gcc.inform(self.fun.start, 'function %s is a special constructor' % (self.fun.decl.name))
+
+ # If we only see do_cleanups calls, and this function is not
+ # itself a constructor, then we can convert it easily to RAII.
+ self.only_do_cleanups_seen = not self.is_constructor
+ # If we ever call a constructor, then we are "cleanup-aware".
+ self.cleanup_aware = False
+
+ entry_bb = self.fun.cfg.entry
+ master_cleanup = MasterCleanup()
+ self.traverse_bbs(None, entry_bb, -1, master_cleanup)
+ if want_raii_info and self.only_do_cleanups_seen and self.cleanup_aware:
+ gcc.inform(self.fun.decl.location,
+ 'function %s could be converted to RAII' % (self.fun.decl.name))
+ if self.is_constructor:
+ return 'constructor'
+ return 'OK'
+
+class CheckerPass(gcc.GimplePass):
+ def execute(self, fun):
+ if fun.decl:
+ log("Starting " + fun.decl.name)
+ if show_cfg:
+ dot = gccutils.cfg_to_dot(fun.cfg, fun.decl.name)
+ gccutils.invoke_dot(dot, name=fun.decl.name)
+ checker = CleanupChecker(fun)
+ what = checker.check_cleanups()
+ if fun.decl:
+ log(fun.decl.name + ': ' + what, 2)
+
+ps = CheckerPass(name = 'check-cleanups')
+# We need the cfg, but we want a relatively high-level Gimple.
+ps.register_after('cfg')
diff --git a/gdb/contrib/gcc-with-excheck b/gdb/contrib/gcc-with-excheck
index b810878..da144ac 100755
--- a/gdb/contrib/gcc-with-excheck
+++ b/gdb/contrib/gcc-with-excheck
@@ -17,12 +17,42 @@

# You must set PYTHON_PLUGIN in the environment.
# It should be the directory holding the "python.so" file.
+# Usage: gcc-with-excheck [-Xx|-Xc] [--] ARGS
+# -Xx means to invoke the exception checker.
+# -Xc means to invoke the cleanup checker.
+# -- means stop processing -X options.
+# ARGS are passed to gcc.

+GCC=${GCC:-gcc}
exdir=`dirname $0`

+pargs=
+while true; do
+ case "$1" in
+ -Xc)
+ pargs="$pargs -fplugin-arg-python-script=$exdir/cleanup_check.py"
+ ;;
+ -Xx)
+ pargs="$pargs -fplugin-arg-python-script=$exdir/excheck.py"
+ ;;
+ -X*)
+ echo "unrecognized argument $1" 1>&2
+ exit 1
+ ;;
+ --)
+ shift
+ break
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+done
+
# Recent versions of the Python plugin build two .so files in
# different directories, so we have to set this. This will be fixed
# upstream at some point.
export LD_LIBRARY_PATH=$PYTHON_PLUGIN:$PYTHON_PLUGIN/gcc-c-api

-gcc -fplugin=$PYTHON_PLUGIN/python.so -fplugin-arg-python-script=$exdir/excheck.py "$@"
+gcc -fplugin=$PYTHON_PLUGIN/python.so $pargs "$@"
--
1.8.1.4