Pádraig Brady <[email protected]> writes: > On 15/08/2026 06:11, Collin Funk wrote: >> I pushed this mostly because I prefer marking default cases with >> unreachable so the reader knows that not all representable values >> can/should occur there. However, I did notice it from an interesting >> -fanalyzer bug/limitation. >> -- 8< -- >> Analyzer mistakenly reports that the CSWTCH lookup table can >> overflow >> here since it cannot infer the value of C from the previously called >> switch statement in main. See: >> <https://gcc.gnu.org/bugzilla/PR126885>. >> * src/stdbuf.c (optc_to_fileno): Add a default label with a call to >> unreachable. >> --- >> src/stdbuf.c | 2 ++ >> 1 file changed, 2 insertions(+) >> diff --git a/src/stdbuf.c b/src/stdbuf.c >> index ff19c6056..fb7de79b3 100644 >> --- a/src/stdbuf.c >> +++ b/src/stdbuf.c >> @@ -184,6 +184,8 @@ optc_to_fileno (int c) >> case 'o': >> ret = STDOUT_FILENO; >> break; >> + default: >> + unreachable (); >> } >> return ret; > > That doesn't introduce a bug but does introduce coupling. > I.e. we only call optc_to_fileno() with one of the handled cases, > but that function in isolation can be called with anything, > so the code is now brittle if adjusted in future. > > Also unreachable() is dangerous IMHO. See: > https://github.com/coreutils/coreutils/commit/e661c7a52 > There with clang at least I saw code just run off the end > of a function into arbitrary code. > I need to audit/think more about its use in coreutils.
Ouch. I forgot about that commit. I never really minded just calling abort, to be honest. I guess that affirm (false) is nicer since you can leave it up to the person configuring what behavior they prefer. > A more appropriate adjustment might be to: > > default: > ret = -1; > break; Yes, that makes sense. Sadly, though, analyzer is still unhappy with it. But I guess analyzer is always a bit noisy, so one shouldn't lose sleep over it. Collin
