[Toybox] __attribute__((__noreturn__)) vs _Noreturn

[Rob Landley]

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];

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.

> 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)...?

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

> 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?

> 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...)

> 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.

Rob