* FYI: more cleanup fixes @ 2011-06-30 18:47 Tom Tromey 0 siblings, 0 replies; 4+ messages in thread From: Tom Tromey @ 2011-06-30 18:47 UTC (permalink / raw) To: gdb-patches I am checking this in on the trunk. I've updated the GCC plugin a little and it caught a few more obviously incorrect uses of cleanups. Built and regtested by the buildbot. There are still a number of failures where the fix was not as obvious: ../../archer/gdb/remote-mips.c: In function ‘mips_initialize’: ../../archer/gdb/remote-mips.c:1419:7: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/remote-mips.c: In function ‘mips_exit_debug’: ../../archer/gdb/remote-mips.c:1400:1: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/remote-mips.c: In function ‘common_open’: ../../archer/gdb/remote-mips.c:1659:1: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/linux-nat.c: In function ‘linux_nat_info_proc_cmd’: ../../archer/gdb/linux-nat.c:4887:1: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/linux-nat.c: In function ‘linux_child_pid_to_exec_file’: ../../archer/gdb/linux-nat.c:4197:1: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/linux-nat.c: In function ‘linux_nat_make_corefile_notes’: ../../archer/gdb/linux-nat.c:4601:3: error: cleanup stack is not empty at return [-fpermissive] The linux_child_pid_to_exec_file one is interesting -- nearly all of the existing pid_to_exec_file methods return malloc'd memory, despite the spec in target.h. Perhaps we should change the spec, I think only the Windows variant would need a minor tweak. ../../archer/gdb/breakpoint.c: In function ‘breakpoint_re_set_default’: ../../archer/gdb/breakpoint.c:11448:1: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/ada-lang.c: In function ‘ada_make_symbol_completion_list’: ../../archer/gdb/ada-lang.c:5649:5: error: cleanup stack is not empty at return [-fpermissive] ../../archer/gdb/record.c: In function ‘record_wait_1’: ../../archer/gdb/record.c:1466:1: error: cleanup stack is not empty at return [-fpermissive] This one seem particularly bad -- lots of return paths without running cleanups. Tom 2011-06-30 Tom Tromey <tromey@redhat.com> * symfile-mem.c (symbol_file_add_from_memory): Call do_cleanups. * solib-svr4.c (open_symbol_file_object): Call do_cleanups on all return paths. Defer final do_cleanups until last return. * arm-tdep.c (arm_exidx_new_objfile): Make null cleanup after early return. diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 433ce21..1a75af1 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -2211,7 +2211,7 @@ arm_obj_section_from_vma (struct objfile *objfile, bfd_vma vma) static void arm_exidx_new_objfile (struct objfile *objfile) { - struct cleanup *cleanups = make_cleanup (null_cleanup, NULL); + struct cleanup *cleanups; struct arm_exidx_data *data; asection *exidx, *extab; bfd_vma exidx_vma = 0, extab_vma = 0; @@ -2222,6 +2222,7 @@ arm_exidx_new_objfile (struct objfile *objfile) /* If we've already touched this file, do nothing. */ if (!objfile || objfile_data (objfile, arm_exidx_data_key) != NULL) return; + cleanups = make_cleanup (null_cleanup, NULL); /* Read contents of exception table and index. */ exidx = bfd_get_section_by_name (objfile->obfd, ".ARM.exidx"); diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c index f668f83..d92a83c 100644 --- a/gdb/solib-svr4.c +++ b/gdb/solib-svr4.c @@ -1021,17 +1021,26 @@ open_symbol_file_object (void *from_ttyp) if (symfile_objfile) if (!query (_("Attempt to reload symbols from process? "))) - return 0; + { + do_cleanups (cleanups); + return 0; + } /* Always locate the debug struct, in case it has moved. */ info->debug_base = 0; if (locate_base (info) == 0) - return 0; /* failed somehow... */ + { + do_cleanups (cleanups); + return 0; /* failed somehow... */ + } /* First link map member should be the executable. */ lm = solib_svr4_r_map (info); if (lm == 0) - return 0; /* failed somehow... */ + { + do_cleanups (cleanups); + return 0; /* failed somehow... */ + } /* Read address of name from target memory to GDB. */ read_memory (lm + lmo->l_name_offset, l_name_buf, l_name_size); @@ -1039,11 +1048,11 @@ open_symbol_file_object (void *from_ttyp) /* Convert the address to host format. */ l_name = extract_typed_address (l_name_buf, ptr_type); - /* Free l_name_buf. */ - do_cleanups (cleanups); - if (l_name == 0) - return 0; /* No filename. */ + { + do_cleanups (cleanups); + return 0; /* No filename. */ + } /* Now fetch the filename from target memory. */ target_read_string (l_name, &filename, SO_NAME_MAX_PATH_SIZE - 1, &errcode); @@ -1053,12 +1062,14 @@ open_symbol_file_object (void *from_ttyp) { warning (_("failed to read exec filename from attached file: %s"), safe_strerror (errcode)); + do_cleanups (cleanups); return 0; } /* Have a pathname: read the symbol file. */ symbol_file_add_main (filename, from_tty); + do_cleanups (cleanups); return 1; } diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c index 6da5a1c..bef28c7 100644 --- a/gdb/symfile-mem.c +++ b/gdb/symfile-mem.c @@ -72,6 +72,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name, bfd_vma loadbase; struct section_addr_info *sai; unsigned int i; + struct cleanup *cleanup; if (bfd_get_flavour (templ) != bfd_target_elf_flavour) error (_("add-symbol-file-from-memory not supported for this target")); @@ -97,7 +98,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name, } sai = alloc_section_addr_info (bfd_count_sections (nbfd)); - make_cleanup (xfree, sai); + cleanup = make_cleanup (xfree, sai); i = 0; for (sec = nbfd->sections; sec != NULL; sec = sec->next) if ((bfd_get_section_flags (nbfd, sec) & (SEC_ALLOC|SEC_LOAD)) != 0) @@ -114,6 +115,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name, /* This might change our ideas about frames already looked at. */ reinit_frame_cache (); + do_cleanups (cleanup); return objf; } ^ permalink raw reply [flat|nested] 4+ messages in thread
* FYI: more cleanup fixes
@ 2011-06-30 19:29 Tom Tromey
2011-07-01 2:02 ` Yao Qi
0 siblings, 1 reply; 4+ messages in thread
From: Tom Tromey @ 2011-06-30 19:29 UTC (permalink / raw)
To: gdb-patches
I'm checking this in on the trunk.
One last round of cleanup fixes.
Built and regtested by the buildbot.
I neglected to mention in the last email: if anybody wants the latest
version of the plugin, just let me know.
Tom
2011-06-30 Tom Tromey <tromey@redhat.com>
* varobj.c (varobj_create): Call do_cleanups on early exit path.
* valops.c (find_overload_match): Call do_cleanups on early exit
path.
* solib.c (solib_find): Call do_cleanups on early exit path.
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.148
diff -u -r1.148 solib.c
--- solib.c 17 Apr 2011 18:38:45 -0000 1.148
+++ solib.c 30 Jun 2011 19:26:24 -0000
@@ -254,6 +254,7 @@
if (remote_filename_p (temp_pathname))
{
*fd = -1;
+ do_cleanups (old_chain);
return temp_pathname;
}
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.280
diff -u -r1.280 valops.c
--- valops.c 27 Jun 2011 19:21:50 -0000 1.280
+++ valops.c 30 Jun 2011 19:26:24 -0000
@@ -2585,6 +2585,7 @@
if (*valp)
{
*staticp = 1;
+ do_cleanups (all_cleanups);
return 0;
}
}
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.178
diff -u -r1.178 varobj.c
--- varobj.c 24 Jun 2011 19:47:37 -0000 1.178
+++ varobj.c 30 Jun 2011 19:26:24 -0000
@@ -580,6 +580,7 @@
return a sensible error. */
if (!gdb_parse_exp_1 (&p, block, 0, &var->root->exp))
{
+ do_cleanups (old_chain);
return NULL;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: FYI: more cleanup fixes 2011-06-30 19:29 Tom Tromey @ 2011-07-01 2:02 ` Yao Qi 2011-07-01 13:24 ` Tom Tromey 0 siblings, 1 reply; 4+ messages in thread From: Yao Qi @ 2011-07-01 2:02 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 07/01/2011 03:29 AM, Tom Tromey wrote: > I neglected to mention in the last email: if anybody wants the latest > version of the plugin, just let me know. Tom, I'd like to have a try on this plugin. How can I get it? -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: FYI: more cleanup fixes 2011-07-01 2:02 ` Yao Qi @ 2011-07-01 13:24 ` Tom Tromey 0 siblings, 0 replies; 4+ messages in thread From: Tom Tromey @ 2011-07-01 13:24 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches [-- 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() ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-01 13:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-30 18:47 FYI: more cleanup fixes Tom Tromey 2011-06-30 19:29 Tom Tromey 2011-07-01 2:02 ` Yao Qi 2011-07-01 13:24 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox