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
From db3ccf78e2a225457cea06b9b4a3a8f9cf09c597 Mon Sep 17 00:00:00 2001
From: Elliott Hughes <e...@google.com>
Date: Tue, 10 May 2022 14:57:02 -0700
Subject: [PATCH 1/2] Default to C11 with GNU extensions.
(Both gcc and clang call this "gnu11".)
---
configure | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/configure b/configure
index d511d948..d96a05f4 100755
--- a/configure
+++ b/configure
@@ -12,7 +12,7 @@ then
fi
# Warn about stuff, disable stupid warnings, be 8-bit clean for utf8.
-CFLAGS="$CFLAGS -Wall -Wundef -Werror=implicit-function-declaration -Wno-char-subscripts -Wno-pointer-sign -Wno-string-plus-int -funsigned-char"
+CFLAGS="$CFLAGS -std=gnu11 -Wall -Wundef -Werror=implicit-function-declaration -Wno-char-subscripts -Wno-pointer-sign -Wno-string-plus-int -funsigned-char"
# Set default values if variable not already set
: ${CC:=cc} ${HOSTCC:=cc} ${GENDIR:=generated} ${KCONFIG_CONFIG:=.config}
--
2.36.0.512.ge40c2bad7a-goog
From 15badcdcaa3ba329c69d8bbc34d48a6a381ce8ae Mon Sep 17 00:00:00 2001
From: Elliott Hughes <e...@google.com>
Date: Tue, 10 May 2022 14:59:53 -0700
Subject: [PATCH 2/2] Use C11 noreturn.
Works around a clang bug in the version of clang shipping with Android T
that causes a miscompile in xstdio_create() on x86-64, that oddly only
affects __attribute__((__noreturn__)) and not _Noreturn!
---
lib/lib.h | 14 +++++++-------
toys.h | 1 +
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/lib/lib.h b/lib/lib.h
index 1bba7b04..279896c4 100644
--- a/lib/lib.h
+++ b/lib/lib.h
@@ -109,8 +109,8 @@ struct dirtree *dirtree_read(char *path, int (*callback)(struct dirtree *node));
// xwrap.c
void xstrncpy(char *dest, char *src, size_t size);
void xstrncat(char *dest, char *src, size_t size);
-void _xexit(void) __attribute__((__noreturn__));
-void xexit(void) __attribute__((__noreturn__));
+noreturn void _xexit(void);
+noreturn void xexit(void);
void *xmmap(void *addr, size_t length, int prot, int flags, int fd, off_t off);
void *xmalloc(size_t size);
void *xzalloc(size_t size);
@@ -189,13 +189,13 @@ time_t xmktime(struct tm *tm, int utc);
void verror_msg(char *msg, int err, va_list va);
void error_msg(char *msg, ...) printf_format;
void perror_msg(char *msg, ...) printf_format;
-void error_exit(char *msg, ...) printf_format __attribute__((__noreturn__));
-void perror_exit(char *msg, ...) printf_format __attribute__((__noreturn__));
-void help_exit(char *msg, ...) printf_format __attribute__((__noreturn__));
+noreturn void error_exit(char *msg, ...) printf_format;
+noreturn void perror_exit(char *msg, ...) printf_format;
+noreturn void help_exit(char *msg, ...) printf_format;
void error_msg_raw(char *msg);
void perror_msg_raw(char *msg);
-void error_exit_raw(char *msg) __attribute__((__noreturn__));
-void perror_exit_raw(char *msg) __attribute__((__noreturn__));
+noreturn void error_exit_raw(char *msg);
+noreturn void perror_exit_raw(char *msg);
ssize_t readall(int fd, void *buf, size_t len);
ssize_t writeall(int fd, void *buf, size_t len);
off_t lskip(int fd, off_t offset);
diff --git a/toys.h b/toys.h
index fe688458..69c1bd97 100644
--- a/toys.h
+++ b/toys.h
@@ -29,6 +29,7 @@
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
+#include <stdnoreturn.h>
#include <string.h>
#include <strings.h>
#include <sys/mman.h>
--
2.36.0.512.ge40c2bad7a-goog
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
