[Toybox] __attribute__((__noreturn__)) vs _Noreturn

[enh]

oh, and if you want to see the miscompile "in action", here's a
godbolt link showing that [[noreturn]] and _Noreturn work in their
respective languages, but __attribute__((__noreturn__)) is broken in C
(but not C++!): https://godbolt.org/z/T6nMoa75G

On Tue, May 10, 2022 at 3:08 PM enh <enh at google.com> wrote:
>
> attached two separate patches; one to move, the other to take
> advantage of the move.
>
> given that `_Noreturn` is required to be at the start, i kind of wish
> they'd made it imply `void`; `noreturn void` seems a bit redundant!
>
> On Tue, May 10, 2022 at 11:00 AM enh <enh at google.com> wrote:
> >
> > 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