PR85817

Message ID CAAgBjMmAmZHjNKJkkzjJU_QmGH-FQ5i2qawN8PYuMj7-n82=9A@mail.gmail.com
State New
Headers show
Series
  • PR85817
Related show

Commit Message

Prathamesh Kulkarni May 18, 2018, 8:49 a.m.
Hi,
In r260250, the condition

if (integer_zerop (retval))
  continue;

was added before checking retval was of pointer type which caused
functions having return type apart from void *, to be marked as
malloc. The attached patch gets rid of the above check since we do not
wish to mark function returning NULL as malloc.
Also, it adds a check to return false if all args to phi are 0,
although I am not sure if this'd actually trigger in practice since
constant propagation should have folded the phi into constant 0
already.

Bootstrap+test in progress on x86_64-linux-gnu and aarch64-linux-gnu.
OK to commit if passes ?

Thanks,
Prathamesh
2018-05-18  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR middle-end/85817
	* ipa-pure-const.c (malloc_candidate_p): Remove the check integer_zerop
	for retval and return false if all args to phi are zero.	

testsuite/
	* gcc.dg/tree-ssa/pr83648.c: Change scan-tree-dump to
	scan-tree-dump-not for h.

Comments

Richard Biener May 18, 2018, 8:51 a.m. | #1
On Fri, 18 May 2018, Prathamesh Kulkarni wrote:

> Hi,

> In r260250, the condition

> 

> if (integer_zerop (retval))

>   continue;

> 

> was added before checking retval was of pointer type which caused

> functions having return type apart from void *, to be marked as

> malloc. The attached patch gets rid of the above check since we do not

> wish to mark function returning NULL as malloc.

> Also, it adds a check to return false if all args to phi are 0,

> although I am not sure if this'd actually trigger in practice since

> constant propagation should have folded the phi into constant 0

> already.

> 

> Bootstrap+test in progress on x86_64-linux-gnu and aarch64-linux-gnu.

> OK to commit if passes ?


OK.

Thanks,
Richard.
Jeff Law May 18, 2018, 1:50 p.m. | #2
On 05/18/2018 02:49 AM, Prathamesh Kulkarni wrote:
> Hi,

> In r260250, the condition

> 

> if (integer_zerop (retval))

>   continue;

> 

> was added before checking retval was of pointer type which caused

> functions having return type apart from void *, to be marked as

> malloc. The attached patch gets rid of the above check since we do not

> wish to mark function returning NULL as malloc.

> Also, it adds a check to return false if all args to phi are 0,

> although I am not sure if this'd actually trigger in practice since

> constant propagation should have folded the phi into constant 0

> already.

> 

> Bootstrap+test in progress on x86_64-linux-gnu and aarch64-linux-gnu.

> OK to commit if passes ?

FWIW, I'm currently digging into a bootstrap failure on riscv64 that is
triggered by the original change to allow functions returning NULL to
potentially be malloc candidates.  I'll give things a spin with this
patch as well.

jeff
Jeff Law May 18, 2018, 2:57 p.m. | #3
On 05/18/2018 02:49 AM, Prathamesh Kulkarni wrote:
> Hi,

> In r260250, the condition

> 

> if (integer_zerop (retval))

>   continue;

> 

> was added before checking retval was of pointer type which caused

> functions having return type apart from void *, to be marked as

> malloc. The attached patch gets rid of the above check since we do not

> wish to mark function returning NULL as malloc.

> Also, it adds a check to return false if all args to phi are 0,

> although I am not sure if this'd actually trigger in practice since

> constant propagation should have folded the phi into constant 0

> already.

> 

> Bootstrap+test in progress on x86_64-linux-gnu and aarch64-linux-gnu.

> OK to commit if passes ?

So I just checked and it appears this will resolve the riscv64 issue.

The additional marking was ultimately causing the alias analysis code to
narrow the aliasing scope of a local variable that had its address taken
-- to the point where it was no longer considered escaping.

With the variable no longer escaping, calls were no longer may-defs of
the variable and Wuninitialized could perform a more precise analysis
and determined the variable was used potentially uninitialized.

In reality the uninitialized use is guarded in such a way that it won't
be used uninitialized, but without some kind of late inlining the
analysis wouldn't be able to see the use is properly guarded.

With the more restrictive marking, the variable is considered escaping
again and thus is may-def'd by random calls and we don't trigger the
-Wuninitialized warning anymore.


Thanks,
Jeff

Patch

diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 567b615fb60..528ea6695ac 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -940,9 +940,6 @@  malloc_candidate_p (function *fun, bool ipa)
       if (!retval)
 	DUMP_AND_RETURN("No return value.")
 
-      if (integer_zerop (retval))
-	continue;
-
       if (TREE_CODE (retval) != SSA_NAME
 	  || TREE_CODE (TREE_TYPE (retval)) != POINTER_TYPE)
 	DUMP_AND_RETURN("Return value is not SSA_NAME or not a pointer type.")
@@ -972,37 +969,44 @@  malloc_candidate_p (function *fun, bool ipa)
 	}
 
       else if (gphi *phi = dyn_cast<gphi *> (def))
-	for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
-	  {
-	    tree arg = gimple_phi_arg_def (phi, i);
-	    if (integer_zerop (arg))
-	      continue;
+	{
+	  bool all_args_zero = true;
+	  for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
+	    {
+	      tree arg = gimple_phi_arg_def (phi, i);
+	      if (integer_zerop (arg))
+		continue;
+
+	      all_args_zero = false;
+	      if (TREE_CODE (arg) != SSA_NAME)
+		DUMP_AND_RETURN ("phi arg is not SSA_NAME.");
+	      if (!check_retval_uses (arg, phi))
+		DUMP_AND_RETURN ("phi arg has uses outside phi"
+				 " and comparisons against 0.")
+
+	      gimple *arg_def = SSA_NAME_DEF_STMT (arg);
+	      gcall *call_stmt = dyn_cast<gcall *> (arg_def);
+	      if (!call_stmt)
+		return false;
+	      tree callee_decl = gimple_call_fndecl (call_stmt);
+	      if (!callee_decl)
+		return false;
+	      if (!ipa && !DECL_IS_MALLOC (callee_decl))
+		DUMP_AND_RETURN("callee_decl does not have malloc attribute"
+				" for non-ipa mode.")
+
+	      cgraph_edge *cs = node->get_edge (call_stmt);
+	      if (cs)
+		{
+		  ipa_call_summary *es = ipa_call_summaries->get (cs);
+		  gcc_assert (es);
+		  es->is_return_callee_uncaptured = true;
+		}
+	    }
 
-	    if (TREE_CODE (arg) != SSA_NAME)
-	      DUMP_AND_RETURN ("phi arg is not SSA_NAME.");
-	    if (!check_retval_uses (arg, phi))
-	      DUMP_AND_RETURN ("phi arg has uses outside phi"
-			       " and comparisons against 0.")
-
-	    gimple *arg_def = SSA_NAME_DEF_STMT (arg);
-	    gcall *call_stmt = dyn_cast<gcall *> (arg_def);
-	    if (!call_stmt)
-	      return false;
-	    tree callee_decl = gimple_call_fndecl (call_stmt);
-	    if (!callee_decl)
-	      return false;
-	    if (!ipa && !DECL_IS_MALLOC (callee_decl))
-	      DUMP_AND_RETURN("callee_decl does not have malloc attribute for"
-			      " non-ipa mode.")
-
-	    cgraph_edge *cs = node->get_edge (call_stmt);
-	    if (cs)
-	      {
-		ipa_call_summary *es = ipa_call_summaries->get (cs);
-		gcc_assert (es);
-		es->is_return_callee_uncaptured = true;
-	      }
-	  }
+	  if (all_args_zero)
+	    DUMP_AND_RETURN ("Return value is a phi with all args equal to 0.");
+	}
 
       else
 	DUMP_AND_RETURN("def_stmt of return value is not a call or phi-stmt.")
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr83648.c b/gcc/testsuite/gcc.dg/tree-ssa/pr83648.c
index febfd7d9319..884faf81167 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr83648.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr83648.c
@@ -12,4 +12,4 @@  void *h()
 }
 
 /* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */
-/* { dg-final { scan-tree-dump "Function found to be malloc: h" "local-pure-const1" } } */
+/* { dg-final { scan-tree-dump-not "Function found to be malloc: h" "local-pure-const1" } } */