1
0
Fork 0
mirror of git://git.code.sf.net/p/cdesktopenv/code synced 2025-03-09 15:50:02 +00:00

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.: 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.
This commit is contained in:
Martijn Dekker 2020-06-15 02:52:50 +02:00
parent 58560db768
commit b7f48e8a10

View file

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