[Toybox] __attribute__((__noreturn__)) vs _Noreturn

[enh]

On Tue, May 10, 2022 at 10:44 AM Rob Landley <rob at landley.net> wrote:
>
> On 5/9/22 18:54, enh via Toybox wrote:
> > i think this question already came up recently, but mainly as a joke
> > before ... "how do you feel about C11?"
>
> It's comfortably past the 7 year support horizon. I haven't got anything against
> C11, I just haven't needed anything from it yet?
>
> Actually, I think that typecast constant array syntax is from C11:
>
>   s = ((char *[]){"NICE", "SCHED", "ETIME", "PCPU", "VSIZE", "UNAME"})[k];

my personal favorite is `struct type var = { .one_thing_i_care_about =
123 };` with everything else guaranteed zeroed.

> So yeah, we may already be depending on it and I should update the design.html
> page...
>
> > because i actually have a specific reason to want it now. (buckle in,
> > because this is going to annoy you...)
> >
> > we'd seen large numbers of native crashes show up from toybox, and
> > oddly they were from the toybox _tests_. which is weird, because we'd
> > have expected the tests to _fail_ in that case, and such a problem not
> > to get checked in.
> >
> > (so, long-term question: should/can we improve the test suite to spot
> > exit-via-signal and automatically make that a FAIL?)
>
> I thought it already did ? runtest.sh line 140:
>
>   # Catch segfaults
>   [ $RETVAL -gt 128 ] && [ $RETVAL -lt 255 ] &&
>     echo "exited with signal (or returned $RETVAL)" >> actual
>
> This replaces the output it's comparing with "exited with signal..." which
> should never match the real output, and should be printed in the diff.

/me checks...

ah, yeah, the trouble was it was the common pattern of a test that
says `command || ...` or `command && ...` which doesn't have a third
case of "but die completely if it was > 128".

> > but short term, what's going on here? turns out i could reduce the
> > specific failing test quite significantly:
> >
> > vsoc_x86_64:/ # gzip /proc/version
> > gzip: /proc/version.gz: No such file or directory
> > Segmentation fault
> > 139|vsoc_x86_64:/ # ^D
> >
> > and the test passes because we manage to get that error message out
> > before we die :-)
>
> Uncaught signals should return 128+signum, and we test for that range and stomp
> the generated output with an error message...?
>
> > why do we die? tl;dr: because clang drops the rest of the function
> > after the conditional "call a noreturn or a non-noreturn function"
> > line in xcreate_stdio:
> >
> >   if (fd == -1) ((flags&WARN_ONLY) ? perror_msg_raw : perror_exit_raw)(path);
> >
> > this all works fine on arm, btw; only x86-64 (tbh, i don't remember
> > whether i tested x86) is affected.
>
> Do you mean "drops" as in "optimizes out"? Except we only do it if (fd == -1)...?

yeah, all the rest of the function disappears.

> I'm not following what's going on here.

i don't think you should expect to ... it's a compiler bug :-)

> > but what does any of this have to do with C11?
> >
> > well, oddly (and, yeah, we'll chase the miscompile as a bug anyway)
> > the best workaround i've seen (thanks here to +jamesfarrell for
> > finding someone to have a proper look at llvm) is to replace the
> > current `__attribute__((__noreturn__)`s with C11 `_Noreturn`. yeah,
> > you'd assume they would boil down to the same thing, but ... nope.
>
> I would very much assume it boils down to the same thing, yes?

apparently the representation is different enough that only the
attribute has trouble. (and i've confirmed that via godbolt.)

> > given that Android T is stuck with that clang (and this bug hasn't
> > actually been fixed upstream yet), i think our two T workaround
> > options are:
> >
> > 1. revert the change
> > [https://github.com/landley/toybox/commit/20376512ae99be103875a1605ad69a7c876b818a]
> > that added the attribute to the _raw() functions.
>
> Yet another reason why compilers generating unnecessary warnings makes me sad.
>
> > 2. use _Noreturn instead, either
> > a) by assuming C11 because it's 2022 and even Linux is on board, or
> > b) in a #if that checks the C standard in use (and change the toybox
> > Android.bp to opt in to C11, since
> > https://android-review.googlesource.com/c/platform/build/soong/+/2043314
> > isn't going in until Android U)
>
> Eh, moving to the 2011 standard 11 years later isn't a heavy lift. I remain
> deeply sad that the committee keeps doing stupid underscore things with
> namespaces rather than just biting the bullet and adding actual names to the
> language. (I get to choose when I upgrade to the new version, and I have to
> shuffle around strlcpy() already unless you think "don't include string.h" is a
> reasonable thing for C code to avoid doing to avoid "polluting" the namespace...)

well, note that (as usual) they've done _both_ to try to keep everyone happy.

#include <stdnoreturn.h> if you want `noreturn` rather than `_Noreturn`.

> > happy to send any/all of the above patches, but wanted to know which
> > option you dislike least...
>
> So we're working around a clang compiler bug by using a C11 name for an
> __attribute__(blah) because the compiler miscompiles one and doesn't miscompile
> the other (even though they SHOULD be synonyms)?
>
> *shrug* I've done uglier things with a similar commit comment. "Here's why this
> is ugly: it's working around a compiler bug." Ok then.

yeah, pretty much. (obviously another alternative is "i revert the
change that tickled this locally in T, and we worry about a proper fix
for U", but given that this still isn't fixed upstream yet, others
might hit it too.)

plus i actually like some of the other C11 stuff, so even though this
is a _stupid_ forcing function, i'm not unhappy to have _a_ forcing
function :-)

(if i didn't already say, my reasoning for moving the AOSP default to
C11 for U is that i feel like we're at the tipping point now where
"even C programmers", not known for their love of the new, are
starting to _assume_ C11, and we're having more and more people have
to manually opt in/ask why stuff doesn't "just work". linux moving
will probably be quite a heavy thumb on that side of the scale. and in
the C committee's defense [in contrast to the C++ committee], they're
pretty good about not breaking stuff. literally the only problems i've
seen have been from people who had untested and incorrect code in #ifs
that checked for C11...)

but, yeah, this kind of "codegen bug specific to one architecture" was
the kind of GCC nonsense i thought LLVM's cleaner design was supposed
to save us from :-(

(to be fair, i _do_ see a lot less of this with LLVM than i used to with GCC.)

> Rob