Message ID | 13oq9o43-6q30-orp1-281q-4q2859sn7r49@fhfr.qr |
---|---|
State | New |
Headers | show |
Series | [RFC] unreachable returns | expand |
> 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
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
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,