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 <e...@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 <e...@google.com> wrote: > > > > On Tue, May 10, 2022 at 10:44 AM Rob Landley <r...@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 _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net