diff mbox

[Pointer,Bounds,Checker,27/x] Strlen

Message ID 20140805124837.GA6691@msticlxl57.ims.intel.com
State New
Headers show

Commit Message

Ilya Enkovich Aug. 5, 2014, 12:48 p.m. UTC 
eOn 31 Jul 14:07, Ilya Enkovich wrote:
> 2014-06-11 12:22 GMT+04:00 Jakub Jelinek <jakub@redhat.com>:
> > On Wed, Jun 11, 2014 at 12:14:14PM +0400, Ilya Enkovich wrote:
> >> This patch adds instrumented code support for strlen optimization.
> >>
> >> Bootstrapped and tested on linux-x86_64.
> >>
> >> Does it look OK?
> >
> > Ugh, this looks terribly ugly.  So you are changing the meaning of arguments
> > of every single builtin that has pointer arguments, so all spots that
> > work with those builtins have to take into account
> > gimple_call_with_bounds_p and do something different?
> > That is very error prone and maintainance nightmare.
> >
> >         Jakub
> 
> Hi Jakub,
> 
> I've tried an approach with internal functions usage as you suggested
> on Cauldron.
> 
> Shortly about used scheme:
>   - only a selected set of builtin functions is instrumented
>   - during instrumentation builtin call is replaced with internal function call
> 
> I tried to implement it and faced few issues:
> 
> 1. On expand pass we need to expand all those internal function and
> since they should be expanded as regular instrumented calls, I need to
> create a CALL_EXPR tree for that. I have nothing to use as fndecl for
> this call except the same instrumented builtin fndecl I use right now.
> Therefore the problem of instrumented call with the same builtin
> function code would still exist during expand pass.
> 2. Some builtins actually may have body. If I instrument such call and
> replace it with internal call then I loose a possibility to inline it.
> If I do not replace it with internal call then I have to make an
> instrumented call and again I have to create and use instrumented
> builtin fndecl having the same situation I have now but this time only
> until call is inlined.
> 3. Usage of internal functions means we cannot support whole program
> instrumentation (or we have to declare internal function for each
> builtin) which may appear a very useful scheme with reduced memory and
> performance penalties.
> 
> Now I want to try another scheme. I want to change enum
> built_in_function so that each builtin code has a paired code to be
> used for instrumented version. I'm not going to actually declare
> additional builtins for users, I just want to obtain special function
> codes to be used for instrumented builtins and thus to not make
> current code handling builtins to work with instrumented calls.
> 
> Ilya


I've tried to use different function codes for instrumented builtin calls and this scheme looks better to me. It allows to work with instrumented builtin calls as with regular calls which is convenient. At the same time all current builtin call handlers are not affected. Here is a core change required:
diff mbox

Patch

diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index b70c262..5b8bb5a 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -168,6 +168,14 @@  enum built_in_class {
 enum built_in_function {
 #include "builtins.def"
 
+  BEGIN_CHKP_BUILTINS,
+
+#undef DEF_BUILTIN
+#define DEF_BUILTIN(ENUM, N, C, T, LT, B, F, NA, AT, IM, COND) ENUM##_CHKP,
+#include "builtins.def"
+
+  END_CHKP_BUILTINS,
+
   /* Complex division routines in libgcc.  These are done via builtins
      because emit_library_call_value can't handle complex values.  */
   BUILT_IN_COMPLEX_MUL_MIN,
@@ -1490,7 +1498,7 @@  struct GTY(()) tree_function_decl {
      DECL_FUNCTION_CODE.  Otherwise unused.
      ???  The bitfield needs to be able to hold all target function
 	  codes as well.  */
-  ENUM_BITFIELD(built_in_function) function_code : 11;
+  ENUM_BITFIELD(built_in_function) function_code : 12;
   ENUM_BITFIELD(built_in_class) built_in_class : 2;
 
   unsigned static_ctor_flag : 1;
@@ -1514,7 +1522,7 @@  struct GTY(()) tree_function_decl {
   unsigned has_debug_args_flag : 1;
   unsigned tm_clone_flag : 1;
   unsigned versioned_function : 1;
-  /* No bits left.  */
+  /* 31 bits left.  */
 };
 
 struct GTY(()) tree_translation_unit_decl {

Unfortunately tree_function_decl does not have any free bits to be used for increased enum size.  Suppose some flag may be moved into tree_decl_with_vis which has some free bits.

Please tell me if this approach with builtins looks OK and I would update affected patches then.

Strlen patch in this thread with new approach transforms from stability to performance patch.  Below is a modified version of this patch with new function codes introduced.

Thanks,
Ilya
--

diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index f55b7ee..93e88f7 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -50,6 +50,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "gimple-pretty-print.h"
 #include "params.h"
 #include "expr.h"
+#include "tree-chkp.h"
 
 /* A vector indexed by SSA_NAME_VERSION.  0 means unknown, positive value
    is an index into strinfo vector, negative value stands for
@@ -426,6 +427,7 @@  get_string_length (strinfo si)
   if (si->stmt)
     {
       gimple stmt = si->stmt, lenstmt;
+      bool with_bounds = gimple_call_with_bounds_p (stmt);
       tree callee, lhs, fn, tem;
       location_t loc;
       gimple_stmt_iterator gsi;
@@ -445,11 +447,20 @@  get_string_length (strinfo si)
 	{
 	case BUILT_IN_STRCAT:
 	case BUILT_IN_STRCAT_CHK:
+	case BUILT_IN_STRCAT_CHKP:
+	case BUILT_IN_STRCAT_CHK_CHKP:
 	  gsi = gsi_for_stmt (stmt);
 	  fn = builtin_decl_implicit (BUILT_IN_STRLEN);
 	  gcc_assert (lhs == NULL_TREE);
 	  tem = unshare_expr (gimple_call_arg (stmt, 0));
-	  lenstmt = gimple_build_call (fn, 1, tem);
+	  if (with_bounds)
+	    {
+	      lenstmt = gimple_build_call (chkp_maybe_create_clone (fn)->decl,
+					   2, tem, gimple_call_arg (stmt, 1));
+	      gimple_call_set_with_bounds (lenstmt, true);
+	    }
+	  else
+	    lenstmt = gimple_build_call (fn, 1, tem);
 	  lhs = make_ssa_name (TREE_TYPE (TREE_TYPE (fn)), lenstmt);
 	  gimple_call_set_lhs (lenstmt, lhs);
 	  gimple_set_vuse (lenstmt, gimple_vuse (stmt));
@@ -472,10 +483,14 @@  get_string_length (strinfo si)
 	  /* FALLTHRU */
 	case BUILT_IN_STRCPY:
 	case BUILT_IN_STRCPY_CHK:
-	  if (gimple_call_num_args (stmt) == 2)
+	case BUILT_IN_STRCPY_CHKP:
+	case BUILT_IN_STRCPY_CHK_CHKP:
+	  if (gimple_call_num_args (stmt) == (with_bounds ? 4 : 2))
 	    fn = builtin_decl_implicit (BUILT_IN_STPCPY);
 	  else
 	    fn = builtin_decl_explicit (BUILT_IN_STPCPY_CHK);
+	  if (with_bounds)
+	    fn = chkp_maybe_create_clone (fn)->decl;
 	  gcc_assert (lhs == NULL_TREE);
 	  if (dump_file && (dump_flags & TDF_DETAILS) != 0)
 	    {
@@ -494,6 +509,8 @@  get_string_length (strinfo si)
 	  /* FALLTHRU */
 	case BUILT_IN_STPCPY:
 	case BUILT_IN_STPCPY_CHK:
+	case BUILT_IN_STPCPY_CHKP:
+	case BUILT_IN_STPCPY_CHK_CHKP:
 	  gcc_assert (lhs != NULL_TREE);
 	  loc = gimple_location (stmt);
 	  si->endptr = lhs;
@@ -780,6 +797,7 @@  adjust_last_stmt (strinfo si, gimple stmt, bool is_strcat)
   tree vuse, callee, len;
   struct laststmt_struct last = laststmt;
   strinfo lastsi, firstsi;
+  unsigned len_arg_no = 2;
 
   laststmt.stmt = NULL;
   laststmt.len = NULL_TREE;
@@ -851,11 +869,15 @@  adjust_last_stmt (strinfo si, gimple stmt, bool is_strcat)
     case BUILT_IN_MEMCPY:
     case BUILT_IN_MEMCPY_CHK:
       break;
+    case BUILT_IN_MEMCPY_CHKP:
+    case BUILT_IN_MEMCPY_CHK_CHKP:
+      len_arg_no = 4;
+      break;
     default:
       return;
     }
 
-  len = gimple_call_arg (last.stmt, 2);
+  len = gimple_call_arg (last.stmt, len_arg_no);
   if (tree_fits_uhwi_p (len))
     {
       if (!tree_fits_uhwi_p (last.len)
@@ -879,7 +901,7 @@  adjust_last_stmt (strinfo si, gimple stmt, bool is_strcat)
   else
     return;
 
-  gimple_call_set_arg (last.stmt, 2, last.len);
+  gimple_call_set_arg (last.stmt, len_arg_no, last.len);
   update_stmt (last.stmt);
 }
 
@@ -970,11 +992,12 @@  handle_builtin_strchr (gimple_stmt_iterator *gsi)
   tree src;
   gimple stmt = gsi_stmt (*gsi);
   tree lhs = gimple_call_lhs (stmt);
+  bool with_bounds = gimple_call_with_bounds_p (stmt);
 
   if (lhs == NULL_TREE)
     return;
 
-  if (!integer_zerop (gimple_call_arg (stmt, 1)))
+  if (!integer_zerop (gimple_call_arg (stmt, with_bounds ? 2 : 1)))
     return;
 
   src = gimple_call_arg (stmt, 0);
@@ -1081,8 +1104,9 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
   gimple stmt = gsi_stmt (*gsi);
   strinfo si, dsi, olddsi, zsi;
   location_t loc;
+  bool with_bounds = gimple_call_with_bounds_p (stmt);
 
-  src = gimple_call_arg (stmt, 1);
+  src = gimple_call_arg (stmt, with_bounds ? 2 : 1);
   dst = gimple_call_arg (stmt, 0);
   lhs = gimple_call_lhs (stmt);
   idx = get_stridx (src);
@@ -1113,11 +1137,15 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
       {
       case BUILT_IN_STRCPY:
       case BUILT_IN_STRCPY_CHK:
+      case BUILT_IN_STRCPY_CHKP:
+      case BUILT_IN_STRCPY_CHK_CHKP:
 	if (lhs != NULL_TREE || !builtin_decl_implicit_p (BUILT_IN_STPCPY))
 	  return;
 	break;
       case BUILT_IN_STPCPY:
       case BUILT_IN_STPCPY_CHK:
+      case BUILT_IN_STPCPY_CHKP:
+      case BUILT_IN_STPCPY_CHK_CHKP:
 	if (lhs == NULL_TREE)
 	  return;
 	else
@@ -1216,16 +1244,19 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
   switch (bcode)
     {
     case BUILT_IN_STRCPY:
+    case BUILT_IN_STRCPY_CHKP:
       fn = builtin_decl_implicit (BUILT_IN_MEMCPY);
       if (lhs)
 	ssa_ver_to_stridx[SSA_NAME_VERSION (lhs)] = didx;
       break;
     case BUILT_IN_STRCPY_CHK:
+    case BUILT_IN_STRCPY_CHK_CHKP:
       fn = builtin_decl_explicit (BUILT_IN_MEMCPY_CHK);
       if (lhs)
 	ssa_ver_to_stridx[SSA_NAME_VERSION (lhs)] = didx;
       break;
     case BUILT_IN_STPCPY:
+    case BUILT_IN_STPCPY_CHKP:
       /* This would need adjustment of the lhs (subtract one),
 	 or detection that the trailing '\0' doesn't need to be
 	 written, if it will be immediately overwritten.
@@ -1237,6 +1268,7 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
 	}
       break;
     case BUILT_IN_STPCPY_CHK:
+    case BUILT_IN_STPCPY_CHK_CHKP:
       /* This would need adjustment of the lhs (subtract one),
 	 or detection that the trailing '\0' doesn't need to be
 	 written, if it will be immediately overwritten.
@@ -1268,14 +1300,33 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
       fprintf (dump_file, "Optimizing: ");
       print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
     }
-  if (gimple_call_num_args (stmt) == 2)
-    success = update_gimple_call (gsi, fn, 3, dst, src, len);
+  if (with_bounds)
+    {
+      fn = chkp_maybe_create_clone (fn)->decl;
+      if (gimple_call_num_args (stmt) == 4)
+	success = update_gimple_call (gsi, fn, 5, dst,
+				      gimple_call_arg (stmt, 1),
+				      src,
+				      gimple_call_arg (stmt, 3),
+				      len);
+      else
+	success = update_gimple_call (gsi, fn, 6, dst,
+				      gimple_call_arg (stmt, 1),
+				      src,
+				      gimple_call_arg (stmt, 3),
+				      len,
+				      gimple_call_arg (stmt, 4));
+    }
   else
-    success = update_gimple_call (gsi, fn, 4, dst, src, len,
-				  gimple_call_arg (stmt, 2));
+    if (gimple_call_num_args (stmt) == 2)
+      success = update_gimple_call (gsi, fn, 3, dst, src, len);
+    else
+      success = update_gimple_call (gsi, fn, 4, dst, src, len,
+				    gimple_call_arg (stmt, 2));
   if (success)
     {
       stmt = gsi_stmt (*gsi);
+      gimple_call_set_with_bounds (stmt, with_bounds);
       update_stmt (stmt);
       if (dump_file && (dump_flags & TDF_DETAILS) != 0)
 	{
@@ -1303,9 +1354,10 @@  handle_builtin_memcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
   tree src, dst, len, lhs, oldlen, newlen;
   gimple stmt = gsi_stmt (*gsi);
   strinfo si, dsi, olddsi;
+  bool with_bounds = gimple_call_with_bounds_p (stmt);
 
-  len = gimple_call_arg (stmt, 2);
-  src = gimple_call_arg (stmt, 1);
+  len = gimple_call_arg (stmt, with_bounds ? 4 : 2);
+  src = gimple_call_arg (stmt, with_bounds ? 2 : 1);
   dst = gimple_call_arg (stmt, 0);
   idx = get_stridx (src);
   if (idx == 0)
@@ -1412,6 +1464,8 @@  handle_builtin_memcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
     {
     case BUILT_IN_MEMCPY:
     case BUILT_IN_MEMCPY_CHK:
+    case BUILT_IN_MEMCPY_CHKP:
+    case BUILT_IN_MEMCPY_CHK_CHKP:
       /* Allow adjust_last_stmt to decrease this memcpy's size.  */
       laststmt.stmt = stmt;
       laststmt.len = dsi->length;
@@ -1421,6 +1475,8 @@  handle_builtin_memcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi)
       break;
     case BUILT_IN_MEMPCPY:
     case BUILT_IN_MEMPCPY_CHK:
+    case BUILT_IN_MEMPCPY_CHKP:
+    case BUILT_IN_MEMPCPY_CHK_CHKP:
       break;
     default:
       gcc_unreachable ();
@@ -1442,8 +1498,9 @@  handle_builtin_strcat (enum built_in_function bcode, gimple_stmt_iterator *gsi)
   gimple stmt = gsi_stmt (*gsi);
   strinfo si, dsi;
   location_t loc;
+  bool with_bounds = gimple_call_with_bounds_p (stmt);
 
-  src = gimple_call_arg (stmt, 1);
+  src = gimple_call_arg (stmt, with_bounds ? 2 : 1);
   dst = gimple_call_arg (stmt, 0);
   lhs = gimple_call_lhs (stmt);
 
@@ -1539,17 +1596,19 @@  handle_builtin_strcat (enum built_in_function bcode, gimple_stmt_iterator *gsi)
   switch (bcode)
     {
     case BUILT_IN_STRCAT:
+    case BUILT_IN_STRCAT_CHKP:
       if (srclen != NULL_TREE)
 	fn = builtin_decl_implicit (BUILT_IN_MEMCPY);
       else
 	fn = builtin_decl_implicit (BUILT_IN_STRCPY);
       break;
     case BUILT_IN_STRCAT_CHK:
+    case BUILT_IN_STRCAT_CHK_CHKP:
       if (srclen != NULL_TREE)
 	fn = builtin_decl_explicit (BUILT_IN_MEMCPY_CHK);
       else
 	fn = builtin_decl_explicit (BUILT_IN_STRCPY_CHK);
-      objsz = gimple_call_arg (stmt, 2);
+      objsz = gimple_call_arg (stmt, with_bounds ? 4 : 2);
       break;
     default:
       gcc_unreachable ();
@@ -1584,15 +1643,35 @@  handle_builtin_strcat (enum built_in_function bcode, gimple_stmt_iterator *gsi)
       fprintf (dump_file, "Optimizing: ");
       print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
     }
-  if (srclen != NULL_TREE)
-    success = update_gimple_call (gsi, fn, 3 + (objsz != NULL_TREE),
-				  dst, src, len, objsz);
+  if (with_bounds)
+    {
+      fn = chkp_maybe_create_clone (fn)->decl;
+      if (srclen != NULL_TREE)
+	success = update_gimple_call (gsi, fn, 5 + (objsz != NULL_TREE),
+				      dst,
+				      gimple_call_arg (stmt, 1),
+				      src,
+				      gimple_call_arg (stmt, 3),
+				      len, objsz);
+      else
+	success = update_gimple_call (gsi, fn, 4 + (objsz != NULL_TREE),
+				      dst,
+				      gimple_call_arg (stmt, 1),
+				      src,
+				      gimple_call_arg (stmt, 3),
+				      objsz);
+    }
   else
-    success = update_gimple_call (gsi, fn, 2 + (objsz != NULL_TREE),
-				  dst, src, objsz);
+    if (srclen != NULL_TREE)
+      success = update_gimple_call (gsi, fn, 3 + (objsz != NULL_TREE),
+				    dst, src, len, objsz);
+    else
+      success = update_gimple_call (gsi, fn, 2 + (objsz != NULL_TREE),
+				    dst, src, objsz);
   if (success)
     {
       stmt = gsi_stmt (*gsi);
+      gimple_call_set_with_bounds (stmt, with_bounds);
       update_stmt (stmt);
       if (dump_file && (dump_flags & TDF_DETAILS) != 0)
 	{
@@ -1831,25 +1910,37 @@  strlen_optimize_stmt (gimple_stmt_iterator *gsi)
 	switch (DECL_FUNCTION_CODE (callee))
 	  {
 	  case BUILT_IN_STRLEN:
+	  case BUILT_IN_STRLEN_CHKP:
 	    handle_builtin_strlen (gsi);
 	    break;
 	  case BUILT_IN_STRCHR:
+	  case BUILT_IN_STRCHR_CHKP:
 	    handle_builtin_strchr (gsi);
 	    break;
 	  case BUILT_IN_STRCPY:
 	  case BUILT_IN_STRCPY_CHK:
 	  case BUILT_IN_STPCPY:
 	  case BUILT_IN_STPCPY_CHK:
+	  case BUILT_IN_STRCPY_CHKP:
+	  case BUILT_IN_STRCPY_CHK_CHKP:
+	  case BUILT_IN_STPCPY_CHKP:
+	  case BUILT_IN_STPCPY_CHK_CHKP:
 	    handle_builtin_strcpy (DECL_FUNCTION_CODE (callee), gsi);
 	    break;
 	  case BUILT_IN_MEMCPY:
 	  case BUILT_IN_MEMCPY_CHK:
 	  case BUILT_IN_MEMPCPY:
 	  case BUILT_IN_MEMPCPY_CHK:
+	  case BUILT_IN_MEMCPY_CHKP:
+	  case BUILT_IN_MEMCPY_CHK_CHKP:
+	  case BUILT_IN_MEMPCPY_CHKP:
+	  case BUILT_IN_MEMPCPY_CHK_CHKP:
 	    handle_builtin_memcpy (DECL_FUNCTION_CODE (callee), gsi);
 	    break;
 	  case BUILT_IN_STRCAT:
 	  case BUILT_IN_STRCAT_CHK:
+	  case BUILT_IN_STRCAT_CHKP:
+	  case BUILT_IN_STRCAT_CHK_CHKP:
 	    handle_builtin_strcat (DECL_FUNCTION_CODE (callee), gsi);
 	    break;
 	  default: