[PATCH] [RFC] unreachable returns

[PATCH] [RFC] unreachable returns

[Richard Biener]

We have quite a number of "default" returns that cannot be reached.
One is particularly interesting since it says (see patch below):

     default:
       gcc_unreachable ();
     }
   /* We can get here with --disable-checking.  */
   return false;

which suggests that _maybe_ the intention was to have the
gcc_unreachable () which expands to __builtin_unreachable ()
with --disable-checking and thus a fallthru to "somewhere"
be catched with a "sane" default return value rather than
falling through to the next function or so.  BUT - that
isn't what actually happens since the 'return false' is
unreachable after CFG construction and will be elided.

In fact the IL after CFG construction is exactly the same
with and without the spurious return.

Now, I wonder if we should, instead of expanding
gcc_unreachable to __builtin_unreachable () with
--disable-checking, expand it to __builtin_trap ()
(or remove the --disable-checking variant completely,
always retaining assert level checking but maybe make
it cheaper in size by using __builtin_trap () or abort ())

Thoughts?

That said, I do have a set of changes removing such spurious
returns.

2021-11-25  Richard Biener  <rguenther@suse.de>

gcc/c/
	* c-typeck.c (c_tree_equal): Remove unreachable return.
---
 gcc/c/c-typeck.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index b71358e1821..7524304f2bd 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -15984,8 +15984,6 @@ c_tree_equal (tree t1, tree t2)
     default:
       gcc_unreachable ();
     }
-  /* We can get here with --disable-checking.  */
-  return false;
 }
 
 /* Returns true when the function declaration FNDECL is implicit,
-- 
2.31.1

Re: [PATCH] [RFC] unreachable returns

[Jan Hubicka]

> We have quite a number of "default" returns that cannot be reached.
> One is particularly interesting since it says (see patch below):
> 
>      default:
>        gcc_unreachable ();
>      }
>    /* We can get here with --disable-checking.  */
>    return false;
> 
> which suggests that _maybe_ the intention was to have the
> gcc_unreachable () which expands to __builtin_unreachable ()
> with --disable-checking and thus a fallthru to "somewhere"
> be catched with a "sane" default return value rather than
> falling through to the next function or so.  BUT - that
> isn't what actually happens since the 'return false' is
> unreachable after CFG construction and will be elided.

I think this is just remat of times we did not have
__builtin_unreachable.  I like the idea of removing the redundant code.

Honza

Re: [PATCH] [RFC] unreachable returns

[Jeff Law]

On 11/25/2021 6:23 AM, Richard Biener via Gcc-patches wrote:
> We have quite a number of "default" returns that cannot be reached.
> One is particularly interesting since it says (see patch below):
>
>       default:
>         gcc_unreachable ();
>       }
>     /* We can get here with --disable-checking.  */
>     return false;
>
> which suggests that _maybe_ the intention was to have the
> gcc_unreachable () which expands to __builtin_unreachable ()
> with --disable-checking and thus a fallthru to "somewhere"
> be catched with a "sane" default return value rather than
> falling through to the next function or so.  BUT - that
> isn't what actually happens since the 'return false' is
> unreachable after CFG construction and will be elided.
>
> In fact the IL after CFG construction is exactly the same
> with and without the spurious return.
>
> Now, I wonder if we should, instead of expanding
> gcc_unreachable to __builtin_unreachable () with
> --disable-checking, expand it to __builtin_trap ()
> (or remove the --disable-checking variant completely,
> always retaining assert level checking but maybe make
> it cheaper in size by using __builtin_trap () or abort ())
>
> Thoughts?
>
> That said, I do have a set of changes removing such spurious
> returns.
>
> 2021-11-25  Richard Biener  <rguenther@suse.de>
>
> gcc/c/
> 	* c-typeck.c (c_tree_equal): Remove unreachable return.
I'd bet if you dig into the history you'll find that the return was 
added first to make enable-checking happy, then later we added the 
gcc_unreachable().

I think expanding to __builtin_trap is highly preferable to 
__builtin_unreachable and it's probably the lowest overhead option. I 
can also live with removing the -disable-checking variant and instead 
using something that always halts execution.  Once we're always halting 
execution on that path I have no objection to removing the extraneous 
return.

jeff