[PATCH] ipa: Careful processing ANCESTOR jump functions and NULL pointers (PR 103083)

Martin Jambor mjambor@suse.cz
Mon Nov 29 18:45:31 GMT 2021


Hi,

On Sat, Nov 27 2021, Jan Hubicka wrote:
>> Hi,
>> 
>> IPA_JF_ANCESTOR jump functions are constructed also when the formal
>> parameter of the caller is first checked whether it is NULL and left
>> as it is if it is NULL, to accommodate C++ casts to an ancestor class.
>> 
>> The jump function type was invented for devirtualization and IPA-CP
>> propagation of tree constants is also careful to apply it only to
>> existing DECLs(*) but as PR 103083 shows, the part propagating "known
>> bits" was not careful about this, which can lead to miscompilations.
>> 
>> This patch introduces a flag to the ancestor jump functions which
>> tells whether a NULL-check was elided when creating it and makes the
>> bits propagation behave accordingly.
>> 
>> (*) There still may remain problems when a DECL resides on address
>> zero (with -fno-delete-null-pointer-checks ...I hope it cannot happen
>> otherwise).  I am looking into that now but I think it will be easier
>> for everyone if I do so in a follow-up patch.
>
> I guess so.  Thinking about it bit more after our discussion yesterday
> I think it really matters where the ADDR_EXPR was created (since in
> funciton with -fno-delete-null-pointer-checks it might be equal to 0)
> and we need to somehow keep track of it.
>
> One observation is that it is unsafe to constant propagate
> ADDR_EXPR with -fno-delete-null-pointer-checks to function with
> -fdelete-null-pointer-checks, so we may just simply stop propagating
> known ADDR_EXPRs on when caller is -fno... and call is -f....

Makes sense.

>> +/* Return true if any of the known bits are non-zero.  */
>> +bool
>> +ipcp_bits_lattice::known_nonzero_p () const
>> +{
>> +  if (!constant_p ())
>> +    return false;
>> +  return !wi::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0);
> There is also ne_p

Changed.

>> @@ -2374,6 +2384,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
>>        tree operand = NULL_TREE;
>>        enum tree_code code;
>>        unsigned src_idx;
>> +      bool only_for_nonzero = false;
>>  
>>        if (jfunc->type == IPA_JF_PASS_THROUGH)
>>  	{
>> @@ -2386,7 +2397,9 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
>>  	{
>>  	  code = POINTER_PLUS_EXPR;
>>  	  src_idx = ipa_get_jf_ancestor_formal_id (jfunc);
>> -	  unsigned HOST_WIDE_INT offset = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
>> +	  unsigned HOST_WIDE_INT offset
>> +	    = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
> I still think the offset should be in bytes (and probably poly_int64).
> There is get_addr_base_and_unit_offset

I agree about bytes, not sure about poly_ints, but could be persuaded.
But probably not as a part of this patch?

>
>> +	  only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset);
>>  	  operand = build_int_cstu (size_type_node, offset);
>>  	}
>>  
>> @@ -2404,16 +2417,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
>>  	 and we store it in jump function during analysis stage.  */
>>  
>>        if (src_lats->bits_lattice.bottom_p ()
>> -	  && jfunc->bits)
>> -	return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
>> -					precision);
>> +	  || (only_for_nonzero && !src_lats->bits_lattice.known_nonzero_p ()))
>> +	{
>> +	  if (jfunc->bits)
>> +	    return dest_lattice->meet_with (jfunc->bits->value,
>> +					    jfunc->bits->mask, precision);
>> +	  else
>> +	    return dest_lattice->set_to_bottom ();
>> +	}
> I do not think you need to set to bottom here. For pointers, we
> primarily track alignment and NULL is aligned - all you need to do is to
> make sure that we do not believe that some bits are 1.

I am not sure I understand, the testcase you wrote has all bits as zeros
and still miscompiles?  It is primarily used for alignment but not only
for that.

>> @@ -3327,7 +3332,8 @@ update_jump_functions_after_inlining (struct cgraph_edge *cs,
>>  		    ipa_set_ancestor_jf (dst,
>>  					 ipa_get_jf_ancestor_offset (src),
>>  					 ipa_get_jf_ancestor_formal_id (src),
>> -					 agg_p);
>> +					 agg_p,
>> +					 ipa_get_jf_ancestor_keep_null (src));
>>  		    break;
> Somewhere here you need to consider the case where you have two accessor
> jump functions to combine together and merge the keep_null flags...

Oops, I missed that, fixed.

>
> Also with the flags, I think you want to modify ipa-cp so NULL pointers
> gets propagated acroess the ancestor jump functions.

OK, added, along with a new testcase.

> Moreover ipa-modref is using ancestor jf to update its summary.  Here I
> think with -fno-delete-null-pointer-checks it is safe to pdate also with
> the flag set, since we know the memory access is invalid otherwise.

I think it currently is, but only because for:

  int array_at_addr_zero[2];

  void __attribute__((noinline))
  bar(int *p)
  {
    clobber(p ? &p[1] : p);
  }
  /* ... */
    bar(array_at_addr_zero);

we do not produce an ancestor jump function (or any jump function
whatsoever).  In the IL it is not represented as an ADDR_EXPR of a
MEM_REF but as a POINTER_PLUS.

Anyway, I guess I'll need to amend it to avoid the set_to_bottom you
claim is not necessary but currently I have the following.

Thanks,

Martin


gcc/ChangeLog:

2021-11-29  Martin Jambor  <mjambor@suse.cz>

	PR ipa/103083
	* ipa-prop.h (ipa_ancestor_jf_data): New flag keep_null;
	(ipa_get_jf_ancestor_keep_null): New function.
	* ipa-prop.c (ipa_set_ancestor_jf): Initialize keep_null field of the
	ancestor function.
	(compute_complex_assign_jump_func): Pass false to keep_null
	parameter of ipa_set_ancestor_jf.
	(compute_complex_ancestor_jump_func): Pass true to keep_null
	parameter of ipa_set_ancestor_jf.
	(update_jump_functions_after_inlining): Carry over keep_null from the
	original ancestor jump-function or merge them.
	(ipa_write_jump_function): Stream keep_null flag.
	(ipa_read_jump_function): Likewise.
	(ipa_print_node_jump_functions_for_edge): Print the new flag.
	* ipa-cp.c (class ipcp_bits_lattice): Make various getters const.  New
	member function known_nonzero_p.
	(ipcp_bits_lattice::known_nonzero_p): New.
	(propagate_bits_across_jump_function): Only process ancestor functions
	when safe.  Remove extraneous condition handling ancestor jump
	functions.
	(propagate_aggs_across_jump_function): Take care of keep_null
	flag.
	(ipa_get_jf_ancestor_result): Propagate NULL accross keep_null
	jump functions.

gcc/testsuite/ChangeLog:

2021-11-25  Martin Jambor  <mjambor@suse.cz>

	* gcc.dg/ipa/pr103083-1.c: New test.
	* gcc.dg/ipa/pr103083-2.c: Likewise.
---
 gcc/ipa-cp.c                          | 42 +++++++++++++++++++--------
 gcc/ipa-prop.c                        | 20 +++++++++----
 gcc/ipa-prop.h                        | 13 +++++++++
 gcc/testsuite/gcc.dg/ipa/pr103083-1.c | 28 ++++++++++++++++++
 gcc/testsuite/gcc.dg/ipa/pr103083-2.c | 30 +++++++++++++++++++
 5 files changed, 116 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-1.c
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-2.c

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 5d9bb974d6b..c19fe9cf2ea 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -306,14 +306,15 @@ public:
 class ipcp_bits_lattice
 {
 public:
-  bool bottom_p () { return m_lattice_val == IPA_BITS_VARYING; }
-  bool top_p () { return m_lattice_val == IPA_BITS_UNDEFINED; }
-  bool constant_p () { return m_lattice_val == IPA_BITS_CONSTANT; }
+  bool bottom_p () const { return m_lattice_val == IPA_BITS_VARYING; }
+  bool top_p () const { return m_lattice_val == IPA_BITS_UNDEFINED; }
+  bool constant_p () const { return m_lattice_val == IPA_BITS_CONSTANT; }
   bool set_to_bottom ();
   bool set_to_constant (widest_int, widest_int);
+  bool known_nonzero_p () const;
 
-  widest_int get_value () { return m_value; }
-  widest_int get_mask () { return m_mask; }
+  widest_int get_value () const { return m_value; }
+  widest_int get_mask () const { return m_mask; }
 
   bool meet_with (ipcp_bits_lattice& other, unsigned, signop,
 		  enum tree_code, tree);
@@ -1081,6 +1082,15 @@ ipcp_bits_lattice::set_to_constant (widest_int value, widest_int mask)
   return true;
 }
 
+/* Return true if any of the known bits are non-zero.  */
+bool
+ipcp_bits_lattice::known_nonzero_p () const
+{
+  if (!constant_p ())
+    return false;
+  return wi::ne_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0);
+}
+
 /* Convert operand to value, mask form.  */
 
 void
@@ -1477,6 +1487,9 @@ ipa_get_jf_ancestor_result (struct ipa_jump_func *jfunc, tree input)
 		     fold_build2 (MEM_REF, TREE_TYPE (TREE_TYPE (input)), input,
 				  build_int_cst (ptr_type_node, byte_offset)));
     }
+  else if (ipa_get_jf_ancestor_keep_null (jfunc)
+	   && zerop (input))
+    return input;
   else
     return NULL_TREE;
 }
@@ -2373,6 +2386,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
       tree operand = NULL_TREE;
       enum tree_code code;
       unsigned src_idx;
+      bool only_for_nonzero = false;
 
       if (jfunc->type == IPA_JF_PASS_THROUGH)
 	{
@@ -2385,7 +2399,9 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
 	{
 	  code = POINTER_PLUS_EXPR;
 	  src_idx = ipa_get_jf_ancestor_formal_id (jfunc);
-	  unsigned HOST_WIDE_INT offset = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
+	  unsigned HOST_WIDE_INT offset
+	    = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
+	  only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset);
 	  operand = build_int_cstu (size_type_node, offset);
 	}
 
@@ -2403,16 +2419,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
 	 and we store it in jump function during analysis stage.  */
 
       if (src_lats->bits_lattice.bottom_p ()
-	  && jfunc->bits)
-	return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
-					precision);
+	  || (only_for_nonzero && !src_lats->bits_lattice.known_nonzero_p ()))
+	{
+	  if (jfunc->bits)
+	    return dest_lattice->meet_with (jfunc->bits->value,
+					    jfunc->bits->mask, precision);
+	  else
+	    return dest_lattice->set_to_bottom ();
+	}
       else
 	return dest_lattice->meet_with (src_lats->bits_lattice, precision, sgn,
 					code, operand);
     }
-
-  else if (jfunc->type == IPA_JF_ANCESTOR)
-    return dest_lattice->set_to_bottom ();
   else if (jfunc->bits)
     return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
 				    precision);
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index bc5643206b9..ef9e353764c 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -357,6 +357,8 @@ ipa_print_node_jump_functions_for_edge (FILE *f, struct cgraph_edge *cs)
 		   jump_func->value.ancestor.offset);
 	  if (jump_func->value.ancestor.agg_preserved)
 	    fprintf (f, ", agg_preserved");
+	  if (jump_func->value.ancestor.keep_null)
+	    fprintf (f, ", keep_null");
 	  fprintf (f, "\n");
 	}
 
@@ -601,12 +603,13 @@ ipa_set_jf_arith_pass_through (struct ipa_jump_func *jfunc, int formal_id,
 
 static void
 ipa_set_ancestor_jf (struct ipa_jump_func *jfunc, HOST_WIDE_INT offset,
-		     int formal_id, bool agg_preserved)
+		     int formal_id, bool agg_preserved, bool keep_null)
 {
   jfunc->type = IPA_JF_ANCESTOR;
   jfunc->value.ancestor.formal_id = formal_id;
   jfunc->value.ancestor.offset = offset;
   jfunc->value.ancestor.agg_preserved = agg_preserved;
+  jfunc->value.ancestor.keep_null = keep_null;
 }
 
 /* Get IPA BB information about the given BB.  FBI is the context of analyzis
@@ -1438,7 +1441,8 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
   index = ipa_get_param_decl_index (info, SSA_NAME_VAR (ssa));
   if (index >= 0 && param_type && POINTER_TYPE_P (param_type))
     ipa_set_ancestor_jf (jfunc, offset,  index,
-			 parm_ref_data_pass_through_p (fbi, index, call, ssa));
+			 parm_ref_data_pass_through_p (fbi, index, call, ssa),
+			 false);
 }
 
 /* Extract the base, offset and MEM_REF expression from a statement ASSIGN if
@@ -1564,7 +1568,8 @@ compute_complex_ancestor_jump_func (struct ipa_func_body_info *fbi,
     }
 
   ipa_set_ancestor_jf (jfunc, offset, index,
-		       parm_ref_data_pass_through_p (fbi, index, call, parm));
+		       parm_ref_data_pass_through_p (fbi, index, call, parm),
+		       true);
 }
 
 /* Inspect the given TYPE and return true iff it has the same structure (the
@@ -3250,6 +3255,7 @@ update_jump_functions_after_inlining (struct cgraph_edge *cs,
 	      dst->value.ancestor.offset += src->value.ancestor.offset;
 	      dst->value.ancestor.agg_preserved &=
 		src->value.ancestor.agg_preserved;
+	      dst->value.ancestor.keep_null |= src->value.ancestor.keep_null;
 	    }
 	  else
 	    ipa_set_jf_unknown (dst);
@@ -3327,7 +3333,8 @@ update_jump_functions_after_inlining (struct cgraph_edge *cs,
 		    ipa_set_ancestor_jf (dst,
 					 ipa_get_jf_ancestor_offset (src),
 					 ipa_get_jf_ancestor_formal_id (src),
-					 agg_p);
+					 agg_p,
+					 ipa_get_jf_ancestor_keep_null (src));
 		    break;
 		  }
 		default:
@@ -4758,6 +4765,7 @@ ipa_write_jump_function (struct output_block *ob,
       streamer_write_uhwi (ob, jump_func->value.ancestor.formal_id);
       bp = bitpack_create (ob->main_stream);
       bp_pack_value (&bp, jump_func->value.ancestor.agg_preserved, 1);
+      bp_pack_value (&bp, jump_func->value.ancestor.keep_null, 1);
       streamer_write_bitpack (&bp);
       break;
     default:
@@ -4883,7 +4891,9 @@ ipa_read_jump_function (class lto_input_block *ib,
 	int formal_id = streamer_read_uhwi (ib);
 	struct bitpack_d bp = streamer_read_bitpack (ib);
 	bool agg_preserved = bp_unpack_value (&bp, 1);
-	ipa_set_ancestor_jf (jump_func, offset, formal_id, agg_preserved);
+	bool keep_null = bp_unpack_value (&bp, 1);
+	ipa_set_ancestor_jf (jump_func, offset, formal_id, agg_preserved,
+			     keep_null);
 	break;
       }
     default:
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index ba49843a510..4e243bcfe19 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -143,6 +143,8 @@ struct GTY(()) ipa_ancestor_jf_data
   int formal_id;
   /* Flag with the same meaning like agg_preserve in ipa_pass_through_data.  */
   unsigned agg_preserved : 1;
+  /* When set, the operation should not have any effect on NULL pointers.  */
+  unsigned keep_null : 1;
 };
 
 /* A jump function for an aggregate part at a given offset, which describes how
@@ -438,6 +440,17 @@ ipa_get_jf_ancestor_type_preserved (struct ipa_jump_func *jfunc)
   return jfunc->value.ancestor.agg_preserved;
 }
 
+/* Return if jfunc represents an operation whether we first check the formal
+   parameter for non-NULLness unless it does not matter because the offset is
+   zero anyway.  */
+
+static inline bool
+ipa_get_jf_ancestor_keep_null (struct ipa_jump_func *jfunc)
+{
+  gcc_checking_assert (jfunc->type == IPA_JF_ANCESTOR);
+  return jfunc->value.ancestor.keep_null;
+}
+
 /* Class for allocating a bundle of various potentially known properties about
    actual arguments of a particular call on stack for the usual case and on
    heap only if there are unusually many arguments.  The data is deallocated
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103083-1.c b/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
new file mode 100644
index 00000000000..e2fbb45d3cc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -Wno-pointer-to-int-cast" } */
+
+struct b {int b;};
+struct a {int a; struct b b;};
+
+long i;
+
+__attribute__ ((noinline))
+static void test2 (struct b *b)
+{
+  if (((int)b)&4)
+    __builtin_abort ();
+}
+
+__attribute__ ((noinline))
+static void
+test (struct a *a)
+{
+  test2(a? &a->b : 0);
+}
+
+int
+main()
+{
+  test(0);
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103083-2.c b/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
new file mode 100644
index 00000000000..ae1b905af81
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-ipa-bit-cp -fdump-tree-optimized" } */
+
+struct b {int b;};
+struct a {int a; struct b b;};
+
+void remove_any_mention (void);
+
+__attribute__ ((noinline))
+static void test2 (struct b *b)
+{
+  if (b)
+    remove_any_mention ();
+}
+
+__attribute__ ((noinline))
+static void
+test (struct a *a)
+{
+  test2(a? &a->b : 0);
+}
+
+int
+foo()
+{
+  test(0);
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "remove_any_mention" "optimized" } } */
-- 
2.33.1







More information about the Gcc-patches mailing list