Skip to content

Commit

Permalink
Restore build without __sync_fetch_and_{add,sub} (re: 07cc71b)
Browse files Browse the repository at this point in the history 
The more I think about it, the more it seems obvious that commit
07cc71b (PR #14) is quite simply a workaround for a GCC optimiser
bug, and (who knows?) possibly an old, long-fixed one, as the bug
report is years old.

The commit also caused ksh to fail to build on HP-UX B.11.11 with
GCC 4.2.3 (hosted at polarhome.com), because it doesn't have
__sync_fetch_and_add() and __sync_fetch_and_sub(). It may fail on
other systems. The GCC documentation says these are legacy:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html

HELP WANTED: what I would like best is if someone could come up
with some way of detecting this optimiser bug and then error out
with a message along the lines of "please upgrade your broken
compiler". It would probably need to be a new iffe test.

Meanwhile, let's try it this way for a while and see what happens:

src/cmd/ksh93/include/jobs.h:
- Restore original ksh version of job_lock()/job_unlock() macros.
- Use the workaround version only if the compiler has the builtins
  __sync_fetch_and_add() and __sync_fetch_and_sub().
  • Loading branch information
@McDutchie
McDutchie committed Jun 14, 2020
1 parent f95d310 commit 58560db
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 2 deletions.
2 changes: 1 addition & 1 deletion TODO
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Fix regression test failures:
______
Fix build system:

- ksh does not currently build on NetBSD, AIX, HP-UX, Solaris, or QNX.
- ksh does not currently build on NetBSD, AIX, Solaris, or QNX.
- Reimport the removed nmake. It is necessary for changes in Makefiles
to take effect. The machine-generated Mamfiles are now used as a fallback,
but they are not meant to be edited by hand.
Expand Down
26 changes: 25 additions & 1 deletion src/cmd/ksh93/include/jobs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,17 @@ 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 { \
Expand All @@ -163,6 +171,22 @@ extern struct jobs job;
asodecint(&job.in_critical); \
} \
} while(0)
#else
/*
* Original ksh93 version.
*/
#define job_lock() (job.in_critical++)
#define job_unlock() \
do { \
int sig; \
if (!--job.in_critical && (sig = job.savesig)) \
{ \
if (!job.in_critical++ && !vmbusy()) \
job_reap(sig); \
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[];
Expand Down

12 comments on commit 58560db

@JohnoKing
Copy link

@JohnoKing JohnoKing commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defined(__sync_fetch_and_add) always evaluates as false. __has_builtin should be used instead to detect GCC builtins.

@McDutchie
Copy link
Author

@McDutchie McDutchie commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JohnoKing, I will have to make that change.

Can I trust that all compilers have __has_builtin, or is that also a gcc-ism? If not, how would I test for the presence of __has_builtin?

@JohnoKing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work for compilers without __has_builtin:

#if !__has_builtin
#define __has_builtin(x) 0
#endif

@McDutchie
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Note to self: in future, read link before asking question... :P

@McDutchie
Copy link
Author

@McDutchie McDutchie commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnoKing : before I goof up again: does this look ok to you?

(edit: silly brokenness deleted)

@JohnoKing
Copy link

@JohnoKing JohnoKing commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will work.

@McDutchie
Copy link
Author

@McDutchie McDutchie commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, thanks for checking!

@JohnoKing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a minute, that actually will not work. All the code for job_lock and job_unlock is enclosed in if defined(__has_builtin). That will break with a compiler without __has_builtin.

@JohnoKing
Copy link

@JohnoKing JohnoKing commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a better version:

/*
 * Job locking and unlocking macros
 */
#if !defined(__has_builtin)
#define __has_builtin(x) 0
#endif
#if __has_builtin(__sync_fetch_and_add) && __has_builtin(__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 { \
          int  sig; \
          if (!--job.in_critical && (sig = job.savesig)) \
          { \
               if (!job.in_critical++ && !vmbusy()) \
                    job_reap(sig); \
               job.in_critical--; \
          } \
     } while(0)
#endif /* __has_builtin(__sync_fetch_and_add) && __has_builtin(__sync_fetch_and_sub) */

@McDutchie
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that just hit me as well, just before I saw your message! Thanks for the better version.

@McDutchie
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a little more research on __has_builtin. It looks like gcc didn't add it until very recently... in GCC 10, in May 2019!

Given the age of the segfault reports, this makes it look rather unlikely that any gcc version with __has_builtin actually has this optimiser bug... surely, wrongly optimising out something rudimentary like a unary increase operator would have been spotted by now?

Absent __has_builtin on older gcc versions, the only way to test for a builtin is by adding an iffe test. I don't know how iffe works, I don't want to invest the time to learn how it works, and I don't see anyone else volunteering to add such a test any time soon.

So now I'm inclined to simply revert the patch instead (and waste your work, sorry about that). What do you think?

@JohnoKing
Copy link

@JohnoKing JohnoKing commented on 58560db Jun 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize only GCC 10 has __has_builtin. Reverting the patch should be fine (for now). I'll see if I can find a different fix that doesn't rely on GCC-specific features.

Please sign in to comment.