Remove unreachable gcc_unreachable () at the end of functions
Commit Message
It seems to be a style to place gcc_unreachable () after a
switch that handles all cases with every case returning.
Those are unreachable (well, yes!), so they will be elided
at CFG construction time and the middle-end will place
another __builtin_unreachable "after" them to note the
path doesn't lead to a return when the function is not declared
void.
So IMHO those explicit gcc_unreachable () serve no purpose,
if they could be replaced by a comment. But since all cases
cover switches not handling a case or not returning will
likely cause some diagnostic to be emitted which is better
than running into an ICE only at runtime.
Bootstrapped and tested on x86_64-unknown-linux-gnu - any
comments?
Thanks,
Richard.
2021-11-24 Richard Biener <rguenther@suse.de>
* tree.h (reverse_storage_order_for_component_p): Remove
spurious gcc_unreachable.
* cfganal.c (dfs_find_deadend): Likewise.
* fold-const-call.c (fold_const_logb): Likewise.
(fold_const_significand): Likewise.
* gimple-ssa-store-merging.c (lhs_valid_for_store_merging_p):
Likewise.
gcc/c-family/
* c-format.c (check_format_string): Remove spurious
gcc_unreachable.
---
gcc/c-family/c-format.c | 2 --
gcc/cfganal.c | 2 --
gcc/fold-const-call.c | 2 --
gcc/gimple-ssa-store-merging.c | 2 --
gcc/tree.h | 2 --
5 files changed, 10 deletions(-)
Comments
Hello,
On Thu, 25 Nov 2021, Richard Biener via Gcc-patches wrote:
> It seems to be a style to place gcc_unreachable () after a
> switch that handles all cases with every case returning.
> Those are unreachable (well, yes!), so they will be elided
> at CFG construction time and the middle-end will place
> another __builtin_unreachable "after" them to note the
> path doesn't lead to a return when the function is not declared
> void.
>
> So IMHO those explicit gcc_unreachable () serve no purpose,
> if they could be replaced by a comment.
Never document in comments what you can document in code (IMO). I think
the code as-is clearly documents the invariants and expectations and
removing the gcc_unreachable() leads to worse sources.
Can't you simply exempt warning on unreachable __builtin_unreachable()?
It seems an obvious thing that the warning should _not_ warn about, after
all, quite clearly, the author is aware of that being unreachable, it says
so, right there.
Ciao,
Michael.
On Thu, 25 Nov 2021, Michael Matz wrote:
> Hello,
>
> On Thu, 25 Nov 2021, Richard Biener via Gcc-patches wrote:
>
> > It seems to be a style to place gcc_unreachable () after a
> > switch that handles all cases with every case returning.
> > Those are unreachable (well, yes!), so they will be elided
> > at CFG construction time and the middle-end will place
> > another __builtin_unreachable "after" them to note the
> > path doesn't lead to a return when the function is not declared
> > void.
> >
> > So IMHO those explicit gcc_unreachable () serve no purpose,
> > if they could be replaced by a comment.
>
> Never document in comments what you can document in code (IMO). I think
> the code as-is clearly documents the invariants and expectations and
> removing the gcc_unreachable() leads to worse sources.
>
> Can't you simply exempt warning on unreachable __builtin_unreachable()?
> It seems an obvious thing that the warning should _not_ warn about, after
> all, quite clearly, the author is aware of that being unreachable, it says
> so, right there.
gcc_unreachable () is not actually __builtin_unreachable () but instead
fancy_abort (__FILE__, __LINE__, __FUNCTION__). Yes, I agree
that the warning shouldn't warn about "this is unrechable", but if it's
not plain __builtin_unreachable () then we'd need a new function
attribute on it which in this particular case means an alternate
"fancy_abort" since in general fancy_aborts are of course reachable.
We could also handle all noreturn calls this way and not diagnose
those if they are unreachable in exchange for some false negatives.
Btw, I don't agree with "Never document in comments what you can document
in code" in this case, but I take it as a hint that removing
gcc_unreachable in those cases should at least leave a comment in
there?
Richard.
On 11/25/2021 6:33 AM, Richard Biener via Gcc-patches wrote:
> It seems to be a style to place gcc_unreachable () after a
> switch that handles all cases with every case returning.
> Those are unreachable (well, yes!), so they will be elided
> at CFG construction time and the middle-end will place
> another __builtin_unreachable "after" them to note the
> path doesn't lead to a return when the function is not declared
> void.
>
> So IMHO those explicit gcc_unreachable () serve no purpose,
> if they could be replaced by a comment. But since all cases
> cover switches not handling a case or not returning will
> likely cause some diagnostic to be emitted which is better
> than running into an ICE only at runtime.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu - any
> comments?
>
> Thanks,
> Richard.
>
> 2021-11-24 Richard Biener <rguenther@suse.de>
>
> * tree.h (reverse_storage_order_for_component_p): Remove
> spurious gcc_unreachable.
> * cfganal.c (dfs_find_deadend): Likewise.
> * fold-const-call.c (fold_const_logb): Likewise.
> (fold_const_significand): Likewise.
> * gimple-ssa-store-merging.c (lhs_valid_for_store_merging_p):
> Likewise.
>
> gcc/c-family/
> * c-format.c (check_format_string): Remove spurious
> gcc_unreachable.
They would be a check if someone added a case to the switch that didn't
return. But we'd get a return-value warning if that happened. So I
don't see that they serve much purpose.
> ---
> gcc/c-family/c-format.c | 2 --
> gcc/cfganal.c | 2 --
> gcc/fold-const-call.c | 2 --
> gcc/gimple-ssa-store-merging.c | 2 --
> gcc/tree.h | 2 --
> 5 files changed, 10 deletions(-)
>
> diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
> index e735e092043..617fb5ea626 100644
> --- a/gcc/c-family/c-format.c
> +++ b/gcc/c-family/c-format.c
> @@ -296,8 +296,6 @@ check_format_string (const_tree fntype, unsigned HOST_WIDE_INT format_num,
> *no_add_attrs = true;
> return false;
> }
> -
> - gcc_unreachable ();
> }
>
> /* Under the control of FLAGS, verify EXPR is a valid constant that
> diff --git a/gcc/cfganal.c b/gcc/cfganal.c
> index 0cba612738d..48598e55c01 100644
> --- a/gcc/cfganal.c
> +++ b/gcc/cfganal.c
> @@ -752,8 +752,6 @@ dfs_find_deadend (basic_block bb)
> next = e ? e->dest : EDGE_SUCC (bb, 0)->dest;
> }
> }
> -
> - gcc_unreachable ();
> }
>
>
> diff --git a/gcc/fold-const-call.c b/gcc/fold-const-call.c
> index d6cb9b11a31..c542e780a18 100644
> --- a/gcc/fold-const-call.c
> +++ b/gcc/fold-const-call.c
> @@ -429,7 +429,6 @@ fold_const_logb (real_value *result, const real_value *arg,
> }
> return false;
> }
> - gcc_unreachable ();
> }
>
> /* Try to evaluate:
> @@ -463,7 +462,6 @@ fold_const_significand (real_value *result, const real_value *arg,
> }
> return false;
> }
> - gcc_unreachable ();
> }
>
> /* Try to evaluate:
> diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
> index e7c90ba8b59..13413ca4cd6 100644
> --- a/gcc/gimple-ssa-store-merging.c
> +++ b/gcc/gimple-ssa-store-merging.c
> @@ -4861,8 +4861,6 @@ lhs_valid_for_store_merging_p (tree lhs)
> default:
> return false;
> }
> -
> - gcc_unreachable ();
> }
>
> /* Return true if the tree RHS is a constant we want to consider
> diff --git a/gcc/tree.h b/gcc/tree.h
> index f0e72b55abe..094501bd9b1 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -5110,8 +5110,6 @@ reverse_storage_order_for_component_p (tree t)
> default:
> return false;
> }
> -
> - gcc_unreachable ();
> }
>
> /* Return true if T is a storage order barrier, i.e. a VIEW_CONVERT_EXPR
On Sun, 28 Nov 2021, Jeff Law wrote:
>
>
> On 11/25/2021 6:33 AM, Richard Biener via Gcc-patches wrote:
> > It seems to be a style to place gcc_unreachable () after a
> > switch that handles all cases with every case returning.
> > Those are unreachable (well, yes!), so they will be elided
> > at CFG construction time and the middle-end will place
> > another __builtin_unreachable "after" them to note the
> > path doesn't lead to a return when the function is not declared
> > void.
> >
> > So IMHO those explicit gcc_unreachable () serve no purpose,
> > if they could be replaced by a comment. But since all cases
> > cover switches not handling a case or not returning will
> > likely cause some diagnostic to be emitted which is better
> > than running into an ICE only at runtime.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu - any
> > comments?
> >
> > Thanks,
> > Richard.
> >
> > 2021-11-24 Richard Biener <rguenther@suse.de>
> >
> > * tree.h (reverse_storage_order_for_component_p): Remove
> > spurious gcc_unreachable.
> > * cfganal.c (dfs_find_deadend): Likewise.
> > * fold-const-call.c (fold_const_logb): Likewise.
> > (fold_const_significand): Likewise.
> > * gimple-ssa-store-merging.c (lhs_valid_for_store_merging_p):
> > Likewise.
> >
> > gcc/c-family/
> > * c-format.c (check_format_string): Remove spurious
> > gcc_unreachable.
> They would be a check if someone added a case to the switch that didn't
> return. But we'd get a return-value warning if that happened. So I don't see
> that they serve much purpose.
I've pushed the change.
Richard.
@@ -296,8 +296,6 @@ check_format_string (const_tree fntype, unsigned HOST_WIDE_INT format_num,
*no_add_attrs = true;
return false;
}
-
- gcc_unreachable ();
}
/* Under the control of FLAGS, verify EXPR is a valid constant that
@@ -752,8 +752,6 @@ dfs_find_deadend (basic_block bb)
next = e ? e->dest : EDGE_SUCC (bb, 0)->dest;
}
}
-
- gcc_unreachable ();
}
@@ -429,7 +429,6 @@ fold_const_logb (real_value *result, const real_value *arg,
}
return false;
}
- gcc_unreachable ();
}
/* Try to evaluate:
@@ -463,7 +462,6 @@ fold_const_significand (real_value *result, const real_value *arg,
}
return false;
}
- gcc_unreachable ();
}
/* Try to evaluate:
@@ -4861,8 +4861,6 @@ lhs_valid_for_store_merging_p (tree lhs)
default:
return false;
}
-
- gcc_unreachable ();
}
/* Return true if the tree RHS is a constant we want to consider
@@ -5110,8 +5110,6 @@ reverse_storage_order_for_component_p (tree t)
default:
return false;
}
-
- gcc_unreachable ();
}
/* Return true if T is a storage order barrier, i.e. a VIEW_CONVERT_EXPR