This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH, RFC] warn about raw pointers that can be unique_ptr<T>


From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>

Hi,

This is a start at warning about various resource allocation issues that can be
improved.  Currently it only warns about functions that call malloc and then
always pass the resulting pointer to free().

It should be pretty simple to extend this to new/delete and new[]/delete[], as
well as checking that in simple cases the pairing is correct.  However it
wasn't obvious to me how to tell if a function call is to a allocating operator
new.  We probably don't want to warn about placement new since complicated
things can be happening there.  There is the DECL_IS_OPERATOR_NEW() but that
doesn't seem to cover class specific operator new overloads, and maybe even
custom ones at global scope?

Other things that may be reasonable to try and include would be open / close
and locks.

It might also be good to warn about functions that take or return unique
ownership of resources, but I'm not quiet sure how to handle functions that
allocate or deallocate shared resources.

bootstrapped but not yet regtested other than the included test on
x86_64-linux-gnu.  All comments and suggestions welcome.

Trev


  ---
 gcc/Makefile.in                          |   1 +
 gcc/c-family/c.opt                       |   4 +
 gcc/gimple-owned-ptr.c                   | 214 +++++++++++++++++++++++++++++++
 gcc/passes.def                           |   1 +
 gcc/testsuite/g++.dg/warn/Wowning-ptr1.C |  76 +++++++++++
 gcc/tree-pass.h                          |   1 +
 6 files changed, 297 insertions(+)
 create mode 100644 gcc/gimple-owned-ptr.c
 create mode 100644 gcc/testsuite/g++.dg/warn/Wowning-ptr1.C

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index f675e073ecc..d33e8591b03 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1294,6 +1294,7 @@ OBJS = \
 	gimple-laddress.o \
 	gimple-low.o \
 	gimple-pretty-print.o \
+	gimple-owned-ptr.o \
 	gimple-ssa-backprop.o \
 	gimple-ssa-isolate-paths.o \
 	gimple-ssa-nonnull-compare.o \
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 8697eb13108..6f12c3ac5eb 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -428,6 +428,10 @@ Wctor-dtor-privacy
 C++ ObjC++ Var(warn_ctor_dtor_privacy) Warning
 Warn when all constructors and destructors are private.
 
+Wowning-ptr
+C++ ObjC++ Var(warn_owning_ptr) Warning
+Warn about pointers that can be marked as owning the resource they refer to.
+
 Wdangling-else
 C ObjC C++ ObjC++ Var(warn_dangling_else) Warning LangEnabledBy(C ObjC C++ ObjC++,Wparentheses)
 Warn about dangling else.
diff --git a/gcc/gimple-owned-ptr.c b/gcc/gimple-owned-ptr.c
new file mode 100644
index 00000000000..f1283940920
--- /dev/null
+++ b/gcc/gimple-owned-ptr.c
@@ -0,0 +1,214 @@
+/* Detection of allocations that are owned by a single function.
+   Copyright (C) 2015-2017 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC 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, or (at your option) any later
+version.
+
+GCC 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 GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "basic-block.h"
+#include "options.h"
+#include "flags.h"
+#include "stmt.h"
+#include "gimple-iterator.h"
+#include "tree-cfg.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "diagnostic-core.h"
+
+/*  Return true if gcall calls a function in the malloc family where the memory
+ *  can be released by calling free().  */
+
+static bool
+is_malloc_alloc (gcall *call)
+{
+  if (!gimple_call_builtin_p (call))
+    return false;
+
+  tree callee = gimple_call_fndecl (call);
+  built_in_function builtin = DECL_FUNCTION_CODE (callee);
+  if (builtin != BUILT_IN_MALLOC
+      && builtin != BUILT_IN_CALLOC
+      && builtin != BUILT_IN_ALIGNED_ALLOC)
+    return false;
+
+  return true;
+}
+
+/* A free_fn_p implementation for free().  We don't need to check what's being
+   passed to free() because the resource in question is used in the statement,
+   and free() should only use one thing.  */
+
+static bool
+is_malloc_free (gcall *call, tree)
+{
+      return gimple_call_builtin_p (call, BUILT_IN_FREE);
+}
+
+namespace {
+
+class pass_owned_ptrs : public gimple_opt_pass
+{
+public:
+  pass_owned_ptrs (gcc::context *ctxt) : gimple_opt_pass (data, ctxt) {}
+
+  static const pass_data data;
+  virtual bool
+  gate (function *)
+  {
+    return true;
+  }
+  virtual unsigned int
+  execute (function *fun);
+
+private:
+  /* Should return true if the call is to a function that allocates some sort
+     of resource.  It is assumed the lhs of the call is a reference to the
+     resource.  */
+  typedef bool (*alloc_fn_p)(gcall *);
+
+  /* Should return true if the call releases the resource.  */
+  typedef bool (*free_fn_p)(gcall *, tree);
+
+  void collect_frees (free_fn_p is_free, tree var,
+		      bitmap freeing_blocks) const;
+  bool always_reaches_free (basic_block bb, const bitmap freeing_blocks) const;
+  void detect_owned (alloc_fn_p is_alloc, free_fn_p is_free) const;
+
+  function *m_fun;
+};
+
+unsigned int
+pass_owned_ptrs::execute (function *fun)
+      {
+	m_fun = fun;
+	detect_owned (is_malloc_alloc, is_malloc_free);
+	return 0;
+      }
+
+void
+pass_owned_ptrs::detect_owned (alloc_fn_p is_alloc, free_fn_p is_free) const
+{
+	basic_block bb;
+	FOR_EACH_BB_FN (bb, m_fun)
+	  {
+	    for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	      {
+		gcall *call = dyn_cast<gcall *> (gsi_stmt (gsi));
+		if (!call || !is_alloc (call))
+		  continue;
+
+		tree lhs = gimple_call_lhs (call);
+		if (!lhs)
+		  continue;
+
+		auto_bitmap freeing_blocks;
+		collect_frees (is_free, lhs, freeing_blocks);
+		if (bitmap_empty_p (freeing_blocks))
+		  continue;
+
+		if (!always_reaches_free (bb, freeing_blocks))
+		  continue;
+
+		warning_at (gimple_location (call), OPT_Wowning_ptr, "result of allocation can be owned by this function");
+	      }
+	  }
+      }
+
+/* Collect the set of places where the value in var is passed to free.  */
+
+void
+pass_owned_ptrs::collect_frees (free_fn_p is_free, tree var,
+				bitmap freeing_blocks) const
+{
+  imm_use_iterator iter;
+  use_operand_p use;
+  FOR_EACH_IMM_USE_FAST (use, iter, var)
+    {
+      gcall *use_stmt = dyn_cast<gcall *> (USE_STMT (use));
+      if (!use_stmt || !is_free (use_stmt, var))
+	continue;
+      if (!gimple_call_builtin_p (use_stmt, BUILT_IN_FREE))
+	continue;
+
+      basic_block bb = gimple_bb (use_stmt);
+      bitmap_set_bit (freeing_blocks, bb->index);
+    }
+}
+
+/* Return true if allocating in bb means we must free the resource in one of
+ * the blocks where it is freed, and false if there is a path on which it is
+   not freed.
+
+   This is the same as checking if the set of freeing blocks as a whole post
+   domminates bb.  However there isn't an immediately obvious way to check that
+   so we just do a bredth first search from bb looking for the exit block and
+   stopping any path that goes through a block in freeing_blocks.  */
+
+bool
+pass_owned_ptrs::always_reaches_free (basic_block bb, const bitmap freeing_blocks) const
+{
+  auto_bitmap worklist, visited;
+  bitmap_set_bit (worklist, bb->index);
+  while (!bitmap_empty_p (worklist)) {
+      unsigned int bb_idx = bitmap_first_set_bit (worklist);
+      bitmap_clear_bit (worklist, bb_idx);
+      if (bb_idx == EXIT_BLOCK)
+	return false;
+
+      if (bitmap_bit_p (freeing_blocks, bb_idx))
+	continue;
+
+      basic_block current = BASIC_BLOCK_FOR_FN (m_fun, bb_idx);
+      edge_iterator ei;
+      edge e;
+      FOR_EACH_EDGE (e, ei, current->succs)
+	{
+	  unsigned int succ_idx = e->dest->index;
+	  if (bitmap_bit_p (visited, succ_idx))
+	    continue;
+
+	  bitmap_set_bit (worklist, succ_idx);
+	  bitmap_set_bit (visited, succ_idx);
+	}
+  }
+
+      return true;
+}
+
+const pass_data pass_owned_ptrs::data = {
+  GIMPLE_PASS,		       /* type */
+  "owned_ptrs", /* name */
+  OPTGROUP_NONE,	       /* optinfo_flags */
+  TV_NONE,		       /* tv_id */
+  (PROP_cfg),		       /* properties_required */
+  0,			       /* properties_provided */
+  0,			       /* properties_destroyed */
+  0,			       /* todo_flags_start */
+  0,	     /* todo_flags_finish */
+};
+
+} // anon namespace
+
+gimple_opt_pass *
+make_pass_warn_owned_ptrs (gcc::context *ctxt)
+{
+  return new pass_owned_ptrs (ctxt);
+}
diff --git a/gcc/passes.def b/gcc/passes.def
index 6b0f05b07bd..1236eb96ec0 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_early_warn_uninitialized);
       NEXT_PASS (pass_nothrow);
       NEXT_PASS (pass_rebuild_cgraph_edges);
+  NEXT_PASS (pass_warn_owned_ptrs);
   POP_INSERT_PASSES ()
 
   NEXT_PASS (pass_chkp_instrumentation_passes);
diff --git a/gcc/testsuite/g++.dg/warn/Wowning-ptr1.C b/gcc/testsuite/g++.dg/warn/Wowning-ptr1.C
new file mode 100644
index 00000000000..7064ba40b47
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wowning-ptr1.C
@@ -0,0 +1,76 @@
+// { dg-do compile }
+// { dg-options "-Wowning-ptr" }
+
+int *
+foo ()
+{
+	int *x = (int *) __builtin_malloc (sizeof (int));
+	return x;
+}
+
+void
+bar ()
+{
+	void *x = __builtin_malloc (4); // { dg-warning "result of allocation can be owned by this function" }
+	__builtin_free (x);
+}
+
+void
+baz (bool b)
+{
+	void *x = __builtin_malloc (4); // { dg-warning "result of allocation can be owned by this function" }
+	if (b)
+	{
+		bar ();
+		__builtin_free (x);
+	return;
+	}
+
+	__builtin_free(x);
+}
+
+void
+foofoo (bool b)
+{
+	void *x = __builtin_malloc (4);
+	if (b)
+	{
+		__builtin_free (x);
+	}
+}
+
+struct s { void *p; };
+
+void
+barbar (s *ptr)
+{
+	ptr->p = __builtin_malloc (4);
+}
+
+s
+qux (bool b)
+{
+	s c = { 0 };
+	void *x = __builtin_malloc (4);
+	if (b)
+		__builtin_free (x);
+	else
+		c.p = x;
+
+	return c;
+}
+
+void something (int *);
+
+void
+blah ()
+{
+	int *x = (int *) __builtin_malloc (sizeof (int) * 100); // { dg-warning "result of allocation can be owned" }
+	for (int i = 0; i < 100; i++)
+	{
+		something (x);
+		something (&x[i]);
+	}
+
+	__builtin_free (x);
+}
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 85bfba7ac28..b7d6202ffda 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -426,6 +426,7 @@ extern gimple_opt_pass *make_pass_post_ipa_warn (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_stdarg (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_early_warn_uninitialized (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_late_warn_uninitialized (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_warn_owned_ptrs (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_cse_reciprocals (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_cse_sincos (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_optimize_bswap (gcc::context *ctxt);
-- 
2.11.0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]