[Toybox] __attribute__((__noreturn__)) vs _Noreturn
enh
enh at google.com
Mon May 9 16:54:34 PDT 2022
i think this question already came up recently, but mainly as a joke
before ... "how do you feel about C11?"
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?)
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 :-)
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.
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.
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.
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)
happy to send any/all of the above patches, but wanted to know which
option you dislike least...
More information about the Toybox
mailing list