[PATCH 00/40] add cleanup checker and fix cleanup bugs

[PATCH 00/40] add cleanup checker and fix cleanup bugs

[Tom Tromey, Tom Tromey]

This series adds a cleanup checker and then fixes most of the bugs it
notices.

The series as a whole was built and regression-tested on x86-64 Fedora
18.  However, there are a couple of patches that I could not test;
e.g., the machoread.c patch.  I would appreciate it if someone could
try these on their machine.  The branch is archer.git tromey/cleanup-checker.

In most cases the patches are pretty obvious.  I kept them short for
ease of reading.

The cleanup checker itself is described in patch #1.  This patch also
describes some of the 

After this series, there are still some cleanup-related bugs noticed
by the checker.  One of these (in dwarf2read.c) is a false report --
the checker is not perfect and there was no nice workaround here.


The remaining errors are:

disasm.c has some spaghetti code; I didn't try to untangle it to see if
there is a bug here or not:

../../archer/gdb/disasm.c: In function ‘do_mixed_source_and_assembly’:
../../archer/gdb/disasm.c:334:16: error: destructor call with unknown argument [-fpermissive]
../../archer/gdb/disasm.c:335:16: error: destructor call with unknown argument [-fpermissive]
../../archer/gdb/disasm.c:344:1: error: cleanup stack is not empty at return [-fpermissive]
../../archer/gdb/disasm.c:212:19: note: leaked cleanup
../../archer/gdb/disasm.c:211:19: note: leaked cleanup
../../archer/gdb/disasm.c:334:16: error: destructor call with unknown argument [-fpermissive]
../../archer/gdb/disasm.c:335:16: error: destructor call with unknown argument [-fpermissive]


infrun.c bugs, reported to bugzilla:

../../archer/gdb/infrun.c: In function ‘normal_stop’:
../../archer/gdb/infrun.c:6171:1: error: cleanup stack is not empty at return [-fpermissive]
../../archer/gdb/infrun.c:5930:18: note: leaked cleanup
../../archer/gdb/infrun.c:5921:19: note: leaked cleanup
../../archer/gdb/infrun.c: In function ‘keep_going’:
../../archer/gdb/infrun.c:5728:8: error: cleanup stack is not empty at return [-fpermissive]
../../archer/gdb/infrun.c:5669:19: note: leaked cleanup


I wasn't sure what to do about this -- there are many return paths
here that don't deal with the cleanups:

../../archer/gdb/record-full.c: In function ‘record_full_wait_1’:
../../archer/gdb/record-full.c:1492:1: error: cleanup stack is not empty at return [-fpermissive]


Likewise:

../../archer/gdb/record-btrace.c: In function ‘record_btrace_insn_history’:
../../archer/gdb/record-btrace.c:336:1: error: cleanup stack is not empty at return [-fpermissive]
../../archer/gdb/record-btrace.c:274:17: note: leaked cleanup
../../archer/gdb/record-btrace.c: In function ‘record_btrace_call_history’:
../../archer/gdb/record-btrace.c:565:1: error: cleanup stack is not empty at return [-fpermissive]
../../archer/gdb/record-btrace.c:503:17: note: leaked cleanup

[PATCH 01/40] add the cleanup checker

[Tom Tromey, Tom Tromey]

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

It does this by applying a few simple rules.

First, it understands that a function whose return type is "struct
cleanup *" is a "cleanup constructor".  Such functions are expected to
return the first cleanup that they make.

Then, it has the notion of a "master cleanup".  The checker keeps a
stack of all cleanups made in a basic block.  The first element is
pushed on the stack is the master cleanup -- the one that must later
be passed to either do_cleanups or discard_cleanups.

It is not perfect -- some constructs confuse it.  So, part of this
series rewrites some code in gdb so that it is analyzable.  I'll note
these spots and you can decide whether or not this is a good idea.

This patch also changes gcc-with-excheck to give it options.  Now you
must use either -Xc (for the cleanup checker) or -Xx (for the
exception checker).

	* contrib/cleanup_check.py: New file.
	* contrib/gcc-with-excheck: Add option parsing.
---
 gdb/contrib/cleanup_check.py | 312 +++++++++++++++++++++++++++++++++++++++++++
 gdb/contrib/gcc-with-excheck |  32 ++++-
 2 files changed, 343 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..17b81f6
--- /dev/null
+++ b/gdb/contrib/cleanup_check.py
@@ -0,0 +1,312 @@
+#   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
+
+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 = []
+        else:
+            self.cleanups = other.cleanups[:]
+
+    def find_base_ssaname(self, arg):
+        while isinstance(arg, gcc.SsaName):
+            if not hasattr(arg, 'def_stmt'):
+                break
+            stmt = arg.def_stmt
+            if isinstance(stmt, gcc.GimpleAssign) and stmt.exprcode is gcc.VarDecl:
+                arg = stmt.rhs[0]
+            else:
+                break
+        return arg
+
+    def compare_vars(self, loc, definition, argument, bb_from):
+        log('[comparing <%s> with <%s>]' % (str(definition), str(argument)), 4)
+        if definition == argument:
+            return True
+        if not isinstance(argument, gcc.SsaName):
+            log('[not an ssaname]', 4)
+            return False
+
+        stmt = argument.def_stmt
+
+        # Maybe ARGUMENT is of the form ARGUMENT = NAME.
+        # In that case, check NAME.
+        while isinstance(stmt, gcc.GimpleAssign) and stmt.exprcode is gcc.VarDecl:
+            argument = stmt.rhs[0]
+            if definition == argument:
+                return True
+            log ('[stripped to %s]' % argument, 4)
+            if not hasattr(argument, 'def_stmt'):
+                return False
+            stmt = argument.def_stmt
+
+        if not isinstance(stmt, gcc.GimplePhi):
+            log('[definition not a Phi: %s]' % repr(stmt), 4)
+            return False
+
+        for (expr, edge) in stmt.args:
+            log('examining edge from %d (bb_from = %d)' % (edge.src.index, bb_from), 4)
+            if edge.src.index == bb_from:
+                argument = expr
+                log('replacing with %s' % str(argument), 4)
+                break
+        else:
+            # gcc.permerror(loc, 'internal error in plugin')
+            return False
+
+        definition = self.find_base_ssaname(definition)
+        argument = self.find_base_ssaname(argument)
+
+        log ('comparing (%s) %s with (%s) %s => %s'
+             % (type(definition), definition, type(argument), argument, definition == argument), 4)
+        return definition == argument
+
+    # 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)
+        self.cleanups.append(obj)
+
+    # 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, bb_from):
+        log('pop:', 4)
+        for i in range(len(self.cleanups) - 1, -1, -1):
+            if isinstance(self.cleanups[i], Dummy):
+                continue
+            if self.compare_vars(location, self.cleanups[i].var,
+                                 back_to, bb_from):
+                self.cleanups = self.cleanups[0:i]
+                return
+        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, bb_from):
+        return (len(self.cleanups) > 0
+                and not isinstance(self.cleanups[0], Dummy)
+                and self.compare_vars(location, self.cleanups[0].var,
+                                      arg, bb_from))
+
+    # 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()
+
+    # 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):
+        log('entering BB %d' % bb.index, 1)
+        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], bb_from)
+                elif needs_special_treatment(stmt.fndecl):
+                    pass
+                    # gcc.permerror(curloc, 'function needs special treatment')
+            elif isinstance(stmt, gcc.GimpleReturn):
+                if self.is_constructor:
+                    if not master_cleanup.verify(curloc, stmt.retval, bb_from):
+                        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):
+        modified = False
+        # if bb.index in self.master_cleanups:
+        #     # FIXME do some checking
+        #     return
+
+        # EDGE is None for the entry BB.
+        if edge is not None:
+            # FIXME
+            if edge in self.seen_edges:
+                return
+
+            # If merging cleanups caused a change, check to see if we
+            # have a bad loop.
+            if modified and edge in self.seen_edges:
+                gcc.permerror(self.guess_bb_location(bb),
+                              'invalid cleanup use in loop')
+            self.seen_edges.add(edge)
+
+        self.master_cleanups[bb.index] = MasterCleanup(entry_master)
+        self.compute_master(bb, bb_from, self.master_cleanups[bb.index])
+
+        # Now propagate to successor nodes.
+        for edge in bb.succs:
+            self.traverse_bbs(edge, edge.dest, bb.index,
+                              self.master_cleanups[bb.index])
+
+    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
+
+        # This maps BB indices to the BB's master cleanup.
+        self.master_cleanups = {}
+
+        entry_bb = self.fun.cfg.entry
+        self.master_cleanups[entry_bb.index] = MasterCleanup()
+        self.traverse_bbs(None, entry_bb, -1,
+                          self.master_cleanups[entry_bb.index])
+        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)
+            # 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('release_ssa')
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

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

[Tom Tromey]

>>>>> "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

[PATCH 02/40] some cleanup checker fixes

[Tom Tromey, Tom Tromey]

Fix some bugs pointed out by the cleanup checker.  This one just fixes
some simple CLI reports, where CLI commands know that their caller
will do cleanups.  This an older style with few instances, so it is
simpler to fix them up than to teach the checker about it.

	* cli/cli-cmds.c (cd_command, alias_command): Call do_cleanups.
	* cli/cli-dump.c (restore_binary_file): Call do_cleanups.
	* interps.c (interpreter_exec_cmd): Call do_cleanups.
	* source.c (show_substitute_path_command): Call do_cleanups.
	(unset_substitute_path_command, set_substitute_path_command):
	Likewise.
	* symfile.c (load_command): Call do_cleanups.
---
 gdb/cli/cli-cmds.c | 10 ++++++++--
 gdb/cli/cli-dump.c |  3 ++-
 gdb/interps.c      |  5 ++++-
 gdb/source.c       | 15 ++++++++++++---
 gdb/symfile.c      |  4 ++++
 5 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index cf88b6d..6ee7673 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -357,6 +357,7 @@ cd_command (char *dir, int from_tty)
   /* Found something other than leading repetitions of "/..".  */
   int found_real_path;
   char *p;
+  struct cleanup *cleanup;
 
   /* If the new directory is absolute, repeat is a no-op; if relative,
      repeat might be useful but is more likely to be a mistake.  */
@@ -366,7 +367,7 @@ cd_command (char *dir, int from_tty)
     dir = "~";
 
   dir = tilde_expand (dir);
-  make_cleanup (xfree, dir);
+  cleanup = make_cleanup (xfree, dir);
 
   if (chdir (dir) < 0)
     perror_with_name (dir);
@@ -450,6 +451,8 @@ cd_command (char *dir, int from_tty)
 
   if (from_tty)
     pwd_command ((char *) 0, 1);
+
+  do_cleanups (cleanup);
 }
 \f
 /* Show the current value of the 'script-extension' option.  */
@@ -1327,13 +1330,14 @@ alias_command (char *args, int from_tty)
   char *args2, *equals, *alias, *command;
   char **alias_argv, **command_argv;
   dyn_string_t alias_dyn_string, command_dyn_string;
+  struct cleanup *cleanup;
   static const char usage[] = N_("Usage: alias [-a] [--] ALIAS = COMMAND");
 
   if (args == NULL || strchr (args, '=') == NULL)
     error (_(usage));
 
   args2 = xstrdup (args);
-  make_cleanup (xfree, args2);
+  cleanup = make_cleanup (xfree, args2);
   equals = strchr (args2, '=');
   *equals = '\0';
   alias_argv = gdb_buildargv (args2);
@@ -1440,6 +1444,8 @@ alias_command (char *args, int from_tty)
 		     command_argv[command_argc - 1],
 		     class_alias, abbrev_flag, c_command->prefixlist);
     }
+
+  do_cleanups (cleanup);
 }
 \f
 /* Print a list of files and line numbers which a user may choose from
diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
index 529e0a0..208916c 100644
--- a/gdb/cli/cli-dump.c
+++ b/gdb/cli/cli-dump.c
@@ -508,6 +508,7 @@ restore_section_callback (bfd *ibfd, asection *isec, void *args)
 static void
 restore_binary_file (char *filename, struct callback_data *data)
 {
+  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
   FILE *file = fopen_with_cleanup (filename, FOPEN_RB);
   gdb_byte *buf;
   long len;
@@ -553,7 +554,7 @@ restore_binary_file (char *filename, struct callback_data *data)
   len = target_write_memory (data->load_start + data->load_offset, buf, len);
   if (len != 0)
     warning (_("restore: memory write failed (%s)."), safe_strerror (len));
-  return;
+  do_cleanups (cleanup);
 }
 
 static void
diff --git a/gdb/interps.c b/gdb/interps.c
index bd23118..25500d6 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -409,12 +409,13 @@ interpreter_exec_cmd (char *args, int from_tty)
   unsigned int nrules;
   unsigned int i;
   int old_quiet, use_quiet;
+  struct cleanup *cleanup;
 
   if (args == NULL)
     error_no_arg (_("interpreter-exec command"));
 
   prules = gdb_buildargv (args);
-  make_cleanup_freeargv (prules);
+  cleanup = make_cleanup_freeargv (prules);
 
   nrules = 0;
   for (trule = prules; *trule != NULL; trule++)
@@ -452,6 +453,8 @@ interpreter_exec_cmd (char *args, int from_tty)
   interp_set (old_interp, 0);
   interp_set_quiet (interp_to_use, use_quiet);
   interp_set_quiet (old_interp, old_quiet);
+
+  do_cleanups (cleanup);
 }
 
 /* List the possible interpreters which could complete the given text.  */
diff --git a/gdb/source.c b/gdb/source.c
index 710b90c..161a6c4 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1840,9 +1840,10 @@ show_substitute_path_command (char *args, int from_tty)
   struct substitute_path_rule *rule = substitute_path_rules;
   char **argv;
   char *from = NULL;
+  struct cleanup *cleanup;
   
   argv = gdb_buildargv (args);
-  make_cleanup_freeargv (argv);
+  cleanup = make_cleanup_freeargv (argv);
 
   /* We expect zero or one argument.  */
 
@@ -1866,6 +1867,8 @@ show_substitute_path_command (char *args, int from_tty)
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
+
+  do_cleanups (cleanup);
 }
 
 /* Implement the "unset substitute-path" command.  */
@@ -1877,10 +1880,11 @@ unset_substitute_path_command (char *args, int from_tty)
   char **argv = gdb_buildargv (args);
   char *from = NULL;
   int rule_found = 0;
+  struct cleanup *cleanup;
 
   /* This function takes either 0 or 1 argument.  */
 
-  make_cleanup_freeargv (argv);
+  cleanup = make_cleanup_freeargv (argv);
   if (argv != NULL && argv[0] != NULL && argv[1] != NULL)
     error (_("Incorrect usage, too many arguments in command"));
 
@@ -1918,6 +1922,8 @@ unset_substitute_path_command (char *args, int from_tty)
     error (_("No substitution rule defined for `%s'"), from);
 
   forget_cached_source_info ();
+
+  do_cleanups (cleanup);
 }
 
 /* Add a new source path substitution rule.  */
@@ -1927,9 +1933,10 @@ set_substitute_path_command (char *args, int from_tty)
 {
   char **argv;
   struct substitute_path_rule *rule;
+  struct cleanup *cleanup;
   
   argv = gdb_buildargv (args);
-  make_cleanup_freeargv (argv);
+  cleanup = make_cleanup_freeargv (argv);
 
   if (argv == NULL || argv[0] == NULL || argv [1] == NULL)
     error (_("Incorrect usage, too few arguments in command"));
@@ -1956,6 +1963,8 @@ set_substitute_path_command (char *args, int from_tty)
 
   add_substitute_path_rule (argv[0], argv[1]);
   forget_cached_source_info ();
+
+  do_cleanups (cleanup);
 }
 
 \f
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 32d6ad0..f93d857 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1813,6 +1813,8 @@ find_sym_fns (bfd *abfd)
 static void
 load_command (char *arg, int from_tty)
 {
+  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+
   dont_repeat ();
 
   /* The user might be reloading because the binary has changed.  Take
@@ -1862,6 +1864,8 @@ load_command (char *arg, int from_tty)
   /* After re-loading the executable, we don't really know which
      overlays are mapped any more.  */
   overlay_cache_invalid = 1;
+
+  do_cleanups (cleanup);
 }
 
 /* This version of "load" should be usable for any target.  Currently
-- 
1.8.1.4

[PATCH 04/40] fix cleanups in som_symtab_read

[Tom Tromey, Tom Tromey]

This fixes som_symtab_read not to leak cleanups.

	* somread.c (som_symtab_read): Call do_cleanups.
---
 gdb/somread.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/somread.c b/gdb/somread.c
index db6c4d4..6c6cc14 100644
--- a/gdb/somread.c
+++ b/gdb/somread.c
@@ -46,6 +46,7 @@ static void
 som_symtab_read (bfd *abfd, struct objfile *objfile,
 		 struct section_offsets *section_offsets)
 {
+  struct cleanup *cleanup;
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   unsigned int number_of_symbols;
   int val, dynamic;
@@ -65,7 +66,7 @@ som_symtab_read (bfd *abfd, struct objfile *objfile,
      We avoid using alloca because the memory size could be so large
      that we could hit the stack size limit.  */
   buf = xmalloc (symsize * number_of_symbols);
-  make_cleanup (xfree, buf);
+  cleanup = make_cleanup (xfree, buf);
   bfd_seek (abfd, obj_som_sym_filepos (abfd), SEEK_SET);
   val = bfd_bread (buf, symsize * number_of_symbols, abfd);
   if (val != symsize * number_of_symbols)
@@ -316,6 +317,8 @@ som_symtab_read (bfd *abfd, struct objfile *objfile,
 								  section),
 					   objfile);
     }
+
+  do_cleanups (cleanup);
 }
 
 /* Scan and build partial symbols for a symbol file.
-- 
1.8.1.4

[PATCH 05/40] fix cleanup buglet in ada-lang.c

[Tom Tromey, Tom Tromey]

This fixes ada_make_symbol_completion_list not to leak cleanups.

	* ada-lang.c (ada_make_symbol_completion_list): Call do_cleanups.
---
 gdb/ada-lang.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 3510750..314f4ca 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -5841,6 +5841,7 @@ ada_make_symbol_completion_list (const char *text0, const char *word,
   struct block *b, *surrounding_static_block = 0;
   int i;
   struct block_iterator iter;
+  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
   gdb_assert (code == TYPE_CODE_UNDEF);
 
@@ -5941,6 +5942,7 @@ ada_make_symbol_completion_list (const char *text0, const char *word,
     }
   }
 
+  do_cleanups (cleanup);
   return completions;
 }
 
-- 
1.8.1.4

Re: [PATCH 05/40] fix cleanup buglet in ada-lang.c

[Tom Tromey]

Tom> This fixes ada_make_symbol_completion_list not to leak cleanups.
Tom> 	* ada-lang.c (ada_make_symbol_completion_list): Call do_cleanups.

Note that this patch was committed already by Joel:

2013-05-16  Joel Brobecker  <brobecker@adacore.com>

	* ada-lang.c (ada_make_symbol_completion_list): Make sure
	all cleanups are done before returning from this function.

So, it isn't in this series.

Tom

[PATCH 03/40] fix print_command_1

[Tom Tromey, Tom Tromey]

This is a stylistic patch to make it so the checker can analyze
print_command_1.  This amounts to installing an outer cleanup and
unconditionally invoking it.

	* printcmd.c (print_command_1): Unconditionally call do_cleanups.
---
 gdb/printcmd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 2cc33d0..7beb334 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -936,11 +936,10 @@ static void
 print_command_1 (const char *exp, int voidprint)
 {
   struct expression *expr;
-  struct cleanup *old_chain = 0;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   char format = 0;
   struct value *val;
   struct format_data fmt;
-  int cleanup = 0;
 
   if (exp && *exp == '/')
     {
@@ -960,8 +959,7 @@ print_command_1 (const char *exp, int voidprint)
   if (exp && *exp)
     {
       expr = parse_expression (exp);
-      old_chain = make_cleanup (free_current_contents, &expr);
-      cleanup = 1;
+      make_cleanup (free_current_contents, &expr);
       val = evaluate_expression (expr);
     }
   else
@@ -996,8 +994,7 @@ print_command_1 (const char *exp, int voidprint)
 	annotate_value_end ();
     }
 
-  if (cleanup)
-    do_cleanups (old_chain);
+  do_cleanups (old_chain);
 }
 
 static void
-- 
1.8.1.4

[PATCH 06/40] remove erroneous return from setup_user_args

[Tom Tromey, Tom Tromey]

This fixes setup_user_args to drop a useless and confusing "return".

	* cli/cli-script.c (setup_user_args): Don't return after error.
---
 gdb/cli/cli-script.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 3b24799..43fd479 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -689,11 +689,8 @@ setup_user_args (char *p)
       int bsquote = 0;
 
       if (arg_count >= MAXUSERARGS)
-	{
-	  error (_("user defined function may only have %d arguments."),
-		 MAXUSERARGS);
-	  return old_chain;
-	}
+	error (_("user defined function may only have %d arguments."),
+	       MAXUSERARGS);
 
       /* Strip whitespace.  */
       while (*p == ' ' || *p == '\t')
-- 
1.8.1.4

[PATCH 08/40] fix up cleanup handling in internal_vproblem

[Tom Tromey, Tom Tromey]

internal_vproblem can return, so this introduces proper cleanup
handling there.  Otherwise it may make a cleanup that is leaked.

	* utils.c (internal_vproblem): Call do_cleanups.
---
 gdb/utils.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/utils.c b/gdb/utils.c
index 218faed..c25dadf 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -713,6 +713,7 @@ internal_vproblem (struct internal_problem *problem,
   int quit_p;
   int dump_core_p;
   char *reason;
+  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
   /* Don't allow infinite error/warning recursion.  */
   {
@@ -821,6 +822,7 @@ internal_vproblem (struct internal_problem *problem,
     }
 
   dejavu = 0;
+  do_cleanups (cleanup);
 }
 
 static struct internal_problem internal_error_problem = {
-- 
1.8.1.4

[PATCH 09/40] cleanup fixes for remote-mips.c

[Tom Tromey, Tom Tromey]

remote-mips.c has a few 'return's where cleanups are not run.

	* remote-mips.c (mips_exit_debug): Call do_cleanups on all
	return paths.
	(mips_initialize): Likewise.
	(common_open): Call do_cleanups.
---
 gdb/remote-mips.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index 3b65b59..1619622 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -1386,13 +1386,19 @@ mips_exit_debug (void)
       mips_request ('x', 0, 0, NULL, mips_receive_wait, NULL);
       mips_need_reply = 0;
       if (!mips_expect (" break!"))
-	return -1;
+	{
+	  do_cleanups (old_cleanups);
+	  return -1;
+	}
     }
   else
     mips_request ('x', 0, 0, &err, mips_receive_wait, NULL);
 
   if (!mips_expect (mips_monitor_prompt))
-    return -1;
+    {
+      do_cleanups (old_cleanups);
+      return -1;
+    }
 
   do_cleanups (old_cleanups);
 
@@ -1406,7 +1412,7 @@ static void
 mips_initialize (void)
 {
   int err;
-  struct cleanup *old_cleanups = make_cleanup (mips_initialize_cleanups, NULL);
+  struct cleanup *old_cleanups;
   int j;
 
   /* What is this code doing here?  I don't see any way it can happen, and
@@ -1419,6 +1425,8 @@ mips_initialize (void)
       return;
     }
 
+  old_cleanups = make_cleanup (mips_initialize_cleanups, NULL);
+
   mips_wait_flag = 0;
   mips_initializing = 1;
 
@@ -1543,6 +1551,7 @@ common_open (struct target_ops *ops, char *name, int from_tty,
   char *remote_name = 0;
   char *local_name = 0;
   char **argv;
+  struct cleanup *cleanup;
 
   if (name == 0)
     error (_("\
@@ -1558,7 +1567,7 @@ seen from the board via TFTP, specify that name as the third parameter.\n"));
   /* Parse the serial port name, the optional TFTP name, and the
      optional local TFTP name.  */
   argv = gdb_buildargv (name);
-  make_cleanup_freeargv (argv);
+  cleanup = make_cleanup_freeargv (argv);
 
   serial_port_name = xstrdup (argv[0]);
   if (argv[1])			/* Remote TFTP name specified?  */
@@ -1655,6 +1664,8 @@ seen from the board via TFTP, specify that name as the third parameter.\n"));
   stop_pc = regcache_read_pc (get_current_regcache ());
   print_stack_frame (get_selected_frame (NULL), 0, SRC_AND_LOC);
   xfree (serial_port_name);
+
+  do_cleanups (cleanup);
 }
 
 /* Open a connection to an IDT board.  */
-- 
1.8.1.4

[PATCH 18/40] fix py-breakpoint.c

[Tom Tromey, Tom Tromey]

One return path in bppy_get_commands was missing a do_cleanups call.

	* python/py-breakpoint.c (bppy_get_commands): Call do_cleanups
	along all return paths.
---
 gdb/python/py-breakpoint.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 5e5f9b3..d099892 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -489,7 +489,11 @@ bppy_get_commands (PyObject *self, void *closure)
       print_command_lines (current_uiout, breakpoint_commands (bp), 0);
     }
   ui_out_redirect (current_uiout, NULL);
-  GDB_PY_HANDLE_EXCEPTION (except);
+  if (except.reason < 0)
+    {
+      do_cleanups (chain);
+      GDB_PY_HANDLE_EXCEPTION (except);
+    }
 
   cmdstr = ui_file_xstrdup (string_file, &length);
   make_cleanup (xfree, cmdstr);
-- 
1.8.1.4

[PATCH 17/40] simplify cli-logging.c for analysis

[Tom Tromey, Tom Tromey]

This is another stylistic patch.  It changes cli-logging.c to be
analyzable by the checker, again following the method of adding an
outer cleanup and unconditionally calling do_cleanups.

	* cli/cli-logging.c (set_logging_redirect): Unconditionally
	call do_cleanups.
---
 gdb/cli/cli-logging.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 9f1477d..5704179 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -79,7 +79,7 @@ static struct ui_file *logging_no_redirect_file;
 static void
 set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
 {
-  struct cleanup *cleanups = NULL;
+  struct cleanup *cleanups;
   struct ui_file *output, *new_logging_no_redirect_file;
   struct ui_out *uiout = current_uiout;
 
@@ -88,13 +88,15 @@ set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
       || (logging_redirect == 0 && logging_no_redirect_file != NULL))
     return;
 
+  cleanups = make_cleanup (null_cleanup, NULL);
+
   if (logging_redirect != 0)
     {
       gdb_assert (logging_no_redirect_file != NULL);
 
       /* ui_out_redirect still has not been called for next
 	 gdb_stdout.  */
-      cleanups = make_cleanup_ui_file_delete (gdb_stdout);
+      make_cleanup_ui_file_delete (gdb_stdout);
 
       output = logging_no_redirect_file;
       new_logging_no_redirect_file = NULL;
@@ -139,8 +141,7 @@ set_logging_redirect (char *args, int from_tty, struct cmd_list_element *c)
       || ui_out_redirect (uiout, output) < 0)
     warning (_("Current output protocol does not support redirection"));
 
-  if (logging_redirect != 0)
-    do_cleanups (cleanups);
+  do_cleanups (cleanups);
 }
 
 static void
-- 
1.8.1.4

[PATCH 24/40] fix mipsread.c

[Tom Tromey, Tom Tromey]

Some code in mipsread.c could leak cleanups along some return paths.

	* mipsread.c (read_alphacoff_dynamic_symtab): Call do_cleanups
	along all return paths.
---
 gdb/mipsread.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/gdb/mipsread.c b/gdb/mipsread.c
index e9f0402..b425780b 100644
--- a/gdb/mipsread.c
+++ b/gdb/mipsread.c
@@ -227,16 +227,28 @@ read_alphacoff_dynamic_symtab (struct section_offsets *section_offsets,
 
   if (!bfd_get_section_contents (abfd, si.sym_sect, sym_secptr,
 				 (file_ptr) 0, sym_secsize))
-    return;
+    {
+      do_cleanups (cleanups);
+      return;
+    }
   if (!bfd_get_section_contents (abfd, si.str_sect, str_secptr,
 				 (file_ptr) 0, str_secsize))
-    return;
+    {
+      do_cleanups (cleanups);
+      return;
+    }
   if (!bfd_get_section_contents (abfd, si.dyninfo_sect, dyninfo_secptr,
 				 (file_ptr) 0, dyninfo_secsize))
-    return;
+    {
+      do_cleanups (cleanups);
+      return;
+    }
   if (!bfd_get_section_contents (abfd, si.got_sect, got_secptr,
 				 (file_ptr) 0, got_secsize))
-    return;
+    {
+      do_cleanups (cleanups);
+      return;
+    }
 
   /* Find the number of local GOT entries and the index for the
      first dynamic symbol in the GOT.  */
@@ -264,7 +276,10 @@ read_alphacoff_dynamic_symtab (struct section_offsets *section_offsets,
 	}
     }
   if (dt_mips_local_gotno < 0 || dt_mips_gotsym < 0)
-    return;
+    {
+      do_cleanups (cleanups);
+      return;
+    }
 
   /* Scan all dynamic symbols and enter them into the minimal symbol
      table if appropriate.  */
-- 
1.8.1.4

[PATCH 10/40] cleanup fixes for inf-ptrace.c

[Tom Tromey, Tom Tromey]

This is one of the stylistic patches.  The code here in inf-ptrace.c
is not incorrect, but it is in a style that the cleanup checker cannot
handle.  This patch changes it to a simpler style, following the usual
method of introducing an unconditional "outer" cleanup.

	* inf-ptrace.c (inf_ptrace_create_inferior): Unconditionally
	call discard_cleanups.
	(inf_ptrace_attach): Likewise.
---
 gdb/inf-ptrace.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 27bbf4d..046e0ce 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -122,20 +122,19 @@ inf_ptrace_create_inferior (struct target_ops *ops,
   /* Do not change either targets above or the same target if already present.
      The reason is the target stack is shared across multiple inferiors.  */
   int ops_already_pushed = target_is_pushed (ops);
-  struct cleanup *back_to = NULL;
+  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
 
   if (! ops_already_pushed)
     {
       /* Clear possible core file with its process_stratum.  */
       push_target (ops);
-      back_to = make_cleanup_unpush_target (ops);
+      make_cleanup_unpush_target (ops);
     }
 
   pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
 		       NULL, NULL, NULL);
 
-  if (! ops_already_pushed)
-    discard_cleanups (back_to);
+  discard_cleanups (back_to);
 
   /* START_INFERIOR_TRAPS_EXPECTED is defined in inferior.h, and will
      be 1 or 2 depending on whether we're starting without or with a
@@ -196,7 +195,7 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
   /* Do not change either targets above or the same target if already present.
      The reason is the target stack is shared across multiple inferiors.  */
   int ops_already_pushed = target_is_pushed (ops);
-  struct cleanup *back_to = NULL;
+  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
 
   pid = parse_pid_to_attach (args);
 
@@ -208,7 +207,7 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
       /* target_pid_to_str already uses the target.  Also clear possible core
 	 file with its process_stratum.  */
       push_target (ops);
-      back_to = make_cleanup_unpush_target (ops);
+      make_cleanup_unpush_target (ops);
     }
 
   if (from_tty)
@@ -243,8 +242,7 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
      target, it should decorate the ptid later with more info.  */
   add_thread_silent (inferior_ptid);
 
-  if (! ops_already_pushed)
-    discard_cleanups (back_to);
+  discard_cleanups (back_to);
 }
 
 #ifdef PT_GET_PROCESS_STATE
-- 
1.8.1.4

[PATCH 12/40] fix cleanup handling in m32r_load

[Tom Tromey, Tom Tromey]

m32r_load is missing a call to do_cleanups along one return path.

	* m32r-rom.c (m32r_load): Call do_cleanups at all returns.
---
 gdb/m32r-rom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/m32r-rom.c b/gdb/m32r-rom.c
index aab59f0..5b27b60 100644
--- a/gdb/m32r-rom.c
+++ b/gdb/m32r-rom.c
@@ -167,6 +167,7 @@ m32r_load (char *filename, int from_tty)
   if (!(catch_errors (m32r_load_1, abfd, "Load aborted!\n", RETURN_MASK_ALL)))
     {
       monitor_printf ("q\n");
+      do_cleanups (cleanup);
       return;
     }
 #endif
-- 
1.8.1.4

[PATCH 13/40] fix cleanup handling in macho_symfile_read

[Tom Tromey, Tom Tromey]

macho_symfile_read leaks a cleanup by assigning to 'back_to' too late.

	* machoread.c (macho_symfile_read): Assign first cleanup to
	'back_to'.
---
 gdb/machoread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/machoread.c b/gdb/machoread.c
index 9877f07..d294960 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -871,10 +871,10 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
           struct cleanup *back_to;
 
 	  symbol_table = (asymbol **) xmalloc (storage_needed);
-	  make_cleanup (xfree, symbol_table);
+	  back_to = make_cleanup (xfree, symbol_table);
 
           init_minimal_symbol_collection ();
-          back_to = make_cleanup_discard_minimal_symbols ();
+          make_cleanup_discard_minimal_symbols ();
 
 	  symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);
 
-- 
1.8.1.4

Re: [PATCH 13/40] fix cleanup handling in macho_symfile_read

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3608 bytes --]

Hi Tom,

On Thu, May 09, 2013 at 12:51:10PM -0600, Tom Tromey wrote:
> macho_symfile_read leaks a cleanup by assigning to 'back_to' too late.
> 
> 	* machoread.c (macho_symfile_read): Assign first cleanup to
> 	'back_to'.

This patch exposes what looks like a fairly awful memory interdependency
between macho_symfile_read and macho_symfile_read_all_oso via
macho_symtab_read/macho_register_oso.  By doing this, which looks
completely correct, we're actually causing the memory to be released
earlier, which then creates dangling pointers, thus causing a SEGV
during symbol reading. Below is the analysis I submitted internally.

I am planning on looking deeper at how to make things cleaner, and
avoid the use of a global variable for that. But my day is almost
over, and I think it's a significant-enough regression that it's
worth reverting until I get this fixed, which should hopefully be
a day or two at most (a side-effect of the current approach is that
it is harder to follow compared to local variables)! Attached is
the revert, tested on x86_64-darwin - so sorry for undoing this
work, but I will fix things ASAP.

The analysis file at AdaCore:
| Which fixes a cleanup leak, but in the process exposes a memory
| management problem. More precisely, macho_symfile_read allocates
| an array of asymbol's, and installs a cleanup:
| 
|           symbol_table = (asymbol **) xmalloc (storage_needed);
|           back_to = make_cleanup (xfree, symbol_table);
| 
| The old code was not saving the cleanup handle returned by
| make_cleanup (thus the leak). The problem is that the code
| then calls macho_symtab_read before calling do_cleanups:
| 
|           macho_symtab_read (objfile, symcount, symbol_table);
| 
|           install_minimal_symbols (objfile);
|           do_cleanups (back_to);
| 
| This is when things go wrong, because macho_symtab_read registers
| the symbols from SYMBOL_TABLE via macho_register_oso.  The latter
| just stacks the list of OSOs to be processed later, via the call
| to macho_symfile_read_all_oso at the end of macho_symfile_read.
| At that point in time, the cleanup has been done and the data saved
| by macho_symtab_read is now stale.
| 
| Attached is a patch that extends the lifetime of all cleanups to
| the entire function, rather than just the sub-block. This side-steps
| the issue mostly successfully, but I still get a failure in
| segv_on_reload:
| 
|     (gdb) run
|     `/private/var/folders/zz/zyxvpxvq6csfxvn_n00001zc0000gv/T/gdb-TS-LD8OGZ/D109
| +-003__segv_on_reload/g' has changed; re-reading symbols.
|     gdb(66092) malloc: *** error for object 0x7fdf41000ed0: pointer being freed
| +was not allocated
| 
| I think the memory management model between the various functions
| should be more explicit anyway, rather than depend on a global
| variable. So I will work on that.

For the record, you patch was:

> --- a/gdb/machoread.c
> +++ b/gdb/machoread.c
> @@ -871,10 +871,10 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
>            struct cleanup *back_to;
>  
>  	  symbol_table = (asymbol **) xmalloc (storage_needed);
> -	  make_cleanup (xfree, symbol_table);
> +	  back_to = make_cleanup (xfree, symbol_table);
>  
>            init_minimal_symbol_collection ();
> -          back_to = make_cleanup_discard_minimal_symbols ();
> +          make_cleanup_discard_minimal_symbols ();
>  
>  	  symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);

This is what I checked in:

gdb/Changelog:

        Revert:
        * machoread.c (macho_symfile_read): Assign first cleanup to
        'back_to'.

-- 
Joel

[-- Attachment #2: 0001-Revert-fix-cleanup-handling-in-macho_symfile_read.patch --]
[-- Type: text/x-diff, Size: 1678 bytes --]

From 47ca19ca1b982adb3e63f00f6e132942a2e624dc Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 3 Jun 2013 17:19:51 +0400
Subject: [PATCH] Revert "fix cleanup handling in macho_symfile_read"

This patch indirectly causes a SEGV by creating a dangling pointer.
Reverting this patch while working on a clearer memory management
method for this part of the code.

gdb/Changelog:

        Revert:
        * machoread.c (macho_symfile_read): Assign first cleanup to
        'back_to'.
---
 gdb/ChangeLog   |    6 ++++++
 gdb/machoread.c |    4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8649045..906cb06 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2013-06-03  Joel Brobecker  <brobecker@adacore.com>
+
+	Revert (indirectly causes a SIGSEGV):
+	* machoread.c (macho_symfile_read): Assign first cleanup to
+	'back_to'.
+
 2013-06-03  Yao Qi  <yao@codesourcery.com>
 
 	* mi/mi-cmd-var.c (mi_no_values, mi_simple_values): Move to
diff --git a/gdb/machoread.c b/gdb/machoread.c
index d294960..9877f07 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -871,10 +871,10 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
           struct cleanup *back_to;
 
 	  symbol_table = (asymbol **) xmalloc (storage_needed);
-	  back_to = make_cleanup (xfree, symbol_table);
+	  make_cleanup (xfree, symbol_table);
 
           init_minimal_symbol_collection ();
-          make_cleanup_discard_minimal_symbols ();
+          back_to = make_cleanup_discard_minimal_symbols ();
 
 	  symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);
 
-- 
1.7.10.4

Re: [PATCH 13/40] fix cleanup handling in macho_symfile_read

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> This is what I checked in:
Joel> gdb/Changelog:
Joel>         Revert:
Joel>         * machoread.c (macho_symfile_read): Assign first cleanup to
Joel>         'back_to'.

I'm sorry about that.
I'll file a bug for it.

Tom

Re: [PATCH 13/40] fix cleanup handling in macho_symfile_read

[Joel Brobecker]

> Joel> This is what I checked in:
> Joel> gdb/Changelog:
> Joel>         Revert:
> Joel>         * machoread.c (macho_symfile_read): Assign first cleanup to
> Joel>         'back_to'.
> 
> I'm sorry about that.
> I'll file a bug for it.

You really shouldn't be. I just spend half a day not making any progress
on this, because the problem does not reproduce outside of the testing
environment, and I cannot debug outside of using printf statements :-(.
It looks like either a double-free, or a memory corruption, with a
slight hunch towards a memory corruption, because I do not see where
the memory at this address got allocated.

If you already opend a PR, you can assign it to me. I think this one
is going to hurt :-(.

-- 
Joel

[patch/Darwin] Fix cleanup leak in machoread.c:macho_symfile_read

[Joel Brobecker]

Hello,

This is a followup on...

    http://www.sourceware.org/ml/gdb-patches/2013-06/msg00014.html

... where a patch from Tom making a correct fix for a cleanup leak
in macho_symfile_read (symbol_table)...

          symbol_table = (asymbol **) xmalloc (storage_needed);
          make_cleanup (xfree, symbol_table);

... exposed a series of latent bugs... The triggering patch was
temporarily reverted, and this patch fixes the leak again.

Unfortunately, fixing the leak alone triggers a crash which occurs
while loading the symbols from an executable:

    % gdb
    (gdb) file g_exe
    [SIGSEGV]

The crash is caused by the fact that performing the cleanup
right after the call to macho_symtab_read, as currently done,
is too early.

Indeed, references to this symbol_table get saved in the oso_vector
global during the call to macho_symtab_read via calls to
macho_register_oso, and those references then get accessed
later on, when processing all the OSOs that got pushed (see
call to macho_symfile_read_all_oso).

This patch prevents this by using one single cleanup queue for
the entire function, rather than having additional separate
cleanup queues (Eg: for the handling of the minimal symbols),
thus preventing the premature free'ing of the minimal_symbols
array.

Secondly, this patch takes this opportunity for avoiding the use
of the oso_vector global, thus making it simpler to track its
lifetime.

gdb/ChangeLog:

        * machoread.c (oso_vector): Delete this global.
        (macho_register_oso): Add new parameter "oso_vector_ptr".
        Use it instead of the "oso_vector" global.
        (macho_symtab_read, macho_symfile_read_all_oso): Likewise.
        (macho_symfile_read): Use a local oso_vector, to be free'ed
        at the end of this function, in place of the old "oso_vector"
        global.  Update various function calls accordingly.  Use one
        single cleanup chain for the entire function.

Tested on x86_64-darwin.  Applying this patch also uncovers
another latent bug, tentatively fixed by:

    [RFA/DWARF] do not use dwarf2_per_objfile in dwarf2_per_objfile_free.
    http://www.sourceware.org/ml/gdb-patches/2013-06/msg00231.html

Without it, we'll get a crashing while unloading the symbols
before loading new symbols. For instance:

    (gdb) file g1
    (gdb) file g2
    [SIGSEGV]

I will therefore wait for approval of the RFA/DWARF before applying
this patch.

Thanks,
-- 
Joel

---
 gdb/machoread.c |   50 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/gdb/machoread.c b/gdb/machoread.c
index 7a4f0c3..8d45f6f 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -64,10 +64,8 @@ typedef struct oso_el
 }
 oso_el;
 
-/* Vector of object files to be read after the executable.  This is one
-   global variable but it's life-time is the one of macho_symfile_read.  */
+/* Vector of object files to be read after the executable.  */
 DEF_VEC_O (oso_el);
-static VEC (oso_el) *oso_vector;
 
 static void
 macho_new_init (struct objfile *objfile)
@@ -83,7 +81,8 @@ macho_symfile_init (struct objfile *objfile)
 /*  Add a new OSO to the vector of OSO to load.  */
 
 static void
-macho_register_oso (struct objfile *objfile,
+macho_register_oso (VEC (oso_el) **oso_vector_ptr,
+		    struct objfile *objfile,
                     asymbol **oso_sym, asymbol **end_sym,
                     unsigned int nbr_syms)
 {
@@ -94,7 +93,7 @@ macho_register_oso (struct objfile *objfile,
   el.oso_sym = oso_sym;
   el.end_sym = end_sym;
   el.nbr_syms = nbr_syms;
-  VEC_safe_push (oso_el, oso_vector, &el);
+  VEC_safe_push (oso_el, *oso_vector_ptr, &el);
 }
 
 /* Add symbol SYM to the minimal symbol table of OBJFILE.  */
@@ -175,7 +174,8 @@ macho_symtab_add_minsym (struct objfile *objfile, const asymbol *sym)
 
 static void
 macho_symtab_read (struct objfile *objfile,
-		   long number_of_symbols, asymbol **symbol_table)
+		   long number_of_symbols, asymbol **symbol_table,
+		   VEC (oso_el) **oso_vector_ptr)
 {
   long i;
   const asymbol *dir_so = NULL;
@@ -299,7 +299,8 @@ macho_symtab_read (struct objfile *objfile,
                 {
                   /* End of file.  */
                   if (state == S_DWARF_FILE)
-                    macho_register_oso (objfile, oso_file, symbol_table + i,
+                    macho_register_oso (oso_vector_ptr, objfile,
+					oso_file, symbol_table + i,
                                         nbr_syms);
                   state = S_NO_SO;
                 }
@@ -642,19 +643,20 @@ macho_add_oso_symfile (oso_el *oso, bfd *abfd,
   do_cleanups (cleanup);
 }
 
-/* Read symbols from the vector of oso files.  */
+/* Read symbols from the vector of oso files.
+
+   Note that this function sorts OSO_VECTOR_PTR.  */
 
 static void
-macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
+macho_symfile_read_all_oso (VEC (oso_el) **oso_vector_ptr,
+			    struct objfile *main_objfile,
+			    int symfile_flags)
 {
   int ix;
-  VEC (oso_el) *vec;
+  VEC (oso_el) *vec = *oso_vector_ptr;
   oso_el *oso;
   struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
-  vec = oso_vector;
-  oso_vector = NULL;
-
   /* Sort oso by name so that files from libraries are gathered.  */
   qsort (VEC_address (oso_el, vec), VEC_length (oso_el, vec),
          sizeof (oso_el), oso_el_compare_name);
@@ -773,7 +775,6 @@ macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
         }
     }
 
-  VEC_free (oso_el, vec);
   do_cleanups (cleanup);
 }
 
@@ -850,6 +851,8 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
   CORE_ADDR offset;
   long storage_needed;
   bfd *dsym_bfd;
+  VEC (oso_el) *oso_vector = NULL;
+  struct cleanup *old_chain = make_cleanup (VEC_cleanup (oso_el), &oso_vector);
 
   /* Get symbols from the symbol table only if the file is an executable.
      The symbol table of object files is not relocated and is expected to
@@ -867,13 +870,12 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 	{
 	  asymbol **symbol_table;
 	  long symcount;
-          struct cleanup *back_to;
 
 	  symbol_table = (asymbol **) xmalloc (storage_needed);
 	  make_cleanup (xfree, symbol_table);
 
           init_minimal_symbol_collection ();
-          back_to = make_cleanup_discard_minimal_symbols ();
+          make_cleanup_discard_minimal_symbols ();
 
 	  symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);
 
@@ -882,10 +884,9 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 		   bfd_get_filename (objfile->obfd),
 		   bfd_errmsg (bfd_get_error ()));
 
-	  macho_symtab_read (objfile, symcount, symbol_table);
+	  macho_symtab_read (objfile, symcount, symbol_table, &oso_vector);
 
           install_minimal_symbols (objfile);
-          do_cleanups (back_to);
 	}
 
       /* Try to read .eh_frame / .debug_frame.  */
@@ -901,15 +902,10 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 	  int ix;
 	  oso_el *oso;
           struct bfd_section *asect, *dsect;
-	  struct cleanup *cleanup;
 
 	  if (mach_o_debug_level > 0)
 	    printf_unfiltered (_("dsym file found\n"));
 
-	  /* Remove oso.  They won't be used.  */
-	  VEC_free (oso_el, oso_vector);
-	  oso_vector = NULL;
-
           /* Set dsym section size.  */
           for (asect = objfile->obfd->sections, dsect = dsym_bfd->sections;
                asect && dsect;
@@ -922,11 +918,11 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
             }
 
 	  /* Add the dsym file as a separate file.  */
-	  cleanup = make_cleanup_bfd_unref (dsym_bfd);
+	  make_cleanup_bfd_unref (dsym_bfd);
           symbol_file_add_separate (dsym_bfd, symfile_flags, objfile);
-	  do_cleanups (cleanup);
 
 	  /* Don't try to read dwarf2 from main file or shared libraries.  */
+	  do_cleanups (old_chain);
           return;
 	}
     }
@@ -939,7 +935,9 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 
   /* Then the oso.  */
   if (oso_vector != NULL)
-    macho_symfile_read_all_oso (objfile, symfile_flags);
+    macho_symfile_read_all_oso (&oso_vector, objfile, symfile_flags);
+
+  do_cleanups (old_chain);
 }
 
 static bfd_byte *
-- 
1.7.10.4

Re: [patch/Darwin] Fix cleanup leak in machoread.c:macho_symfile_read

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> This patch prevents this by using one single cleanup queue for
Joel> the entire function, rather than having additional separate
Joel> cleanup queues (Eg: for the handling of the minimal symbols),
Joel> thus preventing the premature free'ing of the minimal_symbols
Joel> array.

Joel> Secondly, this patch takes this opportunity for avoiding the use
Joel> of the oso_vector global, thus making it simpler to track its
Joel> lifetime.

Thanks for doing this, Joel.

Tom

[PATCH 23/40] fix one bug in symfile.c

[Tom Tromey, Tom Tromey]

find_separate_debug_file could leak a cleanup along some return paths.

	* symfile.c (find_separate_debug_file): Call do_cleanups
	along all return paths.
---
 gdb/symfile.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index f93d857..7781dfa 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1469,7 +1469,10 @@ find_separate_debug_file (const char *dir,
       strcat (debugfile, debuglink);
 
       if (separate_debug_file_exists (debugfile, crc32, objfile))
-	return debugfile;
+	{
+	  do_cleanups (back_to);
+	  return debugfile;
+	}
 
       /* If the file is in the sysroot, try using its base path in the
 	 global debugfile directory.  */
@@ -1484,7 +1487,10 @@ find_separate_debug_file (const char *dir,
 	  strcat (debugfile, debuglink);
 
 	  if (separate_debug_file_exists (debugfile, crc32, objfile))
-	    return debugfile;
+	    {
+	      do_cleanups (back_to);
+	      return debugfile;
+	    }
 	}
     }
 
-- 
1.8.1.4

[PATCH 20/40] fix py-prettyprint.c

[Tom Tromey, Tom Tromey]

print_children, in py-prettyprint.c, could call do_cleanups twice on
the same cleanup.

	* python/py-prettyprint.c (print_children): Remove extra
	do_cleanups call.
---
 gdb/python/py-prettyprint.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index b50e757..bdb1ac5 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -629,8 +629,6 @@ print_children (PyObject *printer, const char *hint,
 	  local_opts.addressprint = 0;
 	  val_print_string (type, encoding, addr, (int) length, stream,
 			    &local_opts);
-
-	  do_cleanups (inner_cleanup);
 	}
       else if (gdbpy_is_string (py_v))
 	{
-- 
1.8.1.4

[PATCH 22/40] fix symtab.c

[Tom Tromey, Tom Tromey]

search_symbols had some bad code resulting in a cleanup being both
discarded and run.

	* symtab.c (search_symbols): Introduce a null cleanup for
	'retval_chain'.
---
 gdb/symtab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index def556b..9040035 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3529,7 +3529,7 @@ search_symbols (char *regexp, enum search_domain kind,
 						&datum);
   }
 
-  retval_chain = old_chain;
+  retval_chain = make_cleanup (null_cleanup, NULL);
 
   /* Here, we search through the minimal symbol tables for functions
      and variables that match, and force their symbols to be read.
-- 
1.8.1.4

[PATCH 11/40] fix list_available_thread_groups

[Tom Tromey, Tom Tromey]

list_available_thread_groups, in mi-main.c, leaks a cleanup.
This changes it to call do_cleanups.

	* mi/mi-main.c (list_available_thread_groups): Call do_cleanups.
---
 gdb/mi/mi-main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 94fda8f..f11b769 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -678,6 +678,7 @@ list_available_thread_groups (VEC (int) *ids, int recurse)
   struct osdata_item *item;
   int ix_items;
   struct ui_out *uiout = current_uiout;
+  struct cleanup *cleanup;
 
   /* This keeps a map from integer (pid) to VEC (struct osdata_item *)*
      The vector contains information about all threads for the given pid.
@@ -687,7 +688,7 @@ list_available_thread_groups (VEC (int) *ids, int recurse)
 
   /* get_osdata will throw if it cannot return data.  */
   data = get_osdata ("processes");
-  make_cleanup_osdata_free (data);
+  cleanup = make_cleanup_osdata_free (data);
 
   if (recurse)
     {
@@ -790,6 +791,8 @@ list_available_thread_groups (VEC (int) *ids, int recurse)
 
       do_cleanups (back_to);
     }
+
+  do_cleanups (cleanup);
 }
 
 void
-- 
1.8.1.4

[PATCH 15/40] make a cleanup unconditionally in tracepoint.c

[Tom Tromey, Tom Tromey]

This is another cosmetic patch.  It introduces an "outer" cleanup in
trace_dump_command and arranges to unconditionally call do_cleanups.
This lets the checker analyze the function.

	* tracepoint.c (trace_dump_command): Unconditionally call
	do_cleanups.
---
 gdb/tracepoint.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index e2b21af..a774b19 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2929,7 +2929,7 @@ trace_dump_command (char *args, int from_tty)
   struct bp_location *loc;
   char *default_collect_line = NULL;
   struct command_line *actions, *default_collect_action = NULL;
-  struct cleanup *old_chain = NULL;
+  struct cleanup *old_chain;
 
   if (tracepoint_number == -1)
     {
@@ -2937,6 +2937,7 @@ trace_dump_command (char *args, int from_tty)
       return;
     }
 
+  old_chain = make_cleanup (null_cleanup, NULL);
   t = get_tracepoint (tracepoint_number);
 
   if (t == NULL)
@@ -2970,7 +2971,7 @@ trace_dump_command (char *args, int from_tty)
   if (*default_collect)
     {
       default_collect_line = xstrprintf ("collect %s", default_collect);
-      old_chain = make_cleanup (xfree, default_collect_line);
+      make_cleanup (xfree, default_collect_line);
       validate_actionline (default_collect_line, &t->base);
       default_collect_action = xmalloc (sizeof (struct command_line));
       make_cleanup (xfree, default_collect_action);
@@ -2981,8 +2982,7 @@ trace_dump_command (char *args, int from_tty)
 
   trace_dump_actions (actions, 0, stepping_frame, from_tty);
 
-  if (*default_collect)
-    do_cleanups (old_chain);
+  do_cleanups (old_chain);
 }
 
 /* Encode a piece of a tracepoint's source-level definition in a form
-- 
1.8.1.4

Re: [PATCH 15/40] make a cleanup unconditionally in tracepoint.c

[Yao Qi]

On 05/10/2013 02:51 AM, Tom Tromey wrote:
> This is another cosmetic patch.  It introduces an "outer" cleanup in
> trace_dump_command and arranges to unconditionally call do_cleanups.
> This lets the checker analyze the function.
>
> 	* tracepoint.c (trace_dump_command): Unconditionally call
> 	do_cleanups.

Tom,
this patch looks right to me.  I'd like to know your plan on these 
patches, will you commit them recently?  I ask this because I have a 
patch touches the same place as yours, and I've picked up this patch in 
my tree.

-- 
Yao (齐尧)

Re: [PATCH 15/40] make a cleanup unconditionally in tracepoint.c

[Tom Tromey]

Yao> this patch looks right to me.  I'd like to know your plan on these
Yao> patches, will you commit them recently?  I ask this because I have a
Yao> patch touches the same place as yours, and I've picked up this patch
Yao> in my tree.

I was planning to check them in soon.  I can do it today.

I'm still not sure about patch #36.

Tom

[PATCH 16/40] fix varobj.c

[Tom Tromey, Tom Tromey]

c_value_of_root is missing a call to do_cleanups at one return.
This fixes the problem by removing that return and letting control
fall through.

	* varobj.c (c_value_of_root): Call do_cleanups along all
	return paths.
---
 gdb/varobj.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 467e59a..cf1a11f 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -3456,13 +3456,11 @@ c_value_of_root (struct varobj **var_handle)
 	{
 	  new_val = evaluate_expression (var->root->exp);
 	}
-
-      return new_val;
     }
 
   do_cleanups (back_to);
 
-  return NULL;
+  return new_val;
 }
 
 static struct value *
-- 
1.8.1.4

[PATCH 19/40] fix py-frame.c

[Tom Tromey, Tom Tromey]

A couple return paths in frapy_read_var were missing do_cleanups calls.

	* python/py-frame.c (frapy_read_var): Call do_cleanups along
	all return paths.
---
 gdb/python/py-frame.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index e2eb9c5..33d0bd0 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -456,6 +456,7 @@ frapy_read_var (PyObject *self, PyObject *args)
 	    {
 	      PyErr_SetString (PyExc_RuntimeError,
 			       _("Second argument must be block."));
+	      do_cleanups (cleanup);
 	      return NULL;
 	    }
 	}
@@ -468,7 +469,11 @@ frapy_read_var (PyObject *self, PyObject *args)
 	    block = get_frame_block (frame, NULL);
 	  var = lookup_symbol (var_name, block, VAR_DOMAIN, NULL);
 	}
-      GDB_PY_HANDLE_EXCEPTION (except);
+      if (except.reason < 0)
+	{
+	  do_cleanups (cleanup);
+	  GDB_PY_HANDLE_EXCEPTION (except);
+	}
 
       if (!var)
 	{
-- 
1.8.1.4

[PATCH 14/40] fix two buglets in breakpoint.c

[Tom Tromey, Tom Tromey]

First, output_thread_groups leaks a cleanup along one return path.

Second, parse_cmd_to_aexpr could return without running its cleanups,
if there was an exception in a TRY_CATCH.

	* breakpoint.c (output_thread_groups): Call do_cleanups along
	all return paths.
	(parse_cmd_to_aexpr): Call do_cleanups earlier.
---
 gdb/breakpoint.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f4f9325..56398a0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2248,6 +2248,8 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
 			  fpieces, nargs, argvec);
     }
 
+  do_cleanups (old_cleanups);
+
   if (ex.reason < 0)
     {
       /* If we got here, it means the command could not be parsed to a valid
@@ -2256,8 +2258,6 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
       return NULL;
     }
 
-  do_cleanups (old_cleanups);
-
   /* We have a valid agent expression, return it.  */
   return aexpr;
 }
@@ -5814,7 +5814,10 @@ output_thread_groups (struct ui_out *uiout,
   /* For backward compatibility, don't display inferiors in CLI unless
      there are several.  Always display them for MI. */
   if (!is_mi && mi_only)
-    return;
+    {
+      do_cleanups (back_to);
+      return;
+    }
 
   for (i = 0; VEC_iterate (int, inf_num, i, inf); ++i)
     {
-- 
1.8.1.4

Re: [PATCH 14/40] fix two buglets in breakpoint.c

[Yao Qi]

On 05/10/2013 02:51 AM, Tom Tromey wrote:
> @@ -5814,7 +5814,10 @@ output_thread_groups (struct ui_out *uiout,
>     /* For backward compatibility, don't display inferiors in CLI unless
>        there are several.  Always display them for MI. */
>     if (!is_mi && mi_only)
> -    return;
> +    {
> +      do_cleanups (back_to);
> +      return;
> +    }

Tom,
A nit here, we may call 'make_cleanup_ui_out_list_begin_end' after this 
condition checking, so we don't need 'do_cleanups' here.

-- 
Yao (齐尧)

Re: [PATCH 14/40] fix two buglets in breakpoint.c

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

>> @@ -5814,7 +5814,10 @@ output_thread_groups (struct ui_out *uiout,
>> /* For backward compatibility, don't display inferiors in CLI unless
>> there are several.  Always display them for MI. */
>> if (!is_mi && mi_only)
>> -    return;
>> +    {
>> +      do_cleanups (back_to);
>> +      return;
>> +    }

Yao> A nit here, we may call 'make_cleanup_ui_out_list_begin_end' after
Yao> this condition checking, so we don't need 'do_cleanups' here.

That isn't what I see in trunk:

static void
output_thread_groups (struct ui_out *uiout,
		      const char *field_name,
		      VEC(int) *inf_num,
		      int mi_only)
{
  struct cleanup *back_to = make_cleanup_ui_out_list_begin_end (uiout,
								field_name);
  int is_mi = ui_out_is_mi_like_p (uiout);
  int inf;
  int i;

  /* For backward compatibility, don't display inferiors in CLI unless
     there are several.  Always display them for MI. */
  if (!is_mi && mi_only)
    return;


That "return" leaves a cleanup dangling.

Tom

Re: [PATCH 14/40] fix two buglets in breakpoint.c

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> A nit here, we may call 'make_cleanup_ui_out_list_begin_end' after
Yao> this condition checking, so we don't need 'do_cleanups' here.

It occurred to me much later that you meant that I could lower
the make_cleanup_ui_out_list_begin_end to after the initial return.
I'm sorry for misreading that.
I'll make the change as you suggest.

Tom

Re: [PATCH 14/40] fix two buglets in breakpoint.c

[Yao Qi]

On 05/14/2013 12:44 AM, Tom Tromey wrote:
> It occurred to me much later that you meant that I could lower
> the make_cleanup_ui_out_list_begin_end to after the initial return.

Right, that is what I meant.  Sorry for the confusing description.

> I'm sorry for misreading that.
> I'll make the change as you suggest.

-- 
Yao (齐尧)

Re: [PATCH 14/40] fix two buglets in breakpoint.c

[Tom Tromey]

Tom> It occurred to me much later that you meant that I could lower
Tom> the make_cleanup_ui_out_list_begin_end to after the initial return.
Tom> I'm sorry for misreading that.
Tom> I'll make the change as you suggest.

Here is the new version of this patch.

Tom

	* breakpoint.c (output_thread_groups, parse_cmd_to_aexpr): Call
	do_cleanups earlier.
---
 gdb/breakpoint.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f4f9325..2f4d4fc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2248,6 +2248,8 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
 			  fpieces, nargs, argvec);
     }
 
+  do_cleanups (old_cleanups);
+
   if (ex.reason < 0)
     {
       /* If we got here, it means the command could not be parsed to a valid
@@ -2256,8 +2258,6 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
       return NULL;
     }
 
-  do_cleanups (old_cleanups);
-
   /* We have a valid agent expression, return it.  */
   return aexpr;
 }
@@ -5805,8 +5805,7 @@ output_thread_groups (struct ui_out *uiout,
 		      VEC(int) *inf_num,
 		      int mi_only)
 {
-  struct cleanup *back_to = make_cleanup_ui_out_list_begin_end (uiout,
-								field_name);
+  struct cleanup *back_to;
   int is_mi = ui_out_is_mi_like_p (uiout);
   int inf;
   int i;
@@ -5816,6 +5815,8 @@ output_thread_groups (struct ui_out *uiout,
   if (!is_mi && mi_only)
     return;
 
+  back_to = make_cleanup_ui_out_list_begin_end (uiout, field_name);
+
   for (i = 0; VEC_iterate (int, inf_num, i, inf); ++i)
     {
       if (is_mi)
-- 
1.8.1.4

[PATCH 21/40] fix py-value.c

[Tom Tromey, Tom Tromey]

Some code in py-value.c could exit a loop without running some
cleanups made in the loop.

	* python/py-value.c (valpy_binop): Call do_cleanups before
	exiting loop.
---
 gdb/python/py-value.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 2cbb0cb..da74982 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -776,11 +776,17 @@ valpy_binop (enum valpy_opcode opcode, PyObject *self, PyObject *other)
 	 a gdb.Value object and need to convert it from python as well.  */
       arg1 = convert_value_from_python (self);
       if (arg1 == NULL)
-	break;
+	{
+	  do_cleanups (cleanup);
+	  break;
+	}
 
       arg2 = convert_value_from_python (other);
       if (arg2 == NULL)
-	break;
+	{
+	  do_cleanups (cleanup);
+	  break;
+	}
 
       switch (opcode)
 	{
-- 
1.8.1.4

[PATCH 26/40] fix top.c

[Tom Tromey, Tom Tromey]

execute_command can leak a cleanup along one return path.

	* top.c (execute_command): Discard 'cleanup_if_error' cleanups.
---
 gdb/top.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/top.c b/gdb/top.c
index 480b67e..8ac756f 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -425,6 +425,7 @@ execute_command (char *p, int from_tty)
   if (p == NULL)
     {
       do_cleanups (cleanup);
+      discard_cleanups (cleanup_if_error);
       return;
     }
 
-- 
1.8.1.4

[PATCH 33/40] fix mi-cmd-stack.c

[Tom Tromey, Tom Tromey]

mi-cmd-stack.d had a conditional cleanup, "cleanup_tuple" that
confused the checker.  However, there was no need for this, since it
was only used via do_cleanups at the end of the function, just before
another call to do_cleanups.

So, while this is a stylistic patch for the checker, I also consider
it a generic improvement for readers of the code.

	* mi/mi-cmd-stack.c (list_arg_or_local): Remove
	"cleanup_tuple".
---
 gdb/mi/mi-cmd-stack.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 062cd5d..4de78ac 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -250,7 +250,6 @@ list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
 		   enum print_values values)
 {
   struct cleanup *old_chain;
-  struct cleanup *cleanup_tuple = NULL;
   struct ui_out *uiout = current_uiout;
   struct ui_file *stb;
 
@@ -268,7 +267,7 @@ list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
 	          && (arg->val || arg->error)));
 
   if (values != PRINT_NO_VALUES || what == all)
-    cleanup_tuple = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+    make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
 
   fputs_filtered (SYMBOL_PRINT_NAME (arg->sym), stb);
   if (arg->entry_kind == print_entry_values_only)
@@ -311,8 +310,6 @@ list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
       ui_out_field_stream (uiout, "value", stb);
     }
 
-  if (values != PRINT_NO_VALUES || what == all)
-    do_cleanups (cleanup_tuple);
   do_cleanups (old_chain);
 }
 
-- 
1.8.1.4

[PATCH 29/40] fix in solib-aix.c

[Tom Tromey, Tom Tromey]

solib_aix_bfd_open has an early "return" that doesn't run cleanups.
This fixes the problem by dropping the null_cleanup and using a later
cleanup as the master cleanup for the function.

	* solib-aix.c (solib_aix_bfd_open): Don't use a null cleanup
	for 'cleanup'; instead use a later one.
---
 gdb/solib-aix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index 9fa5de9..95b1ce1 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -643,7 +643,7 @@ solib_aix_bfd_open (char *pathname)
   int filename_len;
   char *member_name;
   bfd *archive_bfd, *object_bfd;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+  struct cleanup *cleanup;
 
   if (pathname[path_len - 1] != ')')
     return solib_bfd_open (pathname);
@@ -661,7 +661,7 @@ solib_aix_bfd_open (char *pathname)
   filename_len = sep - pathname;
 
   filename = xstrprintf ("%.*s", filename_len, pathname);
-  make_cleanup (xfree, filename);
+  cleanup = make_cleanup (xfree, filename);
   member_name = xstrprintf ("%.*s", path_len - filename_len - 2, sep + 1);
   make_cleanup (xfree, member_name);
 
-- 
1.8.1.4

[PATCH 31/40] fix source.c

[Tom Tromey, Tom Tromey]

find_and_open_source can leak a cleanup.

	* source.c (find_and_open_source): Call do_cleanups.
---
 gdb/source.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/source.c b/gdb/source.c
index 161a6c4..71b6d53 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -985,6 +985,7 @@ find_and_open_source (const char *filename,
   char *path = source_path;
   const char *p;
   int result;
+  struct cleanup *cleanup;
 
   /* Quick way out if we already know its full name.  */
 
@@ -1016,6 +1017,8 @@ find_and_open_source (const char *filename,
       *fullname = NULL;
     }
 
+  cleanup = make_cleanup (null_cleanup, NULL);
+
   if (dirname != NULL)
     {
       /* If necessary, rewrite the compilation directory name according
@@ -1072,6 +1075,7 @@ find_and_open_source (const char *filename,
 	result = openp (path, OPF_SEARCH_IN_PATH, p, OPEN_MODE, fullname);
     }
 
+  do_cleanups (cleanup);
   return result;
 }
 
-- 
1.8.1.4

[PATCH 28/40] use explicit returns to avoid checker confusion

[Tom Tromey, Tom Tromey]

The checker does not understand the idiom

if (except.reason < 0) {
   do_cleanups (whatever);
   GDB_PY_HANDLE_EXCEPTION (except);
}

because it doesn't realize that the nested 'if' actually has the same
condition.

This fixes instances of this to be more explicit.

	* python/py-breakpoint.c (bppy_get_commands): Use
	explicit, unconditional return.
	* python/py-frame.c (frapy_read_var): Likewise.
	* python/python.c (gdbpy_decode_line): Likewise.
---
 gdb/python/py-breakpoint.c | 2 +-
 gdb/python/py-frame.c      | 2 +-
 gdb/python/python.c        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index d099892..fd13811 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -492,7 +492,7 @@ bppy_get_commands (PyObject *self, void *closure)
   if (except.reason < 0)
     {
       do_cleanups (chain);
-      GDB_PY_HANDLE_EXCEPTION (except);
+      return gdbpy_convert_exception (except);
     }
 
   cmdstr = ui_file_xstrdup (string_file, &length);
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 33d0bd0..10a50ea 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -472,7 +472,7 @@ frapy_read_var (PyObject *self, PyObject *args)
       if (except.reason < 0)
 	{
 	  do_cleanups (cleanup);
-	  GDB_PY_HANDLE_EXCEPTION (except);
+	  return gdbpy_convert_exception (except);
 	}
 
       if (!var)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 67d06e5..c4be31b 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -644,7 +644,7 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
     {
       do_cleanups (cleanups);
       /* We know this will always throw.  */
-      GDB_PY_HANDLE_EXCEPTION (except);
+      return gdbpy_convert_exception (except);
     }
 
   if (sals.nelts)
-- 
1.8.1.4

Re: [PATCH 28/40] use explicit returns to avoid checker confusion

[asmwarrior]

On 2013-5-10 2:52, Tom Tromey wrote:
> The checker does not understand the idiom
> 
> if (except.reason < 0) {
>    do_cleanups (whatever);
>    GDB_PY_HANDLE_EXCEPTION (except);
> }
> 
> because it doesn't realize that the nested 'if' actually has the same
> condition.
> 
> This fixes instances of this to be more explicit.
> 
> 	* python/py-breakpoint.c (bppy_get_commands): Use
> 	explicit, unconditional return.
> 	* python/py-frame.c (frapy_read_var): Likewise.
> 	* python/python.c (gdbpy_decode_line): Likewise.
> ---
>  gdb/python/py-breakpoint.c | 2 +-
>  gdb/python/py-frame.c      | 2 +-
>  gdb/python/python.c        | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index d099892..fd13811 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -492,7 +492,7 @@ bppy_get_commands (PyObject *self, void *closure)
>    if (except.reason < 0)
>      {
>        do_cleanups (chain);
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      return gdbpy_convert_exception (except);
>      }
>  
>    cmdstr = ui_file_xstrdup (string_file, &length);
> diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
> index 33d0bd0..10a50ea 100644
> --- a/gdb/python/py-frame.c
> +++ b/gdb/python/py-frame.c
> @@ -472,7 +472,7 @@ frapy_read_var (PyObject *self, PyObject *args)
>        if (except.reason < 0)
>  	{
>  	  do_cleanups (cleanup);
> -	  GDB_PY_HANDLE_EXCEPTION (except);
> +	  return gdbpy_convert_exception (except);
>  	}
>  
>        if (!var)
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 67d06e5..c4be31b 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -644,7 +644,7 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
>      {
>        do_cleanups (cleanups);
>        /* We know this will always throw.  */
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      return gdbpy_convert_exception (except);
>      }
>  
>    if (sals.nelts)
> 

I see a compiler error:

mingw32-gcc -O0 -g -D__USE_MINGW_ACCESS   -I. -I../../archer/gdb -I../../archer/gdb/common -I../../archer/gdb/config -DLOCALEDIR="\"/mingw/share/locale\"" -DHAVE_CONFIG_H -I../../archer/gdb/../include/opcode -I../../archer/gdb/../opcodes/.. -I../../archer/gdb/../readline/.. -I../bfd -I../../archer/gdb/../bfd -I../../archer/gdb/../include -I../libdecnumber -I../../archer/gdb/../libdecnumber  -I../../archer/gdb/gnulib/import -Ibuild-gnulib/import    -IE:/code/python27/include -IE:/code/python27/include -Wall -Wdeclaration-after-statement -Wpointer-arith -Wpointer-sign -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wmissing-prototypes -Wdeclaration-after-statement -Wempty-body -Wno-format -Werror -c -o python.o -MT python.o -MMD -MP -MF .deps/python.Tpo -fno-strict-aliasing -DNDEBUG -fwrapv ../../archer/gdb/python/python.c
../../archer/gdb/python/python.c: In function 'gdbpy_decode_line':
../../archer/gdb/python/python.c:656:7: error: void value not ignored as it ought to be
make[2]: *** [python.o] Error 1
make[2]: Leaving directory `/f/build_gdb/abuild/gdb'
make[1]: *** [all-gdb] Error 2
make[1]: Leaving directory `/f/build_gdb/abuild'
make: *** [all] Error 2

I see that: gdbpy_convert_exception (except) return a "void" type, but the function gdbpy_decode_line() need to return a "PyObject *"

The macro GDB_PY_HANDLE_EXCEPTION (except) did return a NULL, see:
/* Use this after a TRY_EXCEPT to throw the appropriate Python
   exception.  */
#define GDB_PY_HANDLE_EXCEPTION(Exception)	\
  do {						\
    if (Exception.reason < 0)			\
      {						\
	gdbpy_convert_exception (Exception);	\
        return NULL;				\
      }						\
  } while (0)


So, what about:

gdbpy_convert_exception (except);
return NULL;


Yuanhui Zhang

Re: [PATCH 28/40] use explicit returns to avoid checker confusion

[Tom Tromey]

>>>>> ">" == ext asmwarrior <asmwarrior@gmail.com> writes:

>> I see a compiler error:

I think the patch series needs to be rebased.  This function's got
changed as part of the Python reference counting series.  I'll fix it up
and re-send modified patches soon, probably next week.

Tom

Re: [PATCH 28/40] use explicit returns to avoid checker confusion

[Tom Tromey]

Tom> I think the patch series needs to be rebased.  This function's got
Tom> changed as part of the Python reference counting series.  I'll fix it up
Tom> and re-send modified patches soon, probably next week.

I rebased today and this was the only patch requiring a fix.

Here is the new version.

Tom

    	* python/py-breakpoint.c (bppy_get_commands): Use
    	explicit, unconditional return.
    	* python/py-frame.c (frapy_read_var): Likewise.
    	* python/python.c (gdbpy_decode_line): Likewise.

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index eaa1bc5..87f1fdc 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -492,7 +492,8 @@ bppy_get_commands (PyObject *self, void *closure)
   if (except.reason < 0)
     {
       do_cleanups (chain);
-      GDB_PY_HANDLE_EXCEPTION (except);
+      gdbpy_convert_exception (except);
+      return NULL;
     }
 
   cmdstr = ui_file_xstrdup (string_file, &length);
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 2615ddf..f960b08 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -477,7 +477,8 @@ frapy_read_var (PyObject *self, PyObject *args)
       if (except.reason < 0)
 	{
 	  do_cleanups (cleanup);
-	  GDB_PY_HANDLE_EXCEPTION (except);
+	  gdbpy_convert_exception (except);
+	  return NULL;
 	}
 
       if (!var)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 690534f..c94198e 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -653,7 +653,8 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
     {
       do_cleanups (cleanups);
       /* We know this will always throw.  */
-      GDB_PY_HANDLE_EXCEPTION (except);
+      gdbpy_convert_exception (except);
+      return NULL;
     }
 
   if (sals.nelts)

Re: [PATCH 28/40] use explicit returns to avoid checker confusion

[asmwarrior]

On 2013-5-24 23:41, Tom Tromey wrote:
> Tom> I think the patch series needs to be rebased.  This function's got
> Tom> changed as part of the Python reference counting series.  I'll fix it up
> Tom> and re-send modified patches soon, probably next week.
>
> I rebased today and this was the only patch requiring a fix.
>
> Here is the new version.
>
> Tom
>
>     	* python/py-breakpoint.c (bppy_get_commands): Use
>     	explicit, unconditional return.
>     	* python/py-frame.c (frapy_read_var): Likewise.
>     	* python/python.c (gdbpy_decode_line): Likewise.
>
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index eaa1bc5..87f1fdc 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -492,7 +492,8 @@ bppy_get_commands (PyObject *self, void *closure)
>    if (except.reason < 0)
>      {
>        do_cleanups (chain);
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      gdbpy_convert_exception (except);
> +      return NULL;
>      }
>  
>    cmdstr = ui_file_xstrdup (string_file, &length);
> diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
> index 2615ddf..f960b08 100644
> --- a/gdb/python/py-frame.c
> +++ b/gdb/python/py-frame.c
> @@ -477,7 +477,8 @@ frapy_read_var (PyObject *self, PyObject *args)
>        if (except.reason < 0)
>  	{
>  	  do_cleanups (cleanup);
> -	  GDB_PY_HANDLE_EXCEPTION (except);
> +	  gdbpy_convert_exception (except);
> +	  return NULL;
>  	}
>  
>        if (!var)
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 690534f..c94198e 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -653,7 +653,8 @@ gdbpy_decode_line (PyObject *self, PyObject *args)
>      {
>        do_cleanups (cleanups);
>        /* We know this will always throw.  */
> -      GDB_PY_HANDLE_EXCEPTION (except);
> +      gdbpy_convert_exception (except);
> +      return NULL;
>      }
>  
>    if (sals.nelts)
Thanks, with this patch, there is no build failure now.

Yuanhui Zhang

[PATCH 27/40] fix cp-namespace.c

[Tom Tromey, Tom Tromey]

cp_lookup_symbol_imports_or_template could return without
running cleanups.

	* cp-namespace.c (cp_lookup_symbol_imports_or_template): Call
	do_cleanups on all return paths.
---
 gdb/cp-namespace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index add4ccb..792ed48 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -499,7 +499,10 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 				      TYPE_N_TEMPLATE_ARGUMENTS (context),
 				      TYPE_TEMPLATE_ARGUMENTS (context));
 	      if (result != NULL)
-		return result;
+		{
+		  do_cleanups (cleanups);
+		  return result;
+		}
 	    }
 
 	  do_cleanups (cleanups);
-- 
1.8.1.4

[PATCH 32/40] fix dbxread.c

[Tom Tromey, Tom Tromey]

This is a stylistic change to make some code in dbxread.c analyzable
by the checker.

	* dbxread.c (dbx_read_symtab): Declare 'back_to' in a more
	inner scope.  Unconditionally call do_cleanups.
---
 gdb/dbxread.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 8134f13..c0fe2b6 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2449,7 +2449,6 @@ static void
 dbx_read_symtab (struct partial_symtab *self, struct objfile *objfile)
 {
   bfd *sym_bfd;
-  struct cleanup *back_to = NULL;
 
   if (self->readin)
     {
@@ -2461,6 +2460,8 @@ dbx_read_symtab (struct partial_symtab *self, struct objfile *objfile)
 
   if (LDSYMLEN (self) || self->number_of_dependencies)
     {
+      struct cleanup *back_to;
+
       /* Print the message now, before reading the string table,
          to avoid disconcerting pauses.  */
       if (info_verbose)
@@ -2473,6 +2474,8 @@ dbx_read_symtab (struct partial_symtab *self, struct objfile *objfile)
 
       next_symbol_text_func = dbx_next_symbol_text;
 
+      back_to = make_cleanup (null_cleanup, NULL);
+
       if (DBX_STAB_SECTION (objfile))
 	{
 	  stabs_data
@@ -2481,14 +2484,12 @@ dbx_read_symtab (struct partial_symtab *self, struct objfile *objfile)
 					      NULL);
 
 	  if (stabs_data)
-	    back_to = make_cleanup (free_current_contents,
-				    (void *) &stabs_data);
+	    make_cleanup (free_current_contents, (void *) &stabs_data);
 	}
 
       dbx_psymtab_to_symtab_1 (objfile, self);
 
-      if (back_to)
-	do_cleanups (back_to);
+      do_cleanups (back_to);
 
       /* Match with global symbols.  This only needs to be done once,
          after all of the symtabs and dependencies have been read in.   */
-- 
1.8.1.4

[PATCH 25/40] fix one bug in stabsread.c

[Tom Tromey, Tom Tromey]

Some code in stabsread.c can return without running cleanups.

	* stabsread.c (read_struct_type): Call do_cleanups along
	all return paths.
---
 gdb/stabsread.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index c63ecc2..875541c 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -3515,7 +3515,10 @@ read_struct_type (char **pp, struct type *type, enum type_code type_code,
 
     TYPE_LENGTH (type) = read_huge_number (pp, 0, &nbits, 0);
     if (nbits != 0)
-      return error_type (pp, objfile);
+      {
+	do_cleanups (back_to);
+	return error_type (pp, objfile);
+      }
     set_length_in_type_chain (type);
   }
 
-- 
1.8.1.4

[PATCH 30/40] fix linux-thread-db.c

[Tom Tromey, Tom Tromey]

This is a stylistic change to make it so the checker can analyze a
function in linux-thread-db.c.

	* linux-thread-db.c (thread_db_load_search): Unconditionally
	call do_cleanups.
---
 gdb/linux-thread-db.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index a698c65..23c29c9 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1029,17 +1029,17 @@ thread_db_load_search (void)
 	      || this_dir[pdir_len] == '/'))
 	{
 	  char *subdir = NULL;
-	  struct cleanup *free_subdir_cleanup = NULL;
+	  struct cleanup *free_subdir_cleanup
+	    = make_cleanup (null_cleanup, NULL);
 
 	  if (this_dir[pdir_len] == '/')
 	    {
 	      subdir = xmalloc (strlen (this_dir));
-	      free_subdir_cleanup = make_cleanup (xfree, subdir);
+	      make_cleanup (xfree, subdir);
 	      strcpy (subdir, this_dir + pdir_len + 1);
 	    }
 	  rc = try_thread_db_load_from_pdir (subdir);
-	  if (free_subdir_cleanup != NULL)
-	    do_cleanups (free_subdir_cleanup);
+	  do_cleanups (free_subdir_cleanup);
 	  if (rc)
 	    break;
 	}
-- 
1.8.1.4

[PATCH 36/40] introduce dangling_cleanup attribute and change source to use it

[Tom Tromey, Tom Tromey]

Some functions leak cleanups on purpose.  However, these functions
aren't "constructors" in the cleanup-checker sense, as they do not
return the cleanup.

This patch adds an attribute that can be used to mark these functions.
This avoids a bunch of false reports from the checker.

This patch is really a huge hack.  It is basically giving up on
cleanly checking the use of cleanups.  You can see this most easily by
noticing that I elected not to have this property propagate through
the call chain.

Still, I figured I'd see what people thought of this patch.  Without
it the checker is harder to use, as you must know which functions are
ignorable.

Maybe some of these are fixable with a bit more thought.

	* ada-lang.c (old_renaming_is_invisible): Use DANGLING_CLEANUP.
	* breakpoint.c (breakpoint_re_set_default): Use DANGLING_CLEANUP.
	* c-exp.y (operator_stoken): Use DANGLING_CLEANUP.
	* coffread.c (coff_locate_sections): Use DANGLING_CLEANUP.
	* contrib/cleanup_check.py (leaks_cleanup): New global.
	(note_dangling_cleanup, leaves_dangling_cleanup): New functions.
	(CleanupChecker.compute_master): Add comment.  Check
	leaves_dangling_cleanup.
	(CleanupChecker.check_cleanups): Don't mention special constructors
	to the user.
	(register_attributes): New function.  Register it with gcc.
	* defs.h (DANGLING_CLEANUP): New define.
	* dwarf2read.c (read_cutu_die_from_dwo, handle_DW_AT_stmt_list)
	(setup_type_unit_groups, dwarf2_add_field, dwarf2_add_typedef)
	(dwarf2_add_member_fn, psymtab_include_file_name)
	(dwarf_decode_lines): Use DANGLING_CLEANUP.
	* linux-nat.c (linux_child_pid_to_exec_file): Use DANGLING_CLEANUP.
	* python/py-prettyprint.c (push_dummy_python_frame): Use
	DANGLING_CLEANUP.
	* stabsread.c (read_member_functions, read_struct_fields)
	(read_baseclasses): Use DANGLING_CLEANUP.
	* utils.h (gdb_bfd_errmsg): Use DANGLING_CLEANUP.
---
 gdb/ada-lang.c               |  1 +
 gdb/breakpoint.c             |  1 +
 gdb/c-exp.y                  |  1 +
 gdb/coffread.c               |  1 +
 gdb/contrib/cleanup_check.py | 25 ++++++++++++++++++++++---
 gdb/defs.h                   |  6 ++++++
 gdb/dwarf2read.c             |  8 ++++++++
 gdb/linux-nat.c              |  1 +
 gdb/python/py-prettyprint.c  |  1 +
 gdb/stabsread.c              |  3 +++
 gdb/utils.h                  |  3 ++-
 11 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 314f4ca..5823022 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4722,6 +4722,7 @@ is_package_name (const char *name)
 /* Return nonzero if SYM corresponds to a renaming entity that is
    not visible from FUNCTION_NAME.  */
 
+DANGLING_CLEANUP
 static int
 old_renaming_is_invisible (const struct symbol *sym, const char *function_name)
 {
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 56398a0..1cfed3b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14068,6 +14068,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
    breakpoints.  Reevaluate the breakpoint and recreate its
    locations.  */
 
+DANGLING_CLEANUP
 static void
 breakpoint_re_set_default (struct breakpoint *b)
 {
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index dd032d2..a3ca6b8 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1646,6 +1646,7 @@ write_destructor_name (struct stoken token)
 
 /* Returns a stoken of the operator name given by OP (which does not
    include the string "operator").  */ 
+DANGLING_CLEANUP
 static struct stoken
 operator_stoken (const char *op)
 {
diff --git a/gdb/coffread.c b/gdb/coffread.c
index bf39085..886f0b6 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -208,6 +208,7 @@ static void coff_symtab_read (long, unsigned int, struct objfile *);
    section flags to specify what kind of debug section it is
    -kingdon).  */
 
+DANGLING_CLEANUP
 static void
 coff_locate_sections (bfd *abfd, asection *sectp, void *csip)
 {
diff --git a/gdb/contrib/cleanup_check.py b/gdb/contrib/cleanup_check.py
index 17b81f6..dca0ec8 100644
--- a/gdb/contrib/cleanup_check.py
+++ b/gdb/contrib/cleanup_check.py
@@ -58,6 +58,14 @@ special_names = set(['do_final_cleanups', 'discard_final_cleanups',
 def needs_special_treatment(decl):
     return decl.name in special_names
 
+leaks_cleanup = { }
+
+def note_dangling_cleanup(*args):
+    leaks_cleanup[str(args[0].name)] = 1
+
+def leaves_dangling_cleanup(name):
+    return name in leaks_cleanup
+
 # Sometimes we need a new placeholder object that isn't the same as
 # anything else.
 class Dummy(object):
@@ -204,6 +212,10 @@ class CleanupChecker:
             if stmt.loc:
                 curloc = stmt.loc
             if isinstance(stmt, gcc.GimpleCall) and stmt.fndecl:
+                # The below used to also have:
+                #  or leaves_dangling_cleanup(str(stmt.fndecl.name))
+                # but this yields a lot of noise when the point
+                # is really just to pretend that these functions are ok.
                 if is_constructor(stmt.fndecl):
                     log('saw constructor %s in bb=%d' % (str(stmt.fndecl), bb.index), 2)
                     self.cleanup_aware = True
@@ -223,7 +235,7 @@ class CleanupChecker:
                     if not master_cleanup.verify(curloc, stmt.retval, bb_from):
                         gcc.permerror(curloc,
                                       'constructor does not return master cleanup')
-                elif not self.is_special_constructor:
+                elif not self.is_special_constructor and not leaves_dangling_cleanup(str(self.fun.decl.name)):
                     if not master_cleanup.isempty():
                         if curloc not in self.bad_returns:
                             gcc.permerror(curloc, 'cleanup stack is not empty at return')
@@ -273,8 +285,8 @@ class CleanupChecker:
         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 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.
@@ -307,6 +319,13 @@ class CheckerPass(gcc.GimplePass):
         if fun.decl:
             log(fun.decl.name + ': ' + what, 2)
 
+def register_attributes():
+    gcc.register_attribute('dangling_cleanup', 0, 0, True, False, False,
+                           note_dangling_cleanup)
+    gcc.define_macro('WITH_ATTRIBUTE_DANGLING_CLEANUP')
+
+gcc.register_callback(gcc.PLUGIN_ATTRIBUTES, register_attributes)
+
 ps = CheckerPass(name = 'check-cleanups')
 # We need the cfg, but we want a relatively high-level Gimple.
 ps.register_after('release_ssa')
diff --git a/gdb/defs.h b/gdb/defs.h
index d8a1adb..f82fc17 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -98,6 +98,12 @@
 #include "libiberty.h"
 #include "hashtab.h"
 
+#ifdef WITH_ATTRIBUTE_DANGLING_CLEANUP
+#define DANGLING_CLEANUP __attribute__((dangling_cleanup))
+#else
+#define DANGLING_CLEANUP
+#endif
+
 /* Rather than duplicate all the logic in BFD for figuring out what
    types to use (which can be pretty complicated), symply define them
    in terms of the corresponding type from BFD.  */
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 2a5acf0..5392842 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -4437,6 +4437,7 @@ init_cu_die_reader (struct die_reader_specs *reader,
    provided an abbrev table to use.
    The result is non-zero if a valid (non-dummy) DIE was found.  */
 
+DANGLING_CLEANUP
 static int
 read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
 			struct dwo_unit *dwo_unit,
@@ -8061,6 +8062,7 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
    COMP_DIR is the compilation directory.
    WANT_LINE_INFO is non-zero if the pc/line-number mapping is needed.  */
 
+DANGLING_CLEANUP
 static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir) /* ARI: editCase function */
@@ -8177,6 +8179,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
    then restore those symtabs in the line header.
    We don't need the pc/line-number mapping for type units.  */
 
+DANGLING_CLEANUP
 static void
 setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
 {
@@ -10712,6 +10715,7 @@ handle_data_member_location (struct die_info *die, struct dwarf2_cu *cu,
 
 /* Add an aggregate field to the field list.  */
 
+DANGLING_CLEANUP
 static void
 dwarf2_add_field (struct field_info *fip, struct die_info *die,
 		  struct dwarf2_cu *cu)
@@ -10899,6 +10903,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
 
 /* Add a typedef defined in the scope of the FIP's class.  */
 
+DANGLING_CLEANUP
 static void
 dwarf2_add_typedef (struct field_info *fip, struct die_info *die,
 		    struct dwarf2_cu *cu)
@@ -11062,6 +11067,7 @@ dwarf2_is_constructor (struct die_info *die, struct dwarf2_cu *cu)
 
 /* Add a member function to the proper fieldlist.  */
 
+DANGLING_CLEANUP
 static void
 dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
 		      struct type *type, struct dwarf2_cu *cu)
@@ -15368,6 +15374,7 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
 
    The function creates dangling cleanup registration.  */
 
+DANGLING_CLEANUP
 static const char *
 psymtab_include_file_name (const struct line_header *lh, int file_index,
 			   const struct partial_symtab *pst,
@@ -15769,6 +15776,7 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.  */
 
+DANGLING_CLEANUP
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 6ba71ba..6859449 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4301,6 +4301,7 @@ linux_nat_thread_name (struct thread_info *thr)
 /* Accepts an integer PID; Returns a string representing a file that
    can be opened to get the symbols for the child process.  */
 
+DANGLING_CLEANUP
 static char *
 linux_child_pid_to_exec_file (int pid)
 {
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index bdb1ac5..7e482bc 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -400,6 +400,7 @@ py_restore_tstate (void *p)
 
 /* Create a dummy PyFrameObject, needed to work around
    a Python-2.4 bug with generators.  */
+DANGLING_CLEANUP
 static PyObject *
 push_dummy_python_frame (void)
 {
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index 875541c..506e3b4 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -2246,6 +2246,7 @@ stabs_method_name_from_physname (const char *physname)
 
    Returns 1 for success, 0 for failure.  */
 
+DANGLING_CLEANUP
 static int
 read_member_functions (struct field_info *fip, char **pp, struct type *type,
 		       struct objfile *objfile)
@@ -2972,6 +2973,7 @@ read_one_struct_field (struct field_info *fip, char **pp, char *p,
 
    Returns 1 for success, 0 for failure.  */
 
+DANGLING_CLEANUP
 static int
 read_struct_fields (struct field_info *fip, char **pp, struct type *type,
 		    struct objfile *objfile)
@@ -3070,6 +3072,7 @@ read_struct_fields (struct field_info *fip, char **pp, struct type *type,
 
 
 
+DANGLING_CLEANUP
 static int
 read_baseclasses (struct field_info *fip, char **pp, struct type *type,
 		  struct objfile *objfile)
diff --git a/gdb/utils.h b/gdb/utils.h
index ad5bea4..de232b9 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -52,7 +52,8 @@ extern char *safe_strerror (int);
    MATCHING, if non-NULL, is the corresponding argument to
    bfd_check_format_matches, and will be freed.  */
 
-extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
+extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching)
+  DANGLING_CLEANUP;
 
 /* Reset the prompt_for_continue clock.  */
 void reset_prompt_for_continue_wait_time (void);
-- 
1.8.1.4

Re: [PATCH 36/40] introduce dangling_cleanup attribute and change source to use it

[Tom Tromey]

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

Tom> Some functions leak cleanups on purpose.  However, these functions
Tom> aren't "constructors" in the cleanup-checker sense, as they do not
Tom> return the cleanup.

I added a comment explaining WITH_ATTRIBUTE_DANGLING_CLEANUP to this
patch.  Also, the checker found one more dangling cleanup that was not
readily fixable, so I added an attribute there.  (This is the
linux-tdep.c change.)

Tom

	* ada-lang.c (old_renaming_is_invisible): Use DANGLING_CLEANUP.
	* breakpoint.c (breakpoint_re_set_default): Use DANGLING_CLEANUP.
	* c-exp.y (operator_stoken): Use DANGLING_CLEANUP.
	* coffread.c (coff_locate_sections): Use DANGLING_CLEANUP.
	* contrib/cleanup_check.py (leaks_cleanup): New global.
	(note_dangling_cleanup, leaves_dangling_cleanup): New functions.
	(CleanupChecker.compute_master): Add comment.  Check
	leaves_dangling_cleanup.
	(CleanupChecker.check_cleanups): Don't mention special constructors
	to the user.
	(register_attributes): New function.  Register it with gcc.
	* defs.h (DANGLING_CLEANUP): New define.
	* dwarf2read.c (read_cutu_die_from_dwo, handle_DW_AT_stmt_list)
	(setup_type_unit_groups, dwarf2_add_field, dwarf2_add_typedef)
	(dwarf2_add_member_fn, psymtab_include_file_name)
	(dwarf_decode_lines): Use DANGLING_CLEANUP.
	* linux-nat.c (linux_child_pid_to_exec_file): Use DANGLING_CLEANUP.
	* linux-tdep.c (linux_make_corefile_notes): Use DANGLING_CLEANUP.
	* python/py-prettyprint.c (push_dummy_python_frame): Use
	DANGLING_CLEANUP.
	* stabsread.c (read_member_functions, read_struct_fields)
	(read_baseclasses): Use DANGLING_CLEANUP.
	* utils.h (gdb_bfd_errmsg): Use DANGLING_CLEANUP.

more dangling cleanups
---
 gdb/ada-lang.c               |  1 +
 gdb/breakpoint.c             |  1 +
 gdb/c-exp.y                  |  1 +
 gdb/coffread.c               |  1 +
 gdb/contrib/cleanup_check.py | 25 ++++++++++++++++++++++---
 gdb/defs.h                   | 10 ++++++++++
 gdb/dwarf2read.c             |  8 ++++++++
 gdb/linux-nat.c              |  1 +
 gdb/linux-tdep.c             |  1 +
 gdb/python/py-prettyprint.c  |  1 +
 gdb/stabsread.c              |  3 +++
 gdb/utils.h                  |  3 ++-
 12 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index fa6db0f..19bceb6 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4722,6 +4722,7 @@ is_package_name (const char *name)
 /* Return nonzero if SYM corresponds to a renaming entity that is
    not visible from FUNCTION_NAME.  */
 
+DANGLING_CLEANUP
 static int
 old_renaming_is_invisible (const struct symbol *sym, const char *function_name)
 {
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2f4d4fc..4c34a5c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14066,6 +14066,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
    breakpoints.  Reevaluate the breakpoint and recreate its
    locations.  */
 
+DANGLING_CLEANUP
 static void
 breakpoint_re_set_default (struct breakpoint *b)
 {
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index dd032d2..a3ca6b8 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1646,6 +1646,7 @@ write_destructor_name (struct stoken token)
 
 /* Returns a stoken of the operator name given by OP (which does not
    include the string "operator").  */ 
+DANGLING_CLEANUP
 static struct stoken
 operator_stoken (const char *op)
 {
diff --git a/gdb/coffread.c b/gdb/coffread.c
index bf39085..886f0b6 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -208,6 +208,7 @@ static void coff_symtab_read (long, unsigned int, struct objfile *);
    section flags to specify what kind of debug section it is
    -kingdon).  */
 
+DANGLING_CLEANUP
 static void
 coff_locate_sections (bfd *abfd, asection *sectp, void *csip)
 {
diff --git a/gdb/contrib/cleanup_check.py b/gdb/contrib/cleanup_check.py
index b182d8d..0dd149f 100644
--- a/gdb/contrib/cleanup_check.py
+++ b/gdb/contrib/cleanup_check.py
@@ -59,6 +59,14 @@ special_names = set(['do_final_cleanups', 'discard_final_cleanups',
 def needs_special_treatment(decl):
     return decl.name in special_names
 
+leaks_cleanup = { }
+
+def note_dangling_cleanup(*args):
+    leaks_cleanup[str(args[0].name)] = 1
+
+def leaves_dangling_cleanup(name):
+    return name in leaks_cleanup
+
 # Sometimes we need a new placeholder object that isn't the same as
 # anything else.
 class Dummy(object):
@@ -218,6 +226,10 @@ class CleanupChecker:
             if stmt.loc:
                 curloc = stmt.loc
             if isinstance(stmt, gcc.GimpleCall) and stmt.fndecl:
+                # The below used to also have:
+                #  or leaves_dangling_cleanup(str(stmt.fndecl.name))
+                # but this yields a lot of noise when the point
+                # is really just to pretend that these functions are ok.
                 if is_constructor(stmt.fndecl):
                     log('saw constructor %s in bb=%d' % (str(stmt.fndecl), bb.index), 2)
                     self.cleanup_aware = True
@@ -240,7 +252,7 @@ class CleanupChecker:
                     if not master_cleanup.verify(curloc, stmt.retval):
                         gcc.permerror(curloc,
                                       'constructor does not return master cleanup')
-                elif not self.is_special_constructor:
+                elif not self.is_special_constructor and not leaves_dangling_cleanup(str(self.fun.decl.name)):
                     if not master_cleanup.isempty():
                         if curloc not in self.bad_returns:
                             gcc.permerror(curloc, 'cleanup stack is not empty at return')
@@ -299,8 +311,8 @@ class CleanupChecker:
         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 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.
@@ -330,6 +342,13 @@ class CheckerPass(gcc.GimplePass):
         if fun.decl:
             log(fun.decl.name + ': ' + what, 2)
 
+def register_attributes():
+    gcc.register_attribute('dangling_cleanup', 0, 0, True, False, False,
+                           note_dangling_cleanup)
+    gcc.define_macro('WITH_ATTRIBUTE_DANGLING_CLEANUP')
+
+gcc.register_callback(gcc.PLUGIN_ATTRIBUTES, register_attributes)
+
 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/defs.h b/gdb/defs.h
index d8a1adb..fbf6edc 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -98,6 +98,16 @@
 #include "libiberty.h"
 #include "hashtab.h"
 
+/* The cleanup checker (see contrib/cleanup_check.py) defines
+   WITH_ATTRIBUTE_DANGLING_CLEANUP.  If this is defined then we can
+   use the 'dangling_cleanup' attribute to mark functions which are
+   known to leak a cleanup.  */
+#ifdef WITH_ATTRIBUTE_DANGLING_CLEANUP
+#define DANGLING_CLEANUP __attribute__((dangling_cleanup))
+#else
+#define DANGLING_CLEANUP
+#endif
+
 /* Rather than duplicate all the logic in BFD for figuring out what
    types to use (which can be pretty complicated), symply define them
    in terms of the corresponding type from BFD.  */
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index a17cd9d..8e9b5a5 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -4437,6 +4437,7 @@ init_cu_die_reader (struct die_reader_specs *reader,
    provided an abbrev table to use.
    The result is non-zero if a valid (non-dummy) DIE was found.  */
 
+DANGLING_CLEANUP
 static int
 read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
 			struct dwo_unit *dwo_unit,
@@ -8061,6 +8062,7 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
    COMP_DIR is the compilation directory.
    WANT_LINE_INFO is non-zero if the pc/line-number mapping is needed.  */
 
+DANGLING_CLEANUP
 static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 			const char *comp_dir) /* ARI: editCase function */
@@ -8177,6 +8179,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
    then restore those symtabs in the line header.
    We don't need the pc/line-number mapping for type units.  */
 
+DANGLING_CLEANUP
 static void
 setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu)
 {
@@ -10713,6 +10716,7 @@ handle_data_member_location (struct die_info *die, struct dwarf2_cu *cu,
 
 /* Add an aggregate field to the field list.  */
 
+DANGLING_CLEANUP
 static void
 dwarf2_add_field (struct field_info *fip, struct die_info *die,
 		  struct dwarf2_cu *cu)
@@ -10900,6 +10904,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
 
 /* Add a typedef defined in the scope of the FIP's class.  */
 
+DANGLING_CLEANUP
 static void
 dwarf2_add_typedef (struct field_info *fip, struct die_info *die,
 		    struct dwarf2_cu *cu)
@@ -11063,6 +11068,7 @@ dwarf2_is_constructor (struct die_info *die, struct dwarf2_cu *cu)
 
 /* Add a member function to the proper fieldlist.  */
 
+DANGLING_CLEANUP
 static void
 dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
 		      struct type *type, struct dwarf2_cu *cu)
@@ -15370,6 +15376,7 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
 
    The function creates dangling cleanup registration.  */
 
+DANGLING_CLEANUP
 static const char *
 psymtab_include_file_name (const struct line_header *lh, int file_index,
 			   const struct partial_symtab *pst,
@@ -15771,6 +15778,7 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    E.g. expand_line_sal requires this when finding psymtabs to expand.
    A good testcase for this is mb-inline.exp.  */
 
+DANGLING_CLEANUP
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst,
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 6ba71ba..6859449 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4301,6 +4301,7 @@ linux_nat_thread_name (struct thread_info *thr)
 /* Accepts an integer PID; Returns a string representing a file that
    can be opened to get the symbols for the child process.  */
 
+DANGLING_CLEANUP
 static char *
 linux_child_pid_to_exec_file (int pid)
 {
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index bfb6404..b98401d 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1362,6 +1362,7 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 /* Fills the "to_make_corefile_note" target vector.  Builds the note
    section for a corefile, and returns it in a malloc buffer.  */
 
+DANGLING_CLEANUP
 char *
 linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
 			   linux_collect_thread_registers_ftype collect)
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index bdb1ac5..7e482bc 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -400,6 +400,7 @@ py_restore_tstate (void *p)
 
 /* Create a dummy PyFrameObject, needed to work around
    a Python-2.4 bug with generators.  */
+DANGLING_CLEANUP
 static PyObject *
 push_dummy_python_frame (void)
 {
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index 875541c..506e3b4 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -2246,6 +2246,7 @@ stabs_method_name_from_physname (const char *physname)
 
    Returns 1 for success, 0 for failure.  */
 
+DANGLING_CLEANUP
 static int
 read_member_functions (struct field_info *fip, char **pp, struct type *type,
 		       struct objfile *objfile)
@@ -2972,6 +2973,7 @@ read_one_struct_field (struct field_info *fip, char **pp, char *p,
 
    Returns 1 for success, 0 for failure.  */
 
+DANGLING_CLEANUP
 static int
 read_struct_fields (struct field_info *fip, char **pp, struct type *type,
 		    struct objfile *objfile)
@@ -3070,6 +3072,7 @@ read_struct_fields (struct field_info *fip, char **pp, struct type *type,
 
 
 
+DANGLING_CLEANUP
 static int
 read_baseclasses (struct field_info *fip, char **pp, struct type *type,
 		  struct objfile *objfile)
diff --git a/gdb/utils.h b/gdb/utils.h
index 71ce867..146a558 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -53,7 +53,8 @@ extern char *safe_strerror (int);
    MATCHING, if non-NULL, is the corresponding argument to
    bfd_check_format_matches, and will be freed.  */
 
-extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
+extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching)
+  DANGLING_CLEANUP;
 
 /* Reset the prompt_for_continue clock.  */
 void reset_prompt_for_continue_wait_time (void);
-- 
1.8.1.4

[PATCH 35/40] fix mi-cmd-var.c

[Tom Tromey, Tom Tromey]

This is a stylistic change in mi-cmd-var.c that adds outer cleanups
where needed by the checker.

	* mi/mi-cmd-var.c (mi_cmd_var_list_children): Add an outer
	null cleanup.
	(mi_cmd_var_update, varobj_update_one): Likewise.
---
 gdb/mi/mi-cmd-var.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 558454e..c2e8b7a 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -441,14 +441,12 @@ mi_cmd_var_list_children (char *command, char **argv, int argc)
 
   if (from < to)
     {
-      struct cleanup *cleanup_children;
+      struct cleanup *cleanup_children = make_cleanup (null_cleanup, NULL);
 
       if (mi_version (uiout) == 1)
-	cleanup_children
-	  = make_cleanup_ui_out_tuple_begin_end (uiout, "children");
+	make_cleanup_ui_out_tuple_begin_end (uiout, "children");
       else
-	cleanup_children
-	  = make_cleanup_ui_out_list_begin_end (uiout, "children");
+	make_cleanup_ui_out_list_begin_end (uiout, "children");
       for (ix = from;
 	   ix < to && VEC_iterate (varobj_p, children, ix, child);
 	   ++ix)
@@ -702,10 +700,11 @@ mi_cmd_var_update (char *command, char **argv, int argc)
   else
     print_values = PRINT_NO_VALUES;
 
+  cleanup = make_cleanup (null_cleanup, NULL);
   if (mi_version (uiout) <= 1)
-    cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
+    make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
   else
-    cleanup = make_cleanup_ui_out_list_begin_end (uiout, "changelist");
+    make_cleanup_ui_out_list_begin_end (uiout, "changelist");
 
   /* Check if the parameter is a "*", which means that we want to
      update all variables.  */
@@ -741,7 +740,6 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 		   int explicit)
 {
   struct ui_out *uiout = current_uiout;
-  struct cleanup *cleanup = NULL;
   VEC (varobj_update_result) *changes;
   varobj_update_result *r;
   int i;
@@ -752,9 +750,10 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
     {
       char *display_hint;
       int from, to;
+      struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
       if (mi_version (uiout) > 1)
-        cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+	make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
       ui_out_field_string (uiout, "name", varobj_get_objname (r->varobj));
 
       switch (r->status)
@@ -828,8 +827,7 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 	  r->new = NULL;	/* Paranoia.  */
 	}
 
-      if (mi_version (uiout) > 1)
-	do_cleanups (cleanup);
+      do_cleanups (cleanup);
     }
   VEC_free (varobj_update_result, changes);
 }
-- 
1.8.1.4

Re: [PATCH 35/40] fix mi-cmd-var.c

[Tom Tromey]

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

Tom> This is a stylistic change in mi-cmd-var.c that adds outer cleanups
Tom> where needed by the checker.

The new version of the checker is a little more robust here, and so I
was able to drop a couple parts of this patch.  Now the checker
understands constructs like:

if (cond)
  cleanup = something();
else
  cleanup = something_else();
...
do_cleanups (cleanup);

Tom

	* mi/mi-cmd-var.c (varobj_update_one): Add an outer null cleanup.
---
 gdb/mi/mi-cmd-var.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 558454e..a069346 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -741,7 +741,6 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 		   int explicit)
 {
   struct ui_out *uiout = current_uiout;
-  struct cleanup *cleanup = NULL;
   VEC (varobj_update_result) *changes;
   varobj_update_result *r;
   int i;
@@ -752,9 +751,10 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
     {
       char *display_hint;
       int from, to;
+      struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
       if (mi_version (uiout) > 1)
-        cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+	make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
       ui_out_field_string (uiout, "name", varobj_get_objname (r->varobj));
 
       switch (r->status)
@@ -828,8 +828,7 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 	  r->new = NULL;	/* Paranoia.  */
 	}
 
-      if (mi_version (uiout) > 1)
-	do_cleanups (cleanup);
+      do_cleanups (cleanup);
     }
   VEC_free (varobj_update_result, changes);
 }
-- 
1.8.1.4

[PATCH 34/40] fix cli-script.c

[Tom Tromey, Tom Tromey]

read_command_lines_1 had some (IMNSHO) spaghetti-ish code for cleanup
handling.  This makes the code much simpler to understand, by
introducing an outer cleanup.

This is another case where a stylistic change for the checker is also
nice for the reader.

	* cli/cli-script.c (read_command_lines_1): Use a null cleanup
	for 'old_chain'.  Do not check 'head' before processing
	cleanups.
---
 gdb/cli/cli-script.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 43fd479..d35f42f 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -1246,13 +1246,12 @@ read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
 		      void (*validator)(char *, void *), void *closure)
 {
   struct command_line *head, *tail, *next;
-  struct cleanup *old_chain;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   enum command_control_type ret;
   enum misc_command_type val;
 
   control_level = 0;
   head = tail = NULL;
-  old_chain = NULL;
 
   while (1)
     {
@@ -1298,22 +1297,17 @@ read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
       else
 	{
 	  head = next;
-	  old_chain = make_cleanup_free_command_lines (&head);
+	  make_cleanup_free_command_lines (&head);
 	}
       tail = next;
     }
 
   dont_repeat ();
 
-  if (head)
-    {
-      if (ret != invalid_control)
-	{
-	  discard_cleanups (old_chain);
-	}
-      else
-	do_cleanups (old_chain);
-    }
+  if (ret != invalid_control)
+    discard_cleanups (old_chain);
+  else
+    do_cleanups (old_chain);
 
   return head;
 }
-- 
1.8.1.4

[PATCH 39/40] fix compile_rx_or_error

[Tom Tromey, Tom Tromey]

compile_rx_or_error looks like a constructor, but it can return NULL.
This patch changes it to remove the NULL return, making it work
like any other cleanup constructor.

This is a stylistic patch but I think it is also better for code to
follow the normal conventions.

	* probe.c (collect_probes): Check arguments for NULL before
	calling compile_rx_or_error.
	* utils.c (compile_rx_or_error): Require 'rx' to be non-NULL.
	Remove NULL return.
---
 gdb/probe.c | 9 ++++++---
 gdb/utils.c | 7 ++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/probe.c b/gdb/probe.c
index 05bdd1b..3086f4d 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -245,9 +245,12 @@ collect_probes (char *objname, char *provider, char *probe_name,
   cleanup = make_cleanup (VEC_cleanup (probe_p), &result);
 
   cleanup_temps = make_cleanup (null_cleanup, NULL);
-  compile_rx_or_error (&prov_pat, provider, _("Invalid provider regexp"));
-  compile_rx_or_error (&probe_pat, probe_name, _("Invalid probe regexp"));
-  compile_rx_or_error (&obj_pat, objname, _("Invalid object file regexp"));
+  if (provider != NULL)
+    compile_rx_or_error (&prov_pat, provider, _("Invalid provider regexp"));
+  if (probe_name != NULL)
+    compile_rx_or_error (&probe_pat, probe_name, _("Invalid probe regexp"));
+  if (objname != NULL)
+    compile_rx_or_error (&obj_pat, objname, _("Invalid object file regexp"));
 
   ALL_OBJFILES (objfile)
     {
diff --git a/gdb/utils.c b/gdb/utils.c
index c25dadf..fa54e10 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1127,17 +1127,14 @@ get_regcomp_error (int code, regex_t *rx)
 }
 
 /* Compile a regexp and throw an exception on error.  This returns a
-   cleanup to free the resulting pattern on success.  If RX is NULL,
-   this does nothing and returns NULL.  */
+   cleanup to free the resulting pattern on success.  RX must not be
+   NULL.  */
 
 struct cleanup *
 compile_rx_or_error (regex_t *pattern, const char *rx, const char *message)
 {
   int code;
 
-  if (!rx)
-    return NULL;
-
   code = regcomp (pattern, rx, REG_NOSUB);
   if (code != 0)
     {
-- 
1.8.1.4

Re: [PATCH 39/40] fix compile_rx_or_error

[Yao Qi]

On 05/10/2013 02:53 AM, Tom Tromey wrote:
> @@ -1127,17 +1127,14 @@ get_regcomp_error (int code, regex_t *rx)
>   }
>
>   /* Compile a regexp and throw an exception on error.  This returns a
> -   cleanup to free the resulting pattern on success.  If RX is NULL,
> -   this does nothing and returns NULL.  */
> +   cleanup to free the resulting pattern on success.  RX must not be
> +   NULL.  */
>
>   struct cleanup *
>   compile_rx_or_error (regex_t *pattern, const char *rx, const char *message)
>   {
>     int code;
>
> -  if (!rx)
> -    return NULL;
> -

Do we need an assert on RX here?

-- 
Yao (齐尧)

Re: [PATCH 39/40] fix compile_rx_or_error

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

>> -  if (!rx)
>> -    return NULL;
>> -

Yao> Do we need an assert on RX here?

It can't hurt, but I think the subsequent regcomp will just crash.
I will add one.

Tom

Re: [PATCH 39/40] fix compile_rx_or_error

[Tom Tromey]

Yao> Do we need an assert on RX here?

Tom> It can't hurt, but I think the subsequent regcomp will just crash.
Tom> I will add one.

For the record, here is what I am checking in.

Tom

    	* probe.c (collect_probes): Check arguments for NULL before
    	calling compile_rx_or_error.
    	* utils.c (compile_rx_or_error): Require 'rx' to be non-NULL.
    	Remove NULL return.

diff --git a/gdb/probe.c b/gdb/probe.c
index 05bdd1b..3086f4d 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -245,9 +245,12 @@ collect_probes (char *objname, char *provider, char *probe_name,
   cleanup = make_cleanup (VEC_cleanup (probe_p), &result);
 
   cleanup_temps = make_cleanup (null_cleanup, NULL);
-  compile_rx_or_error (&prov_pat, provider, _("Invalid provider regexp"));
-  compile_rx_or_error (&probe_pat, probe_name, _("Invalid probe regexp"));
-  compile_rx_or_error (&obj_pat, objname, _("Invalid object file regexp"));
+  if (provider != NULL)
+    compile_rx_or_error (&prov_pat, provider, _("Invalid provider regexp"));
+  if (probe_name != NULL)
+    compile_rx_or_error (&probe_pat, probe_name, _("Invalid probe regexp"));
+  if (objname != NULL)
+    compile_rx_or_error (&obj_pat, objname, _("Invalid object file regexp"));
 
   ALL_OBJFILES (objfile)
     {
diff --git a/gdb/utils.c b/gdb/utils.c
index c25dadf..18ee9bb 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1127,16 +1127,15 @@ get_regcomp_error (int code, regex_t *rx)
 }
 
 /* Compile a regexp and throw an exception on error.  This returns a
-   cleanup to free the resulting pattern on success.  If RX is NULL,
-   this does nothing and returns NULL.  */
+   cleanup to free the resulting pattern on success.  RX must not be
+   NULL.  */
 
 struct cleanup *
 compile_rx_or_error (regex_t *pattern, const char *rx, const char *message)
 {
   int code;
 
-  if (!rx)
-    return NULL;
+  gdb_assert (rx != NULL);
 
   code = regcomp (pattern, rx, REG_NOSUB);
   if (code != 0)

[PATCH 40/40] fix up xml-support.c

[Tom Tromey, Tom Tromey]

xml-support.c has a function that returns a cleanup via an out parameter.

This changes this function to be a normal cleanup constructor --
returning the cleanup directly and returning the other result via an
out parameter.

This is sort of a hack, but it lets the checker work here.

I also noticed that gdb_xml_create_parser_and_cleanup is not really
needed, so I removed it.

	* xml-support.c (gdb_xml_create_parser_and_cleanup_1): Return
	a cleanup.  Remove 'old_chain' argument.  Add 'parser_result'
	argument.
	(gdb_xml_create_parser_and_cleanup): Remove.
	(gdb_xml_parse_quick): Use gdb_xml_create_parser_and_cleanup_1.
	(xml_process_xincludes): Update.
	* xml-support.h (gdb_xml_create_parser_and_cleanup): Don't
	declare.
---
 gdb/xml-support.c | 39 ++++++++++++---------------------------
 gdb/xml-support.h |  7 -------
 2 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index b777814..7fdbf7e 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -440,17 +440,18 @@ gdb_xml_cleanup (void *arg)
   xfree (parser);
 }
 
-/* Initialize and return a parser.  Register a cleanup to destroy the
-   parser.  */
+/* Initialize a parser and store it to *PARSER_RESULT.  Register a
+   cleanup to destroy the parser.  */
 
-static struct gdb_xml_parser *
+static struct cleanup *
 gdb_xml_create_parser_and_cleanup_1 (const char *name,
 				     const struct gdb_xml_element *elements,
-				     void *user_data, struct cleanup **old_chain)
+				     void *user_data,
+				     struct gdb_xml_parser **parser_result)
 {
   struct gdb_xml_parser *parser;
   struct scope_level start_scope;
-  struct cleanup *dummy;
+  struct cleanup *result;
 
   /* Initialize the parser.  */
   parser = XZALLOC (struct gdb_xml_parser);
@@ -476,25 +477,8 @@ gdb_xml_create_parser_and_cleanup_1 (const char *name,
   start_scope.elements = elements;
   VEC_safe_push (scope_level_s, parser->scopes, &start_scope);
 
-  if (old_chain == NULL)
-    old_chain = &dummy;
-
-  *old_chain = make_cleanup (gdb_xml_cleanup, parser);
-  return parser;
-}
-
-/* Initialize and return a parser.  Register a cleanup to destroy the
-   parser.  */
-
-struct gdb_xml_parser *
-gdb_xml_create_parser_and_cleanup (const char *name,
-				   const struct gdb_xml_element *elements,
-				   void *user_data)
-{
-  struct cleanup *old_chain;
-
-  return gdb_xml_create_parser_and_cleanup_1 (name, elements, user_data,
-					      &old_chain);
+  *parser_result = parser;
+  return make_cleanup (gdb_xml_cleanup, parser);
 }
 
 /* External entity handler.  The only external entities we support
@@ -623,8 +607,8 @@ gdb_xml_parse_quick (const char *name, const char *dtd_name,
   struct cleanup *back_to;
   int result;
 
-  parser = gdb_xml_create_parser_and_cleanup_1 (name, elements,
-						user_data, &back_to);
+  back_to = gdb_xml_create_parser_and_cleanup_1 (name, elements,
+						 user_data, &parser);
   if (dtd_name != NULL)
     gdb_xml_use_dtd (parser, dtd_name);
   result = gdb_xml_parse (parser, document);
@@ -897,7 +881,8 @@ xml_process_xincludes (const char *name, const char *text,
   obstack_init (&data->obstack);
   back_to = make_cleanup (xml_xinclude_cleanup, data);
 
-  parser = gdb_xml_create_parser_and_cleanup (name, xinclude_elements, data);
+  gdb_xml_create_parser_and_cleanup_1 (name, xinclude_elements,
+				       data, &parser);
   parser->is_xinclude = 1;
 
   data->include_depth = depth;
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index a319678..a3a15ca 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -171,13 +171,6 @@ struct gdb_xml_element
   gdb_xml_element_end_handler *end_handler;
 };
 
-/* Initialize and return a parser.  Register a cleanup to destroy the
-   parser.  */
-
-struct gdb_xml_parser *gdb_xml_create_parser_and_cleanup
-  (const char *name, const struct gdb_xml_element *elements,
-   void *user_data);
-
 /* Associate DTD_NAME, which must be the name of a compiled-in DTD,
    with PARSER.  */
 
-- 
1.8.1.4

Re: [PATCH 40/40] fix up xml-support.c

[Tom Tromey]

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

Tom> xml-support.c has a function that returns a cleanup via an out parameter.
Tom> This changes this function to be a normal cleanup constructor --
Tom> returning the cleanup directly and returning the other result via an
Tom> out parameter.

Pedro pointed out that gdb_xml_create_parser_and_cleanup_1 can be
renamed back to gdb_xml_create_parser_and_cleanup now.  So, I am
checking in the appended version of this patch, which implements this
change.

Tom

    	* xml-support.c (gdb_xml_create_parser_and_cleanup): Rename from
    	gdb_xml_create_parser_and_cleanup_1.  Return a cleanup.  Remove
    	'old_chain' argument.  Add 'parser_result' argument.
    	(gdb_xml_create_parser_and_cleanup): Remove old version.
    	(gdb_xml_parse_quick): Update.
    	(xml_process_xincludes): Update.
    	* xml-support.h (gdb_xml_create_parser_and_cleanup): Don't
    	declare.

diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index b777814..1682d8e 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -440,17 +440,18 @@ gdb_xml_cleanup (void *arg)
   xfree (parser);
 }
 
-/* Initialize and return a parser.  Register a cleanup to destroy the
-   parser.  */
+/* Initialize a parser and store it to *PARSER_RESULT.  Register a
+   cleanup to destroy the parser.  */
 
-static struct gdb_xml_parser *
-gdb_xml_create_parser_and_cleanup_1 (const char *name,
-				     const struct gdb_xml_element *elements,
-				     void *user_data, struct cleanup **old_chain)
+static struct cleanup *
+gdb_xml_create_parser_and_cleanup (const char *name,
+				   const struct gdb_xml_element *elements,
+				   void *user_data,
+				   struct gdb_xml_parser **parser_result)
 {
   struct gdb_xml_parser *parser;
   struct scope_level start_scope;
-  struct cleanup *dummy;
+  struct cleanup *result;
 
   /* Initialize the parser.  */
   parser = XZALLOC (struct gdb_xml_parser);
@@ -476,25 +477,8 @@ gdb_xml_create_parser_and_cleanup_1 (const char *name,
   start_scope.elements = elements;
   VEC_safe_push (scope_level_s, parser->scopes, &start_scope);
 
-  if (old_chain == NULL)
-    old_chain = &dummy;
-
-  *old_chain = make_cleanup (gdb_xml_cleanup, parser);
-  return parser;
-}
-
-/* Initialize and return a parser.  Register a cleanup to destroy the
-   parser.  */
-
-struct gdb_xml_parser *
-gdb_xml_create_parser_and_cleanup (const char *name,
-				   const struct gdb_xml_element *elements,
-				   void *user_data)
-{
-  struct cleanup *old_chain;
-
-  return gdb_xml_create_parser_and_cleanup_1 (name, elements, user_data,
-					      &old_chain);
+  *parser_result = parser;
+  return make_cleanup (gdb_xml_cleanup, parser);
 }
 
 /* External entity handler.  The only external entities we support
@@ -623,8 +607,8 @@ gdb_xml_parse_quick (const char *name, const char *dtd_name,
   struct cleanup *back_to;
   int result;
 
-  parser = gdb_xml_create_parser_and_cleanup_1 (name, elements,
-						user_data, &back_to);
+  back_to = gdb_xml_create_parser_and_cleanup (name, elements,
+					       user_data, &parser);
   if (dtd_name != NULL)
     gdb_xml_use_dtd (parser, dtd_name);
   result = gdb_xml_parse (parser, document);
@@ -897,7 +881,8 @@ xml_process_xincludes (const char *name, const char *text,
   obstack_init (&data->obstack);
   back_to = make_cleanup (xml_xinclude_cleanup, data);
 
-  parser = gdb_xml_create_parser_and_cleanup (name, xinclude_elements, data);
+  gdb_xml_create_parser_and_cleanup (name, xinclude_elements,
+				     data, &parser);
   parser->is_xinclude = 1;
 
   data->include_depth = depth;
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index a319678..a3a15ca 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -171,13 +171,6 @@ struct gdb_xml_element
   gdb_xml_element_end_handler *end_handler;
 };
 
-/* Initialize and return a parser.  Register a cleanup to destroy the
-   parser.  */
-
-struct gdb_xml_parser *gdb_xml_create_parser_and_cleanup
-  (const char *name, const struct gdb_xml_element *elements,
-   void *user_data);
-
 /* Associate DTD_NAME, which must be the name of a compiled-in DTD,
    with PARSER.  */
 

[PATCH 38/40] some fixes to infrun.c

[Tom Tromey, Tom Tromey]

This fixes some of the problems in infrun.c that the checker reported.
I filed the remaining problems as bugs.

This patch is purely stylistic.

	* infrun.c (handle_vfork_child_exec_or_exit)
	(fetch_inferior_event, adjust_pc_after_break): Introduce
	an outer null cleanup.
---
 gdb/infrun.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 54e92f2..4e7091e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -665,7 +665,7 @@ handle_vfork_child_exec_or_exit (int exec)
       if (inf->vfork_parent->pending_detach)
 	{
 	  struct thread_info *tp;
-	  struct cleanup *old_chain;
+	  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 	  struct program_space *pspace;
 	  struct address_space *aspace;
 
@@ -677,12 +677,12 @@ handle_vfork_child_exec_or_exit (int exec)
 	    {
 	      /* If we're handling a child exit, then inferior_ptid
 		 points at the inferior's pid, not to a thread.  */
-	      old_chain = save_inferior_ptid ();
+	      save_inferior_ptid ();
 	      save_current_program_space ();
 	      save_current_inferior ();
 	    }
 	  else
-	    old_chain = save_current_space_and_thread ();
+	    save_current_space_and_thread ();
 
 	  /* We're letting loose of the parent.  */
 	  tp = any_live_thread_of_process (inf->vfork_parent->pid);
@@ -2803,10 +2803,11 @@ fetch_inferior_event (void *client_data)
   /* If an error happens while handling the event, propagate GDB's
      knowledge of the executing state to the frontend/user running
      state.  */
+  ts_old_chain = make_cleanup (null_cleanup, NULL);
   if (!non_stop)
-    ts_old_chain = make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
+    make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
   else
-    ts_old_chain = make_cleanup (finish_thread_state_cleanup, &ecs->ptid);
+    make_cleanup (finish_thread_state_cleanup, &ecs->ptid);
 
   /* Get executed before make_cleanup_restore_current_thread above to apply
      still for the thread which has thrown the exception.  */
@@ -3004,10 +3005,10 @@ adjust_pc_after_break (struct execution_control_state *ecs)
   if (software_breakpoint_inserted_here_p (aspace, breakpoint_pc)
       || (non_stop && moribund_breakpoint_here_p (aspace, breakpoint_pc)))
     {
-      struct cleanup *old_cleanups = NULL;
+      struct cleanup *old_cleanups = make_cleanup (null_cleanup, NULL);
 
       if (RECORD_IS_USED)
-	old_cleanups = record_full_gdb_operation_disable_set ();
+	record_full_gdb_operation_disable_set ();
 
       /* When using hardware single-step, a SIGTRAP is reported for both
 	 a completed single-step and a software breakpoint.  Need to
@@ -3033,8 +3034,7 @@ adjust_pc_after_break (struct execution_control_state *ecs)
 	  || ecs->event_thread->prev_pc == breakpoint_pc)
 	regcache_write_pc (regcache, breakpoint_pc);
 
-      if (RECORD_IS_USED)
-	do_cleanups (old_cleanups);
+      do_cleanups (old_cleanups);
     }
 }
 
-- 
1.8.1.4

Re: [PATCH 38/40] some fixes to infrun.c

[Tom Tromey]

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

Tom> This fixes some of the problems in infrun.c that the checker reported.
Tom> I filed the remaining problems as bugs.

The new checker let me drop a couple of parts from this patch.
The new patch is appended.

Tom

	* infrun.c (adjust_pc_after_break): Introduce an outer null
	cleanup.
---
 gdb/infrun.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 54e92f2..b768ffd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3004,10 +3004,10 @@ adjust_pc_after_break (struct execution_control_state *ecs)
   if (software_breakpoint_inserted_here_p (aspace, breakpoint_pc)
       || (non_stop && moribund_breakpoint_here_p (aspace, breakpoint_pc)))
     {
-      struct cleanup *old_cleanups = NULL;
+      struct cleanup *old_cleanups = make_cleanup (null_cleanup, NULL);
 
       if (RECORD_IS_USED)
-	old_cleanups = record_full_gdb_operation_disable_set ();
+	record_full_gdb_operation_disable_set ();
 
       /* When using hardware single-step, a SIGTRAP is reported for both
 	 a completed single-step and a software breakpoint.  Need to
@@ -3033,8 +3033,7 @@ adjust_pc_after_break (struct execution_control_state *ecs)
 	  || ecs->event_thread->prev_pc == breakpoint_pc)
 	regcache_write_pc (regcache, breakpoint_pc);
 
-      if (RECORD_IS_USED)
-	do_cleanups (old_cleanups);
+      do_cleanups (old_cleanups);
     }
 }
 
-- 
1.8.1.4

[PATCH 37/40] fix breakpoint_1

[Tom Tromey, Tom Tromey]

This is a stylistic change to make breakpoint_1 analyzable.

The issue here is maybe fixable in the analyzer with some work.

	* breakpoint.c (breakpoint_1): Introduce an outer null cleanup.
---
 gdb/breakpoint.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1cfed3b..a9f1921 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6372,16 +6372,15 @@ breakpoint_1 (char *args, int allflag,
 	}
     }
 
+  bkpttbl_chain = make_cleanup (null_cleanup, NULL);
   if (opts.addressprint)
-    bkpttbl_chain 
-      = make_cleanup_ui_out_table_begin_end (uiout, 6,
-					     nr_printable_breakpoints,
-                                             "BreakpointTable");
+    make_cleanup_ui_out_table_begin_end (uiout, 6,
+					 nr_printable_breakpoints,
+					 "BreakpointTable");
   else
-    bkpttbl_chain 
-      = make_cleanup_ui_out_table_begin_end (uiout, 5,
-					     nr_printable_breakpoints,
-                                             "BreakpointTable");
+    make_cleanup_ui_out_table_begin_end (uiout, 5,
+					 nr_printable_breakpoints,
+					 "BreakpointTable");
 
   if (nr_printable_breakpoints > 0)
     annotate_breakpoints_headers ();
-- 
1.8.1.4

Re: [PATCH 37/40] fix breakpoint_1

[Tom Tromey]

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

Tom> This is a stylistic change to make breakpoint_1 analyzable.
Tom> The issue here is maybe fixable in the analyzer with some work.

I have dropped this patch.  With the new version of the checker, it is
not needed.

Tom

[PATCH 07/40] fix linespec bug noticed by the checker

[Tom Tromey, Tom Tromey]

This fixes a linespec bug noticed by the cleanup checker.

find_linespec_symbols did this:

  cleanup = demangle_for_lookup (name, state->language->la_language,
				 &lookup_name);
[...]
      cleanup = make_cleanup (xfree, canon);

But this is wrong, as it makes a subsequent call to do_cleanups not
clean up all the local state.

	* linespec.c (find_linespec_symbols): Don't reassign to 'cleanup'.
---
 gdb/linespec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 1c7a7a0..4bbd6e4 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -3093,7 +3093,7 @@ find_linespec_symbols (struct linespec_state *state,
   if (canon != NULL)
     {
       lookup_name = canon;
-      cleanup = make_cleanup (xfree, canon);
+      make_cleanup (xfree, canon);
     }
 
   /* It's important to not call expand_symtabs_matching unnecessarily
-- 
1.8.1.4

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Phil Muldoon]

On 09/05/13 19:47, Tom Tromey wrote:
> This series adds a cleanup checker and then fixes most of the bugs it
> notices.
> 
> The series as a whole was built and regression-tested on x86-64 Fedora
> 18.  However, there are a couple of patches that I could not test;
> e.g., the machoread.c patch.  I would appreciate it if someone could
> try these on their machine.  The branch is archer.git tromey/cleanup-checker.
> 
> In most cases the patches are pretty obvious.  I kept them short for
> ease of reading.
> 
> The cleanup checker itself is described in patch #1.  This patch also
> describes some of the 
> 
> After this series, there are still some cleanup-related bugs noticed
> by the checker.  One of these (in dwarf2read.c) is a false report --
> the checker is not perfect and there was no nice workaround here.
> 
> 
> The remaining errors are:

Thanks for doing this, I think it is awesome.  I am not going to
comment on the other thirty nine patches as the fixes are largely
mechanical in nature.

My questions refer to future cleanup issues.

As evidenced by the mail on gdb@sourceware.org on clean-up issues, I am
more interested in how your contribution can work toward future
cleanup errors.

Will your cleanup checker run as part of the normal build?

(or testsuite)?

(or ARI)?

Or will it be a manual thing?

If it is manual, can it be made automatic?

I really like the idea of automatic approaches for catching errors
like these.  If possible I think it should be part of the everyday
build process (assuming one has Python installed).

Given the number of errors this tool identified, I think these are
common logic errors?

What, if any, are the limitations of this, and other, GCC plugins to
fixing common GDB patterns of errors in a common build scenario?  

Cheers,

Phil

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> Thanks for doing this, I think it is awesome.  I am not going to
Phil> comment on the other thirty nine patches as the fixes are largely
Phil> mechanical in nature.

It wouldn't hurt if someone gave them a second look :)

Phil> Will your cleanup checker run as part of the normal build?
Phil> (or testsuite)?
Phil> (or ARI)?
Phil> Or will it be a manual thing?
Phil> If it is manual, can it be made automatic?

It requires GCC, the GCC Python plugin, and extra arguments to the
compiler to tell it to load the checker script.  So I think it won't
ever be fully automatic.

It is pretty easy to run, though.

I looked just now at having it run using the installed gcc-with-python2,
or using the installed GCC python .so.  I think I can make some more
improvements to make it even simpler.  Though it isn't too hard:

yum install gcc-python2-plugin
make CC='gcc-with-python2 /full/path/to/check_cleanup.py'

Phil> Given the number of errors this tool identified, I think these are
Phil> common logic errors?

Yes.

Phil> What, if any, are the limitations of this, and other, GCC plugins to
Phil> fixing common GDB patterns of errors in a common build scenario?  

The main limitation is the number of false reports it gives.  Also see
patch #36.

Tom

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Joel Brobecker]

> This series adds a cleanup checker and then fixes most of the bugs it
> notices.

Thanks, Tom. Amazing how easy it is to miss a cleanup, and how many
errors this tool found as a result...

I like it! I am wondering if this is something that we could run
nightly, either as part of the ARI, or in addition to it?
-- 
Joel

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I like it! I am wondering if this is something that we could run
Joel> nightly, either as part of the ARI, or in addition to it?

Does the ARI run do a full build?

I assume most people working on gdb use gcc.  It is really easy to build
the plugin.  We could just ask developers to try it on their patch as
part of submitting it.


More ideally I'd like gdb to use gerrit and have patches be
automatically checked as part of the submission process.  I think this
would be better than post hoc checking, or requiring everybody to have a
long checklist of things to do.

Tom

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Joel Brobecker]

> Does the ARI run do a full build?

I do not think so.

> I assume most people working on gdb use gcc.  It is really easy to
> build the plugin.  We could just ask developers to try it on their
> patch as part of submitting it.

I am not sure that the AdaCore compiler has support for Python
plugins, though... And even then, we wouldn't have it for all
platforms I develop on. Therefore...

> More ideally I'd like gdb to use gerrit and have patches be
> automatically checked as part of the submission process.  I think this
> would be better than post hoc checking, or requiring everybody to have a
> long checklist of things to do.

... I like this approach.

-- 
Joel

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Tom Tromey]

>> More ideally I'd like gdb to use gerrit and have patches be
>> automatically checked as part of the submission process.  I think this
>> would be better than post hoc checking, or requiring everybody to have a
>> long checklist of things to do.

Joel> ... I like this approach.

Yeah.  It has to be a long-term thing though.
I think it is a substantial amount of work to set it all up.

Tom

Re: [PATCH 00/40] add cleanup checker and fix cleanup bugs

[Tom Tromey]

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

Tom> This series adds a cleanup checker and then fixes most of the bugs it
Tom> notices.

I am checking most of this in.

For now, I'm leaving out the patch that adds the new "dangling_cleanup"
attribute.  I'm not completely comfortable with papering over problems
like that, even hard ones; and Pedro found a couple of spots in it where
the problem could be fixed without too much trouble.

I'm also not checking in the scoped cleanup change -- it wasn't part of
this series, but it is on the branch, for those looking there.  I plan
to revise and resubmit this one.

Tom> The series as a whole was built and regression-tested on x86-64
Tom> Fedora 18.

I rebased and re-tested this.

Tom