From b7f48e8a100fc63d6b7b28b82c986cb60d1427fe Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Mon, 15 Jun 2020 02:52:50 +0200 Subject: [PATCH] Revert job locking patch (re: 07cc71b8, 58560db7) This reverts the patch for the job_lock and job_unlock macros. As I said in 58560db7, this is very probably a workaround for an optimiser bug in certain versions of GCC, at least 2017 versions. In the version I committed, that workaround version never gets used, because you cannot use #if defined(...) to check for the presence of a compiler builtin. Thanks to Johnothan King for keeping an eye on my code and pointing that out to me. What is needed to test for the presence of a compiler builtin is a builtin macro called __has_builtin (and it *can* be tested for using #if defined...()).. This is a clang invention. It was not added to gcc until version 10, which was only released in a first stable version just over a month ago. Ref.: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970#c14 https://gcc.gnu.org/gcc-10/ However, for gcc 10, it seems unlikely the patch is still needed. (Although it would certainly be a good idea to test that.) And for the older gcc versions that do need it, we cannot use __has_builtin, which means we need to define a dummy that always returns false, so the workaround version is never used. Ref.: https://github.com/ksh93/ksh/commit/58560db7#commitcomment-39900259 And we cannot use the workaround version unconditionally either, because it would cause build failures on compilers without the __sync_fetch_and_add() and __sync_fetch_and_sub() builtins. Which means the only sensible thing left to do is to revert the patch for now. As far as I can tell at this point, for the patch to return to this upstream in a sensible manner, someone would need to: 1. Write a small C program that tests these macros and somehow checks for the presence of that optimiser bug. (This is not going to come from me; my C-fu is simply not good enough.) 2. Incorporate that into the distribution as a test for iffe. (Few know how iffe works, but it's probably not that hard as there are plenty of existing tests to use as a template.) 3. Reinsert the workaround version using the macro defined (or not) by that iffe test, so that it is only compiled in when using a compiler that actually has the bug in question. Until then, this can just continue to be an OS-specific patch for systems with GCC versions that might have that bug. --- src/cmd/ksh93/include/jobs.h | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/src/cmd/ksh93/include/jobs.h b/src/cmd/ksh93/include/jobs.h index af5ea5cd1..84d48dae7 100644 --- a/src/cmd/ksh93/include/jobs.h +++ b/src/cmd/ksh93/include/jobs.h @@ -149,32 +149,6 @@ extern struct jobs job; #define vmbusy() 0 #endif -/* - * Job locking and unlocking macros - */ -#if defined(__sync_fetch_and_add) && defined(__sync_fetch_and_sub) -/* - * This version may prevent segfaults due to a GCC optimizer bug. - * See: https://bugzilla.redhat.com/show_bug.cgi?id=1112306 - * https://bugs.launchpad.net/ubuntu/+source/ksh/+bug/1697501 - */ -#define asoincint(p) __sync_fetch_and_add(p,1) -#define asodecint(p) __sync_fetch_and_sub(p,1) -#define job_lock() asoincint(&job.in_critical) -#define job_unlock() \ - do { \ - int sig; \ - if (asodecint(&job.in_critical)==1 && (sig = job.savesig)) \ - { \ - if (!asoincint(&job.in_critical) && !vmbusy()) \ - job_reap(sig); \ - asodecint(&job.in_critical); \ - } \ - } while(0) -#else -/* - * Original ksh93 version. - */ #define job_lock() (job.in_critical++) #define job_unlock() \ do { \ @@ -186,7 +160,6 @@ extern struct jobs job; job.in_critical--; \ } \ } while(0) -#endif /* defined(__sync_fetch_and_add) && defined(__sync_fetch_and_sub) */ extern const char e_jobusage[]; extern const char e_done[];