Re: Add support to trace comparison instructions and switch statements

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Sep 04, 2017 at 09:36:40PM +0800, 吴潍浠(此彼) wrote:
> gcc/ChangeLog:                                                     
> 
> 2017-09-04  Wish Wu  <wishwu007@gmail.com>                         
> 
>         * asan.c (initialize_sanitizer_builtins):                  
>         * builtin-types.def (BT_FN_VOID_UINT8_UINT8):              
>         (BT_FN_VOID_UINT16_UINT16):                                
>         (BT_FN_VOID_UINT32_UINT32):                                
>         (BT_FN_VOID_FLOAT_FLOAT):                                  
>         (BT_FN_VOID_DOUBLE_DOUBLE):                                
>         (BT_FN_VOID_UINT64_PTR):                                   
>         * common.opt:                                              
>         * flag-types.h (enum sanitize_coverage_code):              
>         * opts.c (COVERAGE_SANITIZER_OPT):                         
>         (get_closest_sanitizer_option):                            
>         (parse_sanitizer_options):                                 
>         (common_handle_option):                                    
>         * sancov.c (instrument_cond):                              
>         (instrument_switch):                                       
>         (sancov_pass):                                             
>         * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_CMP1):       
>         (BUILT_IN_SANITIZER_COV_TRACE_CMP2):                       
>         (BUILT_IN_SANITIZER_COV_TRACE_CMP4):                       
>         (BUILT_IN_SANITIZER_COV_TRACE_CMP8):                       
>         (BUILT_IN_SANITIZER_COV_TRACE_CMPF):                       
>         (BUILT_IN_SANITIZER_COV_TRACE_CMPD):                       
>         (BUILT_IN_SANITIZER_COV_TRACE_SWITCH):                     

mklog just generates a template, you need to fill in the details
on what has been changed or added or removed.  See other ChangeLog
entries etc. to see what is expected.

> For code :
> void bar (void);
> void
> foo (int x)
> {
>   if (x == 21 || x == 64 || x == 98 || x == 135)
>     bar ();
> }
> GIMPLE IL on x86_64:
>   1 
>   2 ;; Function foo (foo, funcdef_no=0, decl_uid=2161, cgraph_uid=0, symbol_order=0)
>   3 
>   4 foo (int x)
>   5 {

...

That is with -O0 though?  With -O2 you'll see that it changes.
IMNSHO you really want to also handle the GIMPLE_ASSIGN with tcc_comparison
class rhs_code.  Shouldn't be that hard to handle that within
instrument_cond, just the way how you extract lhs and rhs from the insn will
differ based on if it is a GIMPLE_COND or GIMPLE_ASSIGN (and in that case
also for tcc_comparison rhs_code or for COND_EXPR with tcc_comparison first
operand).
And I really think we should change the 2 LOGICAL_OP_NON_SHORT_CIRCUIT
uses in fold-const.c and one in tree-ssa-ifcombine.c with
  LOGICAL_OP_NON_SHORT_CIRCUIT
  && !flag_sanitize_coverage
with a comment that for sanitize coverage we want to avoid this optimization
because it negatively affects it.

@@ -1611,38 +1631,51 @@ parse_sanitizer_options (const char *p, location_t
 	}
 
       /* Check to see if the string matches an option class name.  */
-      for (i = 0; sanitizer_opts[i].name != NULL; ++i)
-	if (len == sanitizer_opts[i].len
-	    && memcmp (p, sanitizer_opts[i].name, len) == 0)
+      for (i = 0; opts[i].name != NULL; ++i)
+	if (len == opts[i].len
+	    && memcmp (p, opts[i].name, len) == 0)
 	  {
-	    /* Handle both -fsanitize and -fno-sanitize cases.  */
-	    if (value && sanitizer_opts[i].flag == ~0U)
+	    if (code == OPT_fsanitize_coverage_)
 	      {
-		if (code == OPT_fsanitize_)
-		  {
-		    if (complain)
-		      error_at (loc, "%<-fsanitize=all%> option is not valid");
-		  }
+		if (value)
+		  flags |= opts[i].flag;
 		else
-		  flags |= ~(SANITIZE_THREAD | SANITIZE_LEAK
-			     | SANITIZE_UNREACHABLE | SANITIZE_RETURN);
+		  flags &= ~opts[i].flag;
+		found = true;
+		break;
 	      }
-	    else if (value)
+	    else
 	      {
-		/* Do not enable -fsanitize-recover=unreachable and
-		   -fsanitize-recover=return if -fsanitize-recover=undefined
-		   is selected.  */
-		if (code == OPT_fsanitize_recover_
-		    && sanitizer_opts[i].flag == SANITIZE_UNDEFINED)
-		  flags |= (SANITIZE_UNDEFINED
-			    & ~(SANITIZE_UNREACHABLE | SANITIZE_RETURN));
+		/* Handle both -fsanitize and -fno-sanitize cases.  */
+		if (value && opts[i].flag == ~0U)
+		  {
+		    if (code == OPT_fsanitize_)
+		      {
+			if (complain)
+			  error_at (loc,
+				    "%<-fsanitize=all%> option is not valid");
+		      }
+		    else
+		      flags |= ~(SANITIZE_THREAD | SANITIZE_LEAK
+				 | SANITIZE_UNREACHABLE | SANITIZE_RETURN);
+		  }
+		else if (value)
+		  {
+		    /* Do not enable -fsanitize-recover=unreachable and
+		       -fsanitize-recover=return if -fsanitize-recover=undefined
+		       is selected.  */
+		    if (code == OPT_fsanitize_recover_
+			&& opts[i].flag == SANITIZE_UNDEFINED)
+		      flags |= (SANITIZE_UNDEFINED
+				& ~(SANITIZE_UNREACHABLE | SANITIZE_RETURN));
+		    else
+		      flags |= opts[i].flag;
+		  }
 		else
-		  flags |= sanitizer_opts[i].flag;
+		  flags &= ~opts[i].flag;
+		found = true;
+		break;
 	      }
-	    else
-	      flags &= ~sanitizer_opts[i].flag;
-	    found = true;
-	    break;
 	  }

I don't see a need for this hunk.  For code == OPT_fsanitize_coverage_
(where you know that there is no entry with ~0U flag value and also
know that code is not OPT_fsanitize_recover_) I think it will
just DTRT without any changes.
 
 namespace {

There should be a function comment before the function here, explaining
what the function is for.
 
+static void
+instrument_cond (gimple_stmt_iterator *gsi, gimple *stmt)
+{
+  tree lhs = gimple_cond_lhs (stmt);
+  tree rhs = gimple_cond_rhs (stmt);
+  tree lhs_type = TREE_TYPE (lhs);
+  tree rhs_type = TREE_TYPE (rhs);
+
+  HOST_WIDE_INT size_in_bytes = MAX (int_size_in_bytes (lhs_type),
+				     int_size_in_bytes (rhs_type));
+  if (size_in_bytes == -1)
+    return;

As I said, GIMPLE_COND operands should have the same (or compatible)
type, so there is no need to use rhs_type at all, nor test it.
And there is no need to test size_in_bytes before the if, just do
it right before the switch in there (and no need to special case -1,
because it is like any other default: handled by return;).

+  else if (SCALAR_FLOAT_TYPE_P (lhs_type) && SCALAR_FLOAT_TYPE_P (rhs_type))

Again, no need to test both.

+    {
+      if (TYPE_MODE (lhs_type) == TYPE_MODE (double_type_node)
+	  || TYPE_MODE (rhs_type) == TYPE_MODE (double_type_node))

Or here.

+	{
+      	  fncode = BUILT_IN_SANITIZER_COV_TRACE_CMPD;
+	  to_type = double_type_node;
+	}
+      else if (TYPE_MODE (lhs_type) == TYPE_MODE (float_type_node)
+	       || TYPE_MODE (rhs_type) == TYPE_MODE (float_type_node))

Or here.  Just use type instead of lhs_type.

+	{
+      	  fncode = BUILT_IN_SANITIZER_COV_TRACE_CMPF;
+	  to_type = float_type_node;
+	}
+
+    }
+  if (to_type != NULL_TREE)
+    {
+      gimple_seq seq = NULL;
+
+      gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs));
+      tree clhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));
+
+      gimple_seq_add_stmt (&seq, build_type_cast (to_type, rhs));
+      tree crhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));

If the var already has the right type, that will just create waste
that further opts will need to clean up.  Better:
  if (!useless_type_conversion_p (to_type, lhs_type))	// or s/lhs_// if you do the above
    {
      gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs));
      lhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));

      gimple_seq_add_stmt (&seq, build_type_cast (to_type, rhs));
      rhs = gimple_assign_rhs (gimple_seq_last_stmt (seq));
    }
or perhaps even 
  if (!useless_type_conversion_p (to_type, type))
    {
      if (TREE_CODE (lhs) == INTEGER_CST)
	lhs = fold_convert (to_type, lhs);
      else
	{
	  gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs));
	  lhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));
	}
...
    }

+  tree index = gimple_switch_index (switch_stmt);
+
+  HOST_WIDE_INT size_in_bytes = int_size_in_bytes (TREE_TYPE (index));
+  if (size_in_bytes == -1)
+    return;

Well, you want to punt not just when it is -1, but also when it is
> 8.

+
+  unsigned i, n = gimple_switch_num_labels (switch_stmt), num = 0;
+  for (i = 0; i < n; ++i)

I think gimple_switch_label (switch_stmt, 0) must be the
default label and thus have no low/high case, so you should
use for (i = 1; i < n; ++i).

+  for (i = 0; i < n; ++i)

Ditto.

	Jakub