Message ID | p3s3p794-r83n-5r6-2996-s746qo3q2r7p@fhfr.qr |
---|---|
State | New |
Headers | show |
Series | Remove unreachable gcc_unreachable () at the end of functions | expand |
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.
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