diff mbox

[RFC] warn on dead function calls in ipa-pure-const [1/4]

Message ID CAAgBjMkg_iMo1RB3P+Ey7Q9AitApXQbTsat8DMR+0=FwynFgZg@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni July 25, 2016, 11:35 p.m. UTC 
Hi,
The attached patch emits warnings for functions found to be pure or
const by the ipa-pure-const
pass. It does not warn for functions with unused return values that
have been declared
as pure or const by the user since this is already handled in C and C++ FE's.
I have split it into parts to individually address fallouts observed
during bootstrap+test.
I still have to add more test-cases. Apart from that does the patch look OK  ?

Thanks,
Prathamesh

Comments

Prathamesh Kulkarni July 31, 2016, 5:20 p.m. UTC | #1 
On 31 July 2016 at 22:01, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Tue, 26 Jul 2016, Prathamesh Kulkarni wrote:

>>

>> > +   warning_at (gimple_location (g), OPT_Wunused_value,

>> > +               "Call from %s to %s has no effect",

>> > +               e->caller->name (), e->callee->name ());

>>

>> Diagnostics should not start with capital letters.  Function names in

>> diagnostics should be quoted, so %qs.  Also, what form is this name in?

>> If it's the internal UTF-8 form, you need to use identifier_to_locale on

>> it to produce something suitable for a diagnostic.  And for C++ you need

>> to make sure the name is a proper pretty name (including classes /

>> namespaces / type etc.) as produced by the decl_printable_name langhook,

>> before passing it to identifier_to_locale.

>

> I think you just want to pass e->caller->decl (with corresponding % formatter)

> rather than name()

Hi,
Thanks for the reviews. However after discussing with Richard,
we decided to drop this warning for now, because it can lead to
potentially false positives
like for the following case in stor-layout.c:

   /* Stop if the mode requires too much alignment.  */
      if (GET_MODE_ALIGNMENT (m_mode) > m_align
          && SLOW_UNALIGNED_ACCESS (m_mode, m_align))
        break;

On x86_64, SLOW_UNALIGNED_ACCESS is #defined to 0,
so the condition essentially becomes:

if (get_mode_alignment (m_mode) > m_align && 0)
  break;

and the patch warns for the above dead call.
However the call might not always be dead, since it depends
on conditionally defined macro SLOW_UNALIGNED_ACCESS,
which other targets may perhaps define as a run-time value.

Unfortunately I don't have any good ideas to address this issue.
We could restrict the warning for cases when call is not a
sub-expression, however I suppose we would need some help from
FE's to determine if call_expr is outermost expression ?
I thought of adding another flag to tree_exp for this purpose,
but that doesn't look like a good idea.
I would be grateful for suggestions for addressing this issue.

Thanks,
Prathamesh
>

> Honza
diff mbox

Patch

diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index a9570e4..3b8e774 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -226,6 +226,34 @@  warn_function_noreturn (tree decl)
 			   true, warned_about, "noreturn");
 }
 
+/* Emit diagnostic if the callers don't use return value.
+   Only to be called for pure/const function */
+
+static void
+warn_function_unused_ret (struct cgraph_node *node)
+{
+  tree decl = node->decl;
+  int flags = flags_from_decl_or_type (decl);
+
+  if (flags & ECF_NORETURN)
+    return;
+
+  if (!(flags & (ECF_CONST | ECF_PURE))
+        || (flags & ECF_LOOPING_CONST_OR_PURE))
+    return;
+
+  for (cgraph_edge *e = node->callers; e; e = e->next_caller)
+    {
+      gcall *g = e->call_stmt;
+      if (g
+	  && !VOID_TYPE_P (gimple_call_return_type (g))
+	  && gimple_call_lhs (g) == NULL)
+	warning_at (gimple_location (g), OPT_Wunused_value,
+		    "Call from %s to %s has no effect",
+		    e->caller->name (), e->callee->name ());
+    }
+}
+
 /* Return true if we have a function state for NODE.  */
 
 static inline bool
@@ -1475,6 +1503,8 @@  propagate_pure_const (void)
 	  /* Inline clones share declaration with their offline copies;
 	     do not modify their declarations since the offline copy may
 	     be different.  */
+	  bool warn_unused_ret = false;
+
 	  if (!w->global.inlined_to)
 	    switch (this_state)
 	      {
@@ -1482,6 +1512,7 @@  propagate_pure_const (void)
 		if (!TREE_READONLY (w->decl))
 		  {
 		    warn_function_const (w->decl, !this_looping);
+		    warn_unused_ret = true;
 		    if (dump_file)
 		      fprintf (dump_file, "Function found to be %sconst: %s\n",
 			       this_looping ? "looping " : "",
@@ -1496,6 +1527,8 @@  propagate_pure_const (void)
 							      NULL, true);
 		if (w->set_const_flag (true, this_looping))
 		  {
+		    if (warn_unused_ret)
+		      warn_function_unused_ret (w);
 		    if (dump_file)
 		      fprintf (dump_file,
 			       "Declaration updated to be %sconst: %s\n",
@@ -1509,6 +1542,7 @@  propagate_pure_const (void)
 		if (!DECL_PURE_P (w->decl))
 		  {
 		    warn_function_pure (w->decl, !this_looping);
+		    warn_unused_ret = true;
 		    if (dump_file)
 		      fprintf (dump_file, "Function found to be %spure: %s\n",
 			       this_looping ? "looping " : "",
@@ -1521,6 +1555,8 @@  propagate_pure_const (void)
 							      NULL, true);
 		if (w->set_pure_flag (true, this_looping))
 		  {
+		    if (warn_unused_ret)
+		      warn_function_unused_ret (w);
 		    if (dump_file)
 		      fprintf (dump_file,
 			       "Declaration updated to be %spure: %s\n",
@@ -1808,11 +1844,14 @@  pass_local_pure_const::execute (function *fun)
       changed = true;
     }
 
+  bool warn_unused_ret = false;
+
   switch (l->pure_const_state)
     {
     case IPA_CONST:
       if (!TREE_READONLY (current_function_decl))
 	{
+	  warn_unused_ret = true;
 	  warn_function_const (current_function_decl, !l->looping);
 	  if (dump_file)
 	    fprintf (dump_file, "Function found to be %sconst: %s\n",
@@ -1828,6 +1867,8 @@  pass_local_pure_const::execute (function *fun)
 	}
       if (!skip && node->set_const_flag (true, l->looping))
 	{
+	  if (warn_unused_ret)
+	    warn_function_unused_ret (cgraph_node::get_create (current_function_decl));
 	  if (dump_file)
 	    fprintf (dump_file, "Declaration updated to be %sconst: %s\n",
 		     l->looping ? "looping " : "",
@@ -1840,6 +1881,7 @@  pass_local_pure_const::execute (function *fun)
       if (!DECL_PURE_P (current_function_decl))
 	{
 	  warn_function_pure (current_function_decl, !l->looping);
+	  warn_unused_ret = true;
 	  if (dump_file)
 	    fprintf (dump_file, "Function found to be %spure: %s\n",
 		     l->looping ? "looping " : "",
@@ -1854,6 +1896,8 @@  pass_local_pure_const::execute (function *fun)
 	}
       if (!skip && node->set_pure_flag (true, l->looping))
 	{
+	  if (warn_unused_ret)
+	    warn_function_unused_ret (cgraph_node::get_create (current_function_decl));
 	  if (dump_file)
 	    fprintf (dump_file, "Declaration updated to be %spure: %s\n",
 		     l->looping ? "looping " : "",
diff --git a/gcc/testsuite/gcc.dg/Wunused-value-4.c b/gcc/testsuite/gcc.dg/Wunused-value-4.c
new file mode 100644
index 0000000..1f5463c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wunused-value-4.c
@@ -0,0 +1,23 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wunused-value" } */
+
+__attribute__((noinline, no_icf))
+int f()
+{
+  return 1;
+}
+
+__attribute__((noinline, no_icf, const))
+int g()
+{
+  return 1;
+}   
+
+int foo (void)
+{
+  f ();  /* { dg-warning "Call from foo to f has no effect" } */
+  int k = f ();  /* { dg-bogus "Call from foo to f has no effect" } */
+  g (); /* { dg-warning "statement with no effect" } */
+  return k; 
+}
+