Re: FYI: more cleanup fixes

[Tom Tromey <tromey@redhat.com>]

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

Tom> I neglected to mention in the last email: if anybody wants the latest
Tom> version of the plugin, just let me know.

Yao> Tom, I'd like to have a try on this plugin.  How can I get it?

1. Check out the Python plugin:

    https://fedorahosted.org/gcc-python-plugin/

2. Apply the attached patch.

3. Build the plugin.  It builds without fuss on Fedora 15 with the
   appropriate prereqs installed, I don't know about other distros.

4. Rebuild gdb with the plugin.  I use:

   export PYTHONPATH=/home/tromey/Space/Trunk/gcc-python-plugin/
   make -C /home/tromey/gnu/archer/build/gdb/ -k CFLAGS='-g -O -fplugin=/home/tromey/Space/Trunk/gcc-python-plugin/python.so -fplugin-arg-python-script=/home/tromey/Space/Trunk/gcc-python-plugin/check_cleanups.py' 


Note that the cleanup checker is not bulletproof.  It gives some
incorrect errors, so you have to analyze each one.

If you happen to patch it for some reason, I'm interested in your
changes.

Tom


[-- Attachment #2: the patch --]
[-- Type: application/octet-stream, Size: 9730 bytes --]

diff --git a/check_cleanups.py b/check_cleanups.py
new file mode 100644
index 0000000..4a1e40f
--- /dev/null
+++ b/check_cleanups.py
@@ -0,0 +1,173 @@
+import gcc
+import gccutils
+import sys
+
+want_raii_info = False
+
+def log(msg, indent=0):
+    if False:
+        sys.stderr.write('%s%s\n' % ('  ' * indent, msg))
+
+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
+
+special_names = set(['do_final_cleanups', 'discard_final_cleanups',
+                     'save_cleanups', 'save_final_cleanups',
+                     'restore_cleanups', 'restore_final_cleanups',
+                     'exceptions_state_mc_init'])
+
+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):
+    pass
+
+class CleanupChecker:
+    def __init__(self, fun):
+        self.fun = fun
+
+    def compare_vars(self, loc, definition, argument, bb_from):
+        if definition == argument:
+            return True
+        if not isinstance(argument, gcc.SsaName):
+            log('[not an ssaname]', 4)
+            return False
+        stmt = argument.def_stmt
+        if not isinstance(stmt, gcc.GimplePhi):
+            log('[definition not a Phi]', 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
+
+        return definition == argument
+
+    def compute_master(self, bb, bb_from, master_cleanup):
+        if not isinstance(bb.gimple, list):
+            return None
+        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
+                    if master_cleanup is None:
+                        master_cleanup = stmt.lhs
+                        if master_cleanup is None:
+                            # Maybe trigger another error later.
+                            master_cleanup = Dummy()
+                        log('...installed new master cleanup %s' % str(master_cleanup), 2)
+                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)
+                    # See if we're cleaning up the master cleanup.
+                    if self.compare_vars(curloc, master_cleanup, stmt.args[0],
+                                         bb_from):
+                        master_cleanup = None
+                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 self.compare_vars(curloc, master_cleanup, stmt.retval, bb_from):
+                        gcc.permerror(curloc,
+                                      'constructor does not return master cleanup')
+                elif not self.is_special_constructor:
+                    log('GOT HERE: ' + str(master_cleanup), 2)
+                    if master_cleanup is not None:
+                        gcc.permerror(curloc,
+                                      'cleanup stack is not empty at return')
+                        # sys.stderr.write('master cleanup = %s, bb_from = %d\n' % (str(master_cleanup), bb_from))
+                        # gccutils.write_dot_image(gccutils.cfg_to_dot(self.fun.cfg),
+                        #                          self.fun.decl.name)
+        log('noting master cleanup %s in bb %d' % (str(master_cleanup), bb.index), 2)
+        return master_cleanup
+
+    def traverse_bbs(self, bb, bb_from, entry_master):
+        if bb.index in self.master_cleanups:
+            # FIXME do some checking
+            return
+
+        self.master_cleanups[bb.index] = self.compute_master(bb, bb_from, entry_master)
+        # Now propagate to successor nodes.
+        for edge in bb.succs:
+            self.traverse_bbs(edge.dest, bb.index, self.master_cleanups[bb.index])
+        return
+
+    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 = {}
+
+        self.traverse_bbs(self.fun.cfg.entry, -1, None)
+        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'
+
+def on_pass_execution(optpass, fun, *args, **kwargs):
+    if optpass.name == 'release_ssa':
+        if fun.decl:
+            log("Starting " + fun.decl.name)
+        checker = CleanupChecker(fun)
+        what = checker.check_cleanups()
+        if fun.decl:
+            log(fun.decl.name + ': ' + what, 2)
+
+def main(**kwargs):
+    gcc.register_callback(gcc.PLUGIN_PASS_EXECUTION,
+                          on_pass_execution, **kwargs)
+
+main()
diff --git a/gccutils.py b/gccutils.py
index 4eba0f0..f610fff 100644
--- a/gccutils.py
+++ b/gccutils.py
@@ -37,7 +37,7 @@ def get_variables_as_dict():
         result[var.decl.name] = var
     return result
 
-def invoke_dot(dot):
+def write_dot_image(dot, basename):
     from subprocess import Popen, PIPE
 
     if 1:
@@ -53,10 +53,12 @@ def invoke_dot(dot):
         # Presumably a font selection/font metrics issue
         fmt = 'svg'
 
-    p = Popen(['dot', '-T%s' % fmt, '-o', 'test.%s' % fmt],
+    p = Popen(['dot', '-T%s' % fmt, '-o', '%s.%s' % (basename, fmt)],
               stdin=PIPE)
     p.communicate(dot.encode('ascii'))
 
+def invoke_dot(dot):
+    write_dot_image(dot, 'test')
     p = Popen(['xdg-open', 'test.%s' % fmt])
     p.communicate()
 
@@ -302,14 +304,21 @@ class CfgPrettyPrinter(DotPrettyPrinter):
             for stmtidx, stmt in enumerate(bb.gimple):
                 if curloc != stmt.loc:
                     curloc = stmt.loc
-                    code = get_src_for_loc(stmt.loc).rstrip()
+                    if curloc:
+                        code = get_src_for_loc(curloc).rstrip()
+                        line = curloc.line
+                        column = curloc.column
+                    else:
+                        code = '<<no code>>'
+                        line = 0
+                        column = 0
                     pseudohtml = self.code_to_html(code)
                     # print('pseudohtml: %r' % pseudohtml)
                     result += ('<tr><td align="left">'
-                               + self.to_html('%4i ' % stmt.loc.line)
+                               + self.to_html('%4i ' % line)
                                + pseudohtml
                                + '<br/>'
-                               + (' ' * (5 + stmt.loc.column-1)) + '^'
+                               + (' ' * (5 + column-1)) + '^'
                                + '</td></tr>')
                     
                 result += '<tr><td></td>' + self.stmt_to_html(stmt, stmtidx) + '</tr>\n'
@@ -473,8 +482,8 @@ class TreePrettyPrinter(DotPrettyPrinter):
         result += '}\n'
         return result
 
-def cfg_to_dot(name, cfg):
-    pp = CfgPrettyPrinter(name, cfg)
+def cfg_to_dot(cfg, name = None):
+    pp = CfgPrettyPrinter(cfg, name)
     return pp.to_dot()