Re: [PATCH v2] Add sanopt support for UBSAN_PTR.

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Oct 06, 2017 at 12:21:00PM +0200, Martin Liška wrote:
> +/* Return true when pointer PTR for a given CUR_OFFSET is already sanitized
> +   in a given sanitization context CTX.  */
> +
> +static bool
> +has_dominating_ubsan_ptr_check (sanopt_ctx *ctx, tree ptr,
> +				offset_int &cur_offset)
> +{
> +  bool positive = !wi::neg_p (cur_offset);
> +
> +  sanopt_tree_couple couple;

Perhaps s/positive/pos_p/ for the vars too?  And remove the empty line
in between bool pos_p = ...; and sanopt_tree_couple couple;

> +  couple.ptr = ptr;
> +  couple.pos_p = positive;
> +
> +  auto_vec<gimple *> &v = ctx->ptr_check_map.get_or_insert (couple);
> +  gimple *g = maybe_get_dominating_check (v);
> +  if (!g)
> +    return false;
> +
> +  /* We already have recorded a UBSAN_PTR check for this pointer.  Perhaps we
> +     can drop this one.  But only if this check doesn't specify larger offset.
> +     */
> +  tree offset = gimple_call_arg (g, 1);
> +  gcc_assert (TREE_CODE (offset) == INTEGER_CST);
> +  offset_int ooffset = wi::sext (wi::to_offset (offset), POINTER_SIZE);
> +
> +  if (positive && wi::les_p (cur_offset, ooffset))
> +    return true;
> +  else if (!positive && wi::les_p (ooffset, cur_offset))
> +    return true;

Maybe better as:
  if (pos_p)
    {
      if (wi::les_p (cur_offset, ooffset))
	return true;
    }
  else if (wi::les_p (ooffset, cur_offset))
    return true;
?

> +/* Record UBSAN_PTR check of given context CTX.  Register pointer PTR on
> +   a given OFFSET that it's handled by GIMPLE STMT.  */
> +
> +static void
> +record_ubsan_ptr_check_stmt (sanopt_ctx *ctx, gimple *stmt, tree ptr,
> +			     const offset_int &offset)
> +{
> +  bool positive = !wi::neg_p (offset);
> +
> +  sanopt_tree_couple couple;

Similarly.  Or you could in this case just write couple.pos_p = !wi::neg_p (offset);

> +  couple.ptr = ptr;
> +  couple.pos_p = positive;
> +
> +  auto_vec<gimple *> &v = ctx->ptr_check_map.get_or_insert (couple);
> +  v.safe_push (stmt);
> +}
> +

> +  tree cur_offset = gimple_call_arg (stmt, 1);

I wonder if instead of having cur_offset mean offset_int above and
tree here you couldn't use say off for the tree and cur_offset for
the offset_int.

> +  offset_int cur_offset_int = wi::sext (wi::to_offset (cur_offset),
> +					POINTER_SIZE);

I personally find it more readable to put = on a new line:
  offset_int cur_offset_int
    = wi::sext (wi::to_offset (cur_offset), POINTER_SIZE);
Though, in this case with the above name changes it could even fit:
  offset_int cur_offset = wi::sext (wi::to_offset (off), POINTER_SIZE);

> +	  offset_int expr_offset = bitpos / BITS_PER_UNIT;
> +	  offset_int total_offset = expr_offset + cur_offset_int;

And wi::neg_p (expr_offset) can be more cheaply written as bitpos < 0
in all spots below.

What I'd add here is
  if (total_offset != wi::sext (total_offset, POINTER_SIZE))
    {
      record_ubsan_ptr_check_stmt (...);
      return false;
    }
though (or branch to that stuff at the end of function).

> +
> +	  /* If BASE is a fixed size automatic variable or
> +	     global variable defined in the current TU, we don't have
> +	     to instrument anything if offset is within address
> +	     of the variable.  */
> +	  if ((VAR_P (base)
> +	       || TREE_CODE (base) == PARM_DECL
> +	       || TREE_CODE (base) == RESULT_DECL)
> +	      && DECL_SIZE_UNIT (base)
> +	      && TREE_CODE (DECL_SIZE_UNIT (base)) == INTEGER_CST
> +	      && (!is_global_var (base) || decl_binds_to_current_def_p (base)))
> +	    {
> +	      offset_int base_size = wi::to_offset (DECL_SIZE_UNIT (base));
> +	      if (!wi::neg_p (expr_offset) && wi::les_p (total_offset,
> +							 base_size))

Similar comment about line breaking, putting && on another line would
make the args to the function on one line.

> +		{
> +		  if (!wi::neg_p (total_offset)
> +		      && wi::les_p (total_offset, base_size)
> +		      && wi::fits_shwi_p (total_offset))

Why wi::fits_shwi_p?  total_offset is non-negative and smaller or equal
than base_size, so that should be enough to return true.
And if it is to make sure total_offset has not overflown, then that should
be done by the sext, otherwise it will only work for 64-bit arches.

> +		    return true;
> +		}
> +	    }
> +
> +	  /* Following expression: UBSAN_PTR (&MEM_REF[ptr + x], y) can be
> +	     handled as follows:
> +
> +	     1) sign (x) == sign (y), then check for dominating check of (x + y)
> +	     2) sign (x) != sign (y), then first check if we have a dominating
> +		check for ptr + x.  If so, then we have 2 situations:
> +		a) sign (x) == sign (x + y), here we are done, example:
> +		   UBSAN_PTR (&MEM_REF[ptr + 100], -50)
> +		b) check for dominating check of ptr + x + y.
> +	     */
> +
> +	  bool sign_cur_offset = !wi::neg_p (cur_offset_int);
> +	  bool sign_expr_offset = !wi::neg_p (expr_offset);
> +
> +	  tree base_addr = build1 (ADDR_EXPR,
> +				   build_pointer_type (TREE_TYPE (base)),
> +				   base);

Similarly
	  tree base_addr
	    = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (base)), base);
is more compact and IMHO readable.

> +
> +	  bool add = false;
> +	  if (sign_cur_offset == sign_expr_offset)
> +	    {
> +	      if (has_dominating_ubsan_ptr_check (ctx, base_addr, total_offset))
> +		return true;
> +	      else
> +		add = true;
> +	    }
> +	  else
> +	    {
> +	      if (has_dominating_ubsan_ptr_check (ctx, base_addr, expr_offset))
> +		{
> +		  bool sign_total_offset = !wi::neg_p (total_offset);
> +		  if (sign_expr_offset == sign_total_offset)
> +		    return true;
> +		  else
> +		    {
> +		      if (has_dominating_ubsan_ptr_check (ctx, base_addr,
> +							  total_offset))
> +			return true;
> +		      else
> +			add = true;
> +		    }
> +		}
> +	      else
> +		{} /* Don't record base_addr + expr_offset, it's not a guarding
> +		     check.  */

I don't like the {} much, just use /* ... */; ?

> +	    }
> +
> +	  /* Record a new dominating check for base_addr + total_offset.  */
> +	  if (add
> +	      && !operand_equal_p (base, base_addr, 0)
> +	      && wi::fits_shwi_p (total_offset))

Again, see above about fits_shwi_p.

> +	    record_ubsan_ptr_check_stmt (ctx, stmt, base_addr,
> +					 total_offset);
> +	}
> +    }
> +
> +  /* For this PTR we don't have any UBSAN_PTR stmts recorded, so there's
> +     nothing to optimize yet.  */
> +  record_ubsan_ptr_check_stmt (ctx, stmt, ptr, cur_offset_int);
> +
> +  return false;
> +}

> +
> +#define SIZE_MAX   __PTRDIFF_MAX__

Can you call it SMAX or whatever else that doesn't clash with stdint.h
SIZE_MAX that is a different thing?

> +/* { dg-final { scan-assembler-times "call\\s*__ubsan_handle_pointer_overflow" 17 } } */

\\s+ instead?  You don't want to match call__ubsan_handle_pointer_overflow ,
do you?

	Jakub