[Patch] Fix bogus 'function does return' warning

[Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

Hi,

the GIMPLE EH patch introduced a regression for the attached testcase: a 
bogus 'function does return' is now issued.

The problem is that the EH pass turns

      D.2142 = &"p.adb"[0];
      .gnat_rcheck_20 (D.2142, 7);
      <D.2137>:
      D.2143 = &"p.adb"[0];
      .gnat_rcheck_04 (D.2143, 9);
      goto <D.2151>;
    }
  finally
    {
      p.error (); [static-chain: &FRAME.8]
    }
  <D.2151>:
  return;

into

  D.2142 = &"p.adb"[0];
  .gnat_rcheck_20 (D.2142, 7);
  <D.2137>:
  D.2143 = &"p.adb"[0];
  .gnat_rcheck_04 (D.2143, 9);
  finally_tmp.10 = 0;
  goto <D.2156>;
  <D.2156>:
  p.error (); [static-chain: &FRAME.8]
  switch (finally_tmp.10) <default: <D.2159>, default: <D.2159>, case 1: 
<D.2158>>
  <D.2159>:
  goto <D.2151>;
  <D.2151>:
  return;
  <D.2157>:
  finally_tmp.10 = 1;
  goto <D.2156>;
  <D.2158>:
  resx 1

thus masking the fact that D.2159 and D.2151 are unreachable.  So the CFG ends 
up being

<bb 21>:
  D.2143 = &"p.adb"[0];
  .gnat_rcheck_04 (D.2143, 9);

<L17>:
  return;

<L19>:
  finally_tmp.10 = 1;
  p.error (); [static-chain: &FRAME.8]
  switch (finally_tmp.10) <default: <L17>, case 1: <L20>>

<L20>:
  resx 1

and the EXIT block has a predecessor, hence the warning.


The proposed fix is to remove unreachable statements after noreturn calls 
during the "lower" pass.  Tested on i586-suse-linux, OK for mainline?


2009-10-15  Eric Botcazou  <ebotcazou@adacore.com>

	* gimple-low.c (lower_stmt) <GIMPLE_CALL>: If the call is noreturn,
	remove subsequent regular statements in the sequence.


2009-10-15  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/noreturn1.ad[sb]: New test.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 701 bytes --]

Index: gimple-low.c
===================================================================
--- gimple-low.c	(revision 152797)
+++ gimple-low.c	(working copy)
@@ -387,6 +387,18 @@ lower_stmt (gimple_stmt_iterator *gsi, s
 	    lower_builtin_setjmp (gsi);
 	    return;
 	  }
+
+	/* After a noreturn call, remove the subsequent statements in the
+	   sequence up to the next OMP statement or label.  */
+	if (decl && (flags_from_decl_or_type (decl) & ECF_NORETURN))
+	  {
+	      gsi_next (gsi);
+	      while (!gsi_end_p (*gsi)
+		     && !is_gimple_omp (gsi_stmt (*gsi))
+		     && gimple_code (gsi_stmt (*gsi)) != GIMPLE_LABEL)
+		gsi_remove (gsi, false);
+	      return;
+	  }
       }
       break;
 

[-- Attachment #3: noreturn1.adb --]
[-- Type: text/x-adasrc, Size: 327 bytes --]

-- { dg-compile }

package body Noreturn1 is

   procedure Error (E : in Exception_Occurrence) is
      Occurrence_Message : constant String := Exception_Message (E);
   begin
      if Occurrence_Message = "$" then
         raise Program_Error;
      else
         raise Constraint_Error;
      end if;
   end;

end Noreturn1;

[-- Attachment #4: noreturn1.ads --]
[-- Type: text/x-adasrc, Size: 159 bytes --]

with Ada.Exceptions; use Ada.Exceptions;

package Noreturn1 is

   procedure Error (E : in Exception_Occurrence);
   pragma No_Return (Error);

end Noreturn1;

Re: [Patch] Fix bogus 'function does return' warning

[Richard Guenther]

On Thu, Oct 15, 2009 at 9:48 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> the GIMPLE EH patch introduced a regression for the attached testcase: a
> bogus 'function does return' is now issued.
>
> The problem is that the EH pass turns
>
>      D.2142 = &"p.adb"[0];
>      .gnat_rcheck_20 (D.2142, 7);
>      <D.2137>:
>      D.2143 = &"p.adb"[0];
>      .gnat_rcheck_04 (D.2143, 9);
>      goto <D.2151>;
>    }
>  finally
>    {
>      p.error (); [static-chain: &FRAME.8]
>    }
>  <D.2151>:
>  return;
>
> into
>
>  D.2142 = &"p.adb"[0];
>  .gnat_rcheck_20 (D.2142, 7);
>  <D.2137>:
>  D.2143 = &"p.adb"[0];
>  .gnat_rcheck_04 (D.2143, 9);
>  finally_tmp.10 = 0;
>  goto <D.2156>;
>  <D.2156>:
>  p.error (); [static-chain: &FRAME.8]
>  switch (finally_tmp.10) <default: <D.2159>, default: <D.2159>, case 1:
> <D.2158>>
>  <D.2159>:
>  goto <D.2151>;
>  <D.2151>:
>  return;
>  <D.2157>:
>  finally_tmp.10 = 1;
>  goto <D.2156>;
>  <D.2158>:
>  resx 1
>
> thus masking the fact that D.2159 and D.2151 are unreachable.  So the CFG ends
> up being
>
> <bb 21>:
>  D.2143 = &"p.adb"[0];
>  .gnat_rcheck_04 (D.2143, 9);
>
> <L17>:
>  return;
>
> <L19>:
>  finally_tmp.10 = 1;
>  p.error (); [static-chain: &FRAME.8]
>  switch (finally_tmp.10) <default: <L17>, case 1: <L20>>
>
> <L20>:
>  resx 1
>
> and the EXIT block has a predecessor, hence the warning.
>
>
> The proposed fix is to remove unreachable statements after noreturn calls
> during the "lower" pass.  Tested on i586-suse-linux, OK for mainline?

Huh.  I wonder why the unreachable blocks are not removed
after CFG build?  After all noreturn calls end BBs.

Am I missing sth?

Richard.

>
> 2009-10-15  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * gimple-low.c (lower_stmt) <GIMPLE_CALL>: If the call is noreturn,
>        remove subsequent regular statements in the sequence.
>
>
> 2009-10-15  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * gnat.dg/noreturn1.ad[sb]: New test.
>
>
> --
> Eric Botcazou
>

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

> Huh.  I wonder why the unreachable blocks are not removed
> after CFG build?  After all noreturn calls end BBs.

It's -O0 so the compiler doesn't see that <L17> is unreachable:

<L17>:
  return;

<L19>:
  finally_tmp.10 = 1;
  p.error (); [static-chain: &FRAME.8]
  switch (finally_tmp.10) <default: <L17>, case 1: <L20>>

-- 
Eric Botcazou

Re: [Patch] Fix bogus 'function does return' warning

[Richard Guenther]

On Fri, Oct 16, 2009 at 11:55 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Huh.  I wonder why the unreachable blocks are not removed
>> after CFG build?  After all noreturn calls end BBs.
>
> It's -O0 so the compiler doesn't see that <L17> is unreachable:
>
> <L17>:
>  return;
>
> <L19>:
>  finally_tmp.10 = 1;
>  p.error (); [static-chain: &FRAME.8]
>  switch (finally_tmp.10) <default: <L17>, case 1: <L20>>

Ok, so we get lucky at -O1 because decide_copy_try_finally
decides to copy the finally region.  Thus the real reason for
the warning is the return in

        if ((integer) R6b == (integer) R5b &&
VIEW_CONVERT_EXPR<character[1:1]>(*R3b.P_ARRAY) == "$")
          {
            .gnat_rcheck_20 ("noreturn1.adb", 9);
          }
        else
          {
            .gnat_rcheck_04 ("noreturn1.adb", 11);
          }
        return;

or that .gnat_rcheck_* can appearantly throw (or instead that
EH lowering adds EH edges to the finally block even though
that the functions cannot return).

So, can they throw or is EH lowering at fault here?

Richard.

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

> Thus the real reason for the warning is the return in
>
>         if ((integer) R6b == (integer) R5b &&
> VIEW_CONVERT_EXPR<character[1:1]>(*R3b.P_ARRAY) == "$")
>           {
>             .gnat_rcheck_20 ("noreturn1.adb", 9);
>           }
>         else
>           {
>             .gnat_rcheck_04 ("noreturn1.adb", 11);
>           }
>         return;
>
> or that .gnat_rcheck_* can appearantly throw (or instead that
> EH lowering adds EH edges to the finally block even though
> that the functions cannot return).
>
> So, can they throw or is EH lowering at fault here?

Yes, they can throw, but that's not a normal return.  And EH lowering must add 
the edges because "lower" has introduced goto <D.2151>:

      D.2142 = &"p.adb"[0];
      .gnat_rcheck_20 (D.2142, 7);
      <D.2137>:
      D.2143 = &"p.adb"[0];
      .gnat_rcheck_04 (D.2143, 9);
      goto <D.2151>;
    }
  finally
    {
      p.error (); [static-chain: &FRAME.8]
    }
  <D.2151>:
  return;

I think the problem is this return -> goto transformation in "lower".

-- 
Eric Botcazou

Re: [Patch] Fix bogus 'function does return' warning

[Richard Guenther]

On Fri, Oct 16, 2009 at 12:44 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Thus the real reason for the warning is the return in
>>
>>         if ((integer) R6b == (integer) R5b &&
>> VIEW_CONVERT_EXPR<character[1:1]>(*R3b.P_ARRAY) == "$")
>>           {
>>             .gnat_rcheck_20 ("noreturn1.adb", 9);
>>           }
>>         else
>>           {
>>             .gnat_rcheck_04 ("noreturn1.adb", 11);
>>           }
>>         return;
>>
>> or that .gnat_rcheck_* can appearantly throw (or instead that
>> EH lowering adds EH edges to the finally block even though
>> that the functions cannot return).
>>
>> So, can they throw or is EH lowering at fault here?
>
> Yes, they can throw, but that's not a normal return.  And EH lowering must add
> the edges because "lower" has introduced goto <D.2151>:
>
>      D.2142 = &"p.adb"[0];
>      .gnat_rcheck_20 (D.2142, 7);
>      <D.2137>:
>      D.2143 = &"p.adb"[0];
>      .gnat_rcheck_04 (D.2143, 9);

(X)

>      goto <D.2151>;
>    }
>  finally
>    {
>      p.error (); [static-chain: &FRAME.8]
>    }
>  <D.2151>:
>  return;
>
> I think the problem is this return -> goto transformation in "lower".

But EH lowering must still go through finally even if the return
remains, no?  So it indeed is the problem that nothing in EH
lowering sees that we cannot arrive at (X) but we only have
EH edges reaching the finally.

So the question is how does EH lowering see that for

      .gnat_rcheck_04 (D.2175, 11);
    }
  finally
    {
      noreturn1.error (); [static-chain: &FRAME.8]
    }

we can't enter the finally block through a regular edge and
whether we can extend that to also grok

      .gnat_rcheck_04 (D.2175, 11);
      goto <D.2183>;
    }
  finally
    {
      noreturn1.error (); [static-chain: &FRAME.8]
    }

the same...

Richard.

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

> But EH lowering must still go through finally even if the return
> remains, no? 

Yes, you're right.

> So the question is how does EH lowering see that for
>
>       .gnat_rcheck_04 (D.2175, 11);
>     }
>   finally
>     {
>       noreturn1.error (); [static-chain: &FRAME.8]
>     }
>
> we can't enter the finally block through a regular edge and
> whether we can extend that to also grok
>
>       .gnat_rcheck_04 (D.2175, 11);
>       goto <D.2183>;
>     }
>   finally
>     {
>       noreturn1.error (); [static-chain: &FRAME.8]
>     }
>
> the same...

In the former case there is no fallthru and no goto for the try block, in the 
latter there is no fallthru either but a goto, which is detected by the pass.
So ndests starts with 1 instead of 0 in lower_try_finally.

I'm not sure how to extend the pass, this would mean detecting unreachable 
gotos and returns.  Eliminating them in "lower" seems easier, maybe I can 
restrict my patch and eliminate only them.

-- 
Eric Botcazou

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

> I'm not sure how to extend the pass, this would mean detecting unreachable
> gotos and returns.  Eliminating them in "lower" seems easier, maybe I can
> restrict my patch and eliminate only them.

What about this?


        * gimple-low.c (lower_stmt) <GIMPLE_CALL>: If the call is noreturn,
        remove a subsequent GOTO or RETURN statement.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 789 bytes --]

Index: gimple-low.c
===================================================================
--- gimple-low.c	(revision 152797)
+++ gimple-low.c	(working copy)
@@ -387,6 +387,19 @@ lower_stmt (gimple_stmt_iterator *gsi, s
 	    lower_builtin_setjmp (gsi);
 	    return;
 	  }
+
+	/* After a noreturn call, remove a subsequent GOTO or RETURN that
+	   might have been mechanically added.  This will prevent the EH
+	   lowering pass to add useless edges and complicate the CFG.  */
+	if (decl && (flags_from_decl_or_type (decl) & ECF_NORETURN))
+	  {
+	      gsi_next (gsi);
+	      if (!gsi_end_p (*gsi)
+		  && (gimple_code (gsi_stmt (*gsi)) == GIMPLE_GOTO
+		      || gimple_code (gsi_stmt (*gsi)) == GIMPLE_RETURN))
+		gsi_remove (gsi, false);
+	      return;
+	  }
       }
       break;
 

Re: [Patch] Fix bogus 'function does return' warning

[Richard Guenther]

On Sat, Oct 17, 2009 at 10:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I'm not sure how to extend the pass, this would mean detecting unreachable
>> gotos and returns.  Eliminating them in "lower" seems easier, maybe I can
>> restrict my patch and eliminate only them.
>
> What about this?
>
>
>        * gimple-low.c (lower_stmt) <GIMPLE_CALL>: If the call is noreturn,
>        remove a subsequent GOTO or RETURN statement.

Well, I'm only debating if this is the correct place to do this kind
of stuff at all.
What I'd certainly would accept is at the place we transform the RETURN to
the GOTO to the single RETURN drop the RETURN in case it was after a
noreturn call - that would still look like lowering.

Thanks,
Richard.

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

> What I'd certainly would accept is at the place we transform the RETURN to
> the GOTO to the single RETURN drop the RETURN in case it was after a
> noreturn call - that would still look like lowering.

OK, but this means adding a new machinery to the "lower" pass to track and 
record whether a RETURN is after a noreturn call and before a label.  And 
what do you suggest for GOTOs?  They need to be removed as well.

-- 
Eric Botcazou

Re: [Patch] Fix bogus 'function does return' warning

[Richard Guenther]

On Sat, Oct 17, 2009 at 1:34 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> What I'd certainly would accept is at the place we transform the RETURN to
>> the GOTO to the single RETURN drop the RETURN in case it was after a
>> noreturn call - that would still look like lowering.
>
> OK, but this means adding a new machinery to the "lower" pass to track and
> record whether a RETURN is after a noreturn call and before a label.  And

Correct.

> what do you suggest for GOTOs?  They need to be removed as well.

Well, but that's not lowering.  I think it's perfectly valid to warn if the user
wrote for example

 noreturn();
 return;

in a noreturn function.  Likewise if he writes

  noreturn();
  goto x;

x:;
}

As EH lowering complicates matters in your case (otherwise cfgcleanup
would fixup all issues) I think a proper place to address this is EH lowering
(thus I cced rth).

Richard.

> --
> Eric Botcazou
>

Re: [Patch] Fix bogus 'function does return' warning

[Richard Henderson]

On 10/17/2009 04:46 AM, Richard Guenther wrote:
> As EH lowering complicates matters in your case (otherwise cfgcleanup
> would fixup all issues) I think a proper place to address this is EH lowering
> (thus I cced rth).

What would you suggest that EH lowering do?  I suppose we could always 
copy the eh path for finally.  It would eliminate the problematic case here.

Really, I'd have preferred this be cleaned up in the 
remove_useless_stmts pass, which Matz recently removed.  But given a 
choice between complicating pass_lower_cf and pass_lower_eh, frankly I'd 
prefer pass_lower_cf -- eh lowering is already complicated enough.


r~

Re: [Patch] Fix bogus 'function does return' warning

[Richard Guenther]

On Sat, Oct 17, 2009 at 6:51 PM, Richard Henderson <rth@redhat.com> wrote:
> On 10/17/2009 04:46 AM, Richard Guenther wrote:
>>
>> As EH lowering complicates matters in your case (otherwise cfgcleanup
>> would fixup all issues) I think a proper place to address this is EH
>> lowering
>> (thus I cced rth).
>
> What would you suggest that EH lowering do?  I suppose we could always copy
> the eh path for finally.  It would eliminate the problematic case here.
>
> Really, I'd have preferred this be cleaned up in the remove_useless_stmts
> pass, which Matz recently removed.  But given a choice between complicating
> pass_lower_cf and pass_lower_eh, frankly I'd prefer pass_lower_cf -- eh
> lowering is already complicated enough.

Heh.  Well then - the original patch is ok.

Thanks,
Richard.

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

> Heh.  Well then - the original patch is ok.

Thanks.  However, I simplified the original testcase far too much so the fix 
isn't general enough, sorry about that.  The new testcase (attached) involves 
nested GIMPLE_TRY_FINALLYs and GIMPLE_BINDs.

So, in the end, we really need to track things.  I think it's better to think 
in terms of fallthruness since there is already a lot of code in gimple-low.c 
that does this.  The patch fixes a typo in the process, whereby GIMPLE_SWITCH 
is deemed may_fallthru by the code of gimple_stmt_may_fallthru whereas the 
comment just above explains why it is not.  It also attempts to remove only 
unreachable return statements, that's sufficient and probably safer.

Tested on x86_64-suse-linux, OK for mainline?


2009-10-18  Eric Botcazou  <ebotcazou@adacore.com>

        * gimple-low.c (struct lower_data): Add cannot_fallthru field.
	(lower_stmt) <GIMPLE_BIND>: Add comment.
	<GIMPLE_COND, GIMPLE_GOTO, GIMPLE_SWITCH>: Set cannot_fallthru to true
	and return.
	<GIMPLE_RETURN>: Remove the statement if cannot_fallthru is set.
	Otherwise lower it and set cannot_fallthru to true.
	<GIMPLE_TRY>: Update cannot_fallthru for GIMPLE_TRY_FINALLY.  Set it
	to false otherwise.  Return.
	<GIMPLE_CATCH, GIMPLE_EH_FILTER>; Set cannot_fallthru to false.
	<GIMPLE_CALL>: Set cannot_fallthru to false for BUILT_IN_SETJMP and
	to true for a noreturn call.  Do not remove statements.
	<GIMPLE_OMP_PARALLEL, GIMPLE_OMP_TASK>: Set cannot_fallthru to false.
	Set cannot_fallthru to false on function exit.
	(gimple_stmt_may_fallthru) <GIMPLE_SWITCH>: Really return false.
	<GIMPLE_ASSIGN>: Remove.


2009-10-18  Eric Botcazou  <ebotcazou@adacore.com>

        * gnat.dg/noreturn2.ad[sb]: New test.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 5123 bytes --]

Index: gimple-low.c
===================================================================
--- gimple-low.c	(revision 152959)
+++ gimple-low.c	(working copy)
@@ -76,6 +76,9 @@ struct lower_data
      of the function.  */
   VEC(return_statements_t,heap) *return_statements;
 
+  /* True if the current statement cannot fall through.  */
+  bool cannot_fallthru;
+
   /* True if the function calls __builtin_setjmp.  */
   bool calls_builtin_setjmp;
 };
@@ -317,7 +320,12 @@ lower_omp_directive (gimple_stmt_iterato
 }
 
 
-/* Lower statement GSI.  DATA is passed through the recursion.  */
+/* Lower statement GSI.  DATA is passed through the recursion.  We try
+   to track the fallthruness of statements and get rid of unreachable
+   return statements in order to prevent the next EH lowering pass from
+   adding useless edges that can cause bogus warnings to be issued later.
+   This guess need not be 100% accurate; simply be conservative and reset
+   cannot_fallthru to false if we don't know.  */
 
 static void
 lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data)
@@ -330,36 +338,60 @@ lower_stmt (gimple_stmt_iterator *gsi, s
     {
     case GIMPLE_BIND:
       lower_gimple_bind (gsi, data);
+      /* Propagate fallthruness.  */
       return;
 
     case GIMPLE_COND:
-      /* The gimplifier has already lowered this into gotos.  */
-      break;
+    case GIMPLE_GOTO:
+    case GIMPLE_SWITCH:
+      data->cannot_fallthru = true;
+      gsi_next (gsi);
+      return;
 
     case GIMPLE_RETURN:
-      lower_gimple_return (gsi, data);
+      if (data->cannot_fallthru)
+	{
+	  gsi_remove (gsi, false);
+	  /* Propagate fallthruness.  */
+	}
+      else
+	{
+	  lower_gimple_return (gsi, data);
+	  data->cannot_fallthru = true;
+	}
       return;
 
     case GIMPLE_TRY:
-      lower_sequence (gimple_try_eval (stmt), data);
-      lower_sequence (gimple_try_cleanup (stmt), data);
-      break;
+      {
+	bool try_cannot_fallthru;
+	lower_sequence (gimple_try_eval (stmt), data);
+	try_cannot_fallthru = data->cannot_fallthru;
+	data->cannot_fallthru = false;
+	lower_sequence (gimple_try_cleanup (stmt), data);
+	/* See gimple_stmt_may_fallthru for the rationale.  */
+	if (gimple_try_kind (stmt) == GIMPLE_TRY_FINALLY)
+	  data->cannot_fallthru |= try_cannot_fallthru;
+	else
+	  data->cannot_fallthru = false;
+	gsi_next (gsi);
+	return;
+      }
 
     case GIMPLE_CATCH:
+      data->cannot_fallthru = false;
       lower_sequence (gimple_catch_handler (stmt), data);
       break;
 
     case GIMPLE_EH_FILTER:
+      data->cannot_fallthru = false;
       lower_sequence (gimple_eh_filter_failure (stmt), data);
       break;
 
     case GIMPLE_NOP:
     case GIMPLE_ASM:
     case GIMPLE_ASSIGN:
-    case GIMPLE_GOTO:
     case GIMPLE_PREDICT:
     case GIMPLE_LABEL:
-    case GIMPLE_SWITCH:
     case GIMPLE_EH_MUST_NOT_THROW:
     case GIMPLE_OMP_FOR:
     case GIMPLE_OMP_SECTIONS:
@@ -383,21 +415,16 @@ lower_stmt (gimple_stmt_iterator *gsi, s
 	    && DECL_BUILT_IN_CLASS (decl) == BUILT_IN_NORMAL
 	    && DECL_FUNCTION_CODE (decl) == BUILT_IN_SETJMP)
 	  {
-	    data->calls_builtin_setjmp = true;
 	    lower_builtin_setjmp (gsi);
+	    data->cannot_fallthru = false;
+	    data->calls_builtin_setjmp = true;
 	    return;
 	  }
 
-	/* After a noreturn call, remove a subsequent GOTO or RETURN that might
-	   have been mechanically added; this will prevent the EH lowering pass
-	   from adding useless edges and thus complicating the initial CFG.  */
 	if (decl && (flags_from_decl_or_type (decl) & ECF_NORETURN))
 	  {
+	    data->cannot_fallthru = true;
 	    gsi_next (gsi);
-	    if (!gsi_end_p (*gsi)
-		&& (gimple_code (gsi_stmt (*gsi)) == GIMPLE_GOTO
-		    || gimple_code (gsi_stmt (*gsi)) == GIMPLE_RETURN))
-	      gsi_remove (gsi, false);
 	    return;
 	  }
       }
@@ -405,13 +432,16 @@ lower_stmt (gimple_stmt_iterator *gsi, s
 
     case GIMPLE_OMP_PARALLEL:
     case GIMPLE_OMP_TASK:
+      data->cannot_fallthru = false;
       lower_omp_directive (gsi, data);
+      data->cannot_fallthru = false;
       return;
 
     default:
       gcc_unreachable ();
     }
 
+  data->cannot_fallthru = false;
   gsi_next (gsi);
 }
 
@@ -660,9 +690,9 @@ gimple_stmt_may_fallthru (gimple stmt)
       return false;
 
     case GIMPLE_SWITCH:
-      /* Switch has already been lowered and represents a
-	 branch to a selected label and hence can not fall through.  */
-      return true;
+      /* Switch has already been lowered and represents a branch
+	 to a selected label and hence can't fall through.  */
+      return false;
 
     case GIMPLE_COND:
       /* GIMPLE_COND's are already lowered into a two-way branch.  They
@@ -688,13 +718,10 @@ gimple_stmt_may_fallthru (gimple stmt)
       return (gimple_seq_may_fallthru (gimple_try_eval (stmt))
 	      && gimple_seq_may_fallthru (gimple_try_cleanup (stmt)));
 
-    case GIMPLE_ASSIGN:
-      return true;
-
     case GIMPLE_CALL:
       /* Functions that do not return do not fall through.  */
       return (gimple_call_flags (stmt) & ECF_NORETURN) == 0;
-    
+
     default:
       return true;
     }

[-- Attachment #3: noreturn2.ads --]
[-- Type: text/x-adasrc, Size: 166 bytes --]

with Ada.Exceptions; use Ada.Exceptions;

package Noreturn2 is

   procedure Raise_From (X : Exception_Occurrence);
   pragma No_Return (Raise_From);

end Noreturn2;

[-- Attachment #4: noreturn2.adb --]
[-- Type: text/x-adasrc, Size: 599 bytes --]

-- { dg-do compile }

package body Noreturn2 is

   procedure Raise_Exception_No_Defer (Message : String);
   pragma No_Return (Raise_Exception_No_Defer);

   procedure Raise_From (X : Exception_Occurrence) is
      Occurrence_Message : constant String := Exception_Message (X);
   begin
      if Occurrence_Message = "$" then
         Raise_Exception_No_Defer (Occurrence_Message);
      else
         Raise_Exception_No_Defer ("::" & Occurrence_Message);
      end if;
   end;

   procedure Raise_Exception_No_Defer (Message : String) is
   begin
     raise Program_Error;
   end;

end Noreturn2;

Re: [Patch] Fix bogus 'function does return' warning

[Richard Henderson]

On 10/18/2009 08:03 AM, Eric Botcazou wrote:
> ... It also attempts to remove only
> unreachable return statements, that's sufficient and probably safer.

If you want to prevent EH from adding unnecessary edges, you also need 
to remove gotos; removing just the returns is not sufficient.

>      case GIMPLE_TRY:
> -      lower_sequence (gimple_try_eval (stmt), data);
> -      lower_sequence (gimple_try_cleanup (stmt), data);
> -      break;
> +      {
> +	bool try_cannot_fallthru;
> +	lower_sequence (gimple_try_eval (stmt), data);
> +	try_cannot_fallthru = data->cannot_fallthru;
> +	data->cannot_fallthru = false;
> +	lower_sequence (gimple_try_cleanup (stmt), data);
> +	/* See gimple_stmt_may_fallthru for the rationale.  */
> +	if (gimple_try_kind (stmt) == GIMPLE_TRY_FINALLY)
> +	  data->cannot_fallthru |= try_cannot_fallthru;
> +	else
> +	  data->cannot_fallthru = false;

This last line is not correct.  It should read

   data->cannot_fallthru = try_cannot_fallthru;


r~

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

> If you want to prevent EH from adding unnecessary edges, you also need
> to remove gotos; removing just the returns is not sufficient.

That seems sufficient to fix the regressions related to the warning though.
Richard G. suggested in an earlier message to handle only returns.

> > +	if (gimple_try_kind (stmt) == GIMPLE_TRY_FINALLY)
> > +	  data->cannot_fallthru |= try_cannot_fallthru;
> > +	else
> > +	  data->cannot_fallthru = false;
>
> This last line is not correct.  It should read
>
>    data->cannot_fallthru = try_cannot_fallthru;

Setting cannot_fallthru to false cannot be not correct, see the head comment.  
Are you positive that this will work if one of the GIMPLE_CATCHes falls thru?

-- 
Eric Botcazou

Re: [Patch] Fix bogus 'function does return' warning

[Richard Henderson]

On 10/18/2009 09:52 AM, Eric Botcazou wrote:
>>     data->cannot_fallthru = try_cannot_fallthru;
>
> Setting cannot_fallthru to false cannot be not correct, see the head comment.
> Are you positive that this will work if one of the GIMPLE_CATCHes falls thru?

Oops, no.  You're right.



r~

Re: [Patch] Fix bogus 'function does return' warning

[Eric Botcazou]

> Oops, no.  You're right.

OK, thanks.   So is the patch a good enough middle ground between your 
(Richard H. and Richard G.) respective views to be installed?

-- 
Eric Botcazou

Re: [Patch] Fix bogus 'function does return' warning

[Richard Henderson]

On 10/19/2009 10:27 AM, Eric Botcazou wrote:
>> Oops, no.  You're right.
>
> OK, thanks.   So is the patch a good enough middle ground between your
> (Richard H. and Richard G.) respective views to be installed?
>
Yep.


r~