Re: [PATCH] Add sanopt support for UBSAN_PTR.

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Oct 04, 2017 at 11:05:23AM +0200, Martin Liška wrote:
> Following patch adds support for optimizing out unnecessary UBSAN_PTR checks.
> It handles separately positive and negative offsets, zero offset is ignored.
> Apart from that, we utilize get_inner_reference for local and global variables,
> that also helps to reduce some.

Thanks for working on it.

> @@ -148,6 +149,61 @@ struct sanopt_tree_triplet_hash : typed_noop_remove <sanopt_tree_triplet>
>    }
>  };
>  
> +/* Tree couple for ptr_check_map.  */
> +struct sanopt_tree_couple
> +{
> +  tree ptr;
> +  bool positive;

Maybe pos_p instead, so that it isn't that long?

> +  static inline bool
> +  equal (const sanopt_tree_couple &ref1, const sanopt_tree_couple &ref2)
> +  {
> +    return operand_equal_p (ref1.ptr, ref2.ptr, 0)
> +	   && ref1.positive == ref2.positive;

Or this would need to use ()s around the return expression for emacs
users.

> +/* Return true when pointer PTR for a given OFFSET is already sanitized
> +   in a given sanitization context CTX.  */
> +
> +static bool
> +has_dominating_ubsan_ptr_check (sanopt_ctx *ctx, tree ptr, tree cur_offset)

Any reason why you pass cur_offset as a tree rather than wide_int & or
offset_int &?  If the caller makes sure it is sign extended from
pointer/sizetype's precision, then:

> +{
> +  int r = get_range_pos_neg (cur_offset);

here you can just wi::neg_p or similar.

> +  /* 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);
> +
> +  if (positive && tree_int_cst_le (cur_offset, offset))
> +    return true;
> +  else if (!positive && tree_int_cst_le (offset, cur_offset))
> +    return true;

And the comparisons here in wide_int would be cheaper too.

> +
> +static void
> +record_ubsan_ptr_check_stmt (sanopt_ctx *ctx, gimple *stmt, tree ptr,
> +			     tree offset)
> +{
> +  int r = get_range_pos_neg (offset);

See above.

> +  gcc_assert (r != 3);
> +  bool positive = r == 1;
> +
> +  sanopt_tree_couple couple;
> +  couple.ptr = ptr;
> +  couple.positive = positive;
> +
> +  auto_vec<gimple *> &v = ctx->ptr_check_map.get_or_insert (couple);
> +  v.safe_push (stmt);
> +}
> +
> +/* Optimize away redundant UBSAN_PTR calls.  */
> +
> +static bool
> +maybe_optimize_ubsan_ptr_ifn (sanopt_ctx *ctx, gimple *stmt)
> +{
> +  gcc_assert (gimple_call_num_args (stmt) == 2);
> +  tree ptr = gimple_call_arg (stmt, 0);
> +  tree cur_offset = gimple_call_arg (stmt, 1);
> +
> +  if (TREE_CODE (cur_offset) != INTEGER_CST)
> +    return false;
> +
> +  if (integer_zerop (cur_offset))
> +    return true;
> +
> +  if (has_dominating_ubsan_ptr_check (ctx, ptr, cur_offset))
> +    return true;
> +
> +  tree base = ptr;
> +  if (TREE_CODE (base) == ADDR_EXPR)
> +    {
> +      base = TREE_OPERAND (base, 0);
> +
> +      HOST_WIDE_INT bitsize, bitpos;
> +      machine_mode mode;
> +      int volatilep = 0, reversep, unsignedp = 0;
> +      tree offset;
> +      base = get_inner_reference (base, &bitsize, &bitpos, &offset, &mode,
> +				  &unsignedp, &reversep, &volatilep);
> +      if (DECL_P (base))
> +	{
> +	  gcc_assert (!DECL_REGISTER (base));
> +	  /* If BASE is a fixed size automatic variable or
> +	     global variable defined in the current TU and bitpos
> +	     fits, don't instrument anything.  */
> +	  if ((offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)

Do you really need to handle offset != NULL_TREE?
If the bit offset is representable in shwi, then it will just be
in bitpos and offset will be NULL.

> +	      && (VAR_P (base)
> +		  || TREE_CODE (base) == PARM_DECL
> +		  || TREE_CODE (base) == RESULT_DECL)
> +	      && DECL_SIZE (base)
> +	      && TREE_CODE (DECL_SIZE (base)) == INTEGER_CST

Why are you testing here DECL_SIZE when you actually then use
DECL_SIZE_UNIT?

> +	      && (!is_global_var (base) || decl_binds_to_current_def_p (base)))
> +	    {
> +	      HOST_WIDE_INT bytepos = bitpos / BITS_PER_UNIT;
> +	      offset_int total_offset = bytepos;
> +	      if (offset != NULL_TREE)
> +		total_offset += wi::sext (wi::to_offset (offset), POINTER_SIZE);
> +	      total_offset += wi::sext (wi::to_offset (cur_offset),
> +					POINTER_SIZE);

Why are you sign extending it each time?  Can't it be sign extended after
all the additions?
On the other side, is it safe to add the (bytepos + offset) part with
the cur_offset part unconditionally?
If the former is "positive" and cur_offset is "negative", they cancel each
other and we IMHO shouldn't try to optimize it away.  bytepos could be some
very large integer, outside of bounds of the var, and cur_offset some
negative constant, where both var + bytepos and (var + bytepos) + cur_offset
overflow.  Or bytepos could be negative, outside of the bounds of the
variable.  I think you need to check separately that bytepos is >= 0 and
<= DECL_SIZE_UNIT and that bytepos + cur_offset is within that range too.

> +	      /* New total offset can fit in pointer type.  */
> +	      if (wi::fits_shwi_p (total_offset))

Why do you need to convert a wide_int to signed HOST_WIDE_INT and back?
Or do you want to check for some overflows above?

> +		{
> +		  tree toffset = build_int_cst (TREE_TYPE (cur_offset),
> +						total_offset.to_shwi ());
> +		  tree ptr2 = build1 (ADDR_EXPR,
> +				      build_pointer_type (TREE_TYPE (base)),
> +				      base);

Why do you need to create these trees here when you actually don't use them
(or shouldn't need)

> +
> +		  /* If total offset is non-negative and smaller or equal
> +		     to size of declaration, then no check is needed.  */
> +		  if (get_range_pos_neg (toffset) == 1
> +		      && tree_int_cst_le (toffset, DECL_SIZE_UNIT (base)))
> +		    return true;

above.

> +		  /* Maybe we already have a dominating check for the BASE.  */
> +		  else if (has_dominating_ubsan_ptr_check (ctx, ptr2, toffset))
> +		    return true;

And, isn't this something that really doesn't depend on whether base is a
decl (so move it outside of the if (DECL_P ()) if), but rather just on
whether bytepos and cur_offset have the same "sign"?
If ptr is say &MEM_REF[whatever + 1024] and 2048 (both positive), then
it will not overflow if we've checked that UBSAN(whatever, 3072) (or more).
Similarly for both negative values.  For &MEM_REF[whatever + 3072] and -2048,
previous UBSAN(whatever, 3072) check would be sufficient too, or for
&MEM_REF[whatever + 1024] and -3072 a previous UBSAN(whatever, -2048) check
(or more negative).  But in any case, you need to compare the signs and if
they aren't the same, reason what is appropriate and what is not.
>        else
> -	gsi_next (&gsi);
> +	{
> +	  if (dump_file && (dump_flags & TDF_DETAILS))
> +	    {
> +	      fprintf (dump_file, "Leaving: ");
> +	      print_gimple_stmt (dump_file, stmt, 0, dump_flags);
> +	    }

Why this?  Printing all other stmts?  That doesn't make sense to me.

	Jakub