[Toybox] __attribute__((__noreturn__)) vs _Noreturn

[enh]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Default-to-C11-with-GNU-extensions.patch
Type: text/x-patch
Size: 977 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20220510/bf577ca0/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Use-C11-noreturn.patch
Type: text/x-patch
Size: 2419 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20220510/bf577ca0/attachment-0005.bin>