Conversation
This commit is a compilation of many old patches of mine.
It removes obsolete code and enforces C99 as a build
requirement.
Changes:
- Remove many unused tests and useless feature tests.
(This was a bit more involved for <sys/types.h>, which
had annoying special casing.) This included the
'check for global headers' in the iffe script.
- Assume fchmod is present and use it instead of chmod in
shcomp for slightly better performance.
- dotpaths_lib(): Use fstat rather than stat for the PWD
for better performance.
- Scrap pre-C99 fallbacks for iswblank(3).
- Replaced usage of the obsolescent timeofday() macro with the
more portable tvgettime() function in libast, which itself uses
the modern clock_gettime internally.
- Replaced usage of strcopy with either strcpy or stpcpy in
cases where no buffer overlap occurs. There are perhaps
more places where replacement is possible (ksh93v- and ksh2020
do this far more liberally), but I'm trying to be conservative
with these changes to avoid accidentally introducing bugs.
- Replaced an instance of strncopy with strlcpy in keytrap()
because the buffers don't overlap.
- Also replaced some instances of strncpy with strlcpy
(vide att#266 and f63ebd0).
- Add pertinent documentation as it relates to str*copy's
usage with overlapping buffers.
- Backported a modified version of ksh93v-'s stpcpy fallback.
- Introduced usage of C99 restrict, primarily for standards
compliance in compatibility functions (though some regular
functions like sfopen() do also use restrict now).
- Assume C99 __func__ is available and drop now unneeded
C90 feature tests.
- Added a few casts to fix some compiler warnings.
- Remove basic iffe probes for various C99 multibyte functions
(excluding those that still need tests due to platform-specific
bugs). Also removed other unnecessary probes for C99 and POSIX
functionality.
- Remove unnecessary mktime.c, wc.c and wordexp.c compat files
(the operating system should provide these functions).
- Switched to using C99-style struct init throughout the codebase.
- Removed usage of bzero, bcopy, memclear and memzero in favor
of ISO C memset and memcpy.
- make.probe: Automatically use the latest C standard available
(i.e. C23) because GCC/Clang may use something older by default
whilst having support for newer standards.
I have additional changes for using C99-style for loops and bools,
but those have been excluded because this patch is big enough as it is.
Progresses ksh93#777
(This commit doesn't include or fix the removed _c99_in_the_wild code.)
I would also like to think about/discuss this some more before committing to these, because so far I'm failing to see an actual advantage to either of these. I quite like the C90 norm that all variables are declared at the beginning of each braces block that they're scoped to; it's good for clarity, IMHO. As for bools, are they really any more than syntactic sugar? I could be missing something, e.g., do they improve performance? If not, I don't feel the need for them. |
|
Alright, I think we're going to a need C99 flexible struct member to fix the alignment issue in #964 without more hackery. So, it's time for this. |
|
Unfortunately, the PR doesn't build on macOS. |
Author
Local variables in for loops via
I don't have anything running macOS, but perhaps that's caused by sfstrtof.h using --- a/src/lib/libast/sfio/sfstrtof.h
+++ b/src/lib/libast/sfio/sfstrtof.h
@@ -59,11 +59,6 @@
#endif
#endif
-#if S2F_type == 2 && _ast_fltmax_double
-#undef S2F_type
-#define S2F_type 1
-#endif
-
#if S2F_type == 0
#define S2F_number float
#define S2F_ldexp ldexp |
|
|
While working on another future PR (heavily focused on performance tuning), I noticed that the generated assembly for sh_subshell() will have two vzeroupper instructions at the start when compiling with -O3 -march=x86-64-v3. This moves the sfsync() to occur prior to struct initialization and sh_arguse to after sh_pushcontext(), which removes one expensive vzeroupper. Seeing as variables will still be declared at the beginning of blocks for stylistic reasons, this reverts sh_subshell() to using C89-style struct init. _dtopen() is reverted to the old code for this reason also.
|
Unfortunately, it doesn't fix it. The patch makes it compile, but hundreds of regression tests in most test scripts are now failing with a segfault at sfstrtof.h:510. Example: |
|
The PR compiles and runs fine on my system now. I notice you're still tweaking it. Let me know when you're ready. |
Author
|
The tweaking is already done. The |
|
Wait, why the new S2F_type 3? Isn't it equivalent to 2? |
Author
|
That's a workaround which allows |
|
OK, that's a bit of a hack but we can look at that later. |
|
Unfortunately, we're not ready yet. Build fails on QNX 6.5.0 (with gcc 4.4.2) at a very early stage -- a test in features/common fails to compile. Can confirm that this PR causes it and the latest dev branch commit builds fine. This is the failing test: ksh/src/lib/libast/features/common Lines 101 to 229 in a6dd3a3 Relevant part of build log with IFFEFLAGS=-d1 |
|
This PR also fails to build on my Solaris 9/SPARC VM with gcc 4.6.4. It's a simpler failure: But it's odd that apparently stpcpy is not detected in the libraries (_lib_stpcpy is not defined) so that it encounters this conflicting function definition. But conflicting with what? So, if this OS doesn't provide stpcpy in either the headers or the libraries, what on earth is GCC complaining about then? |
Author
|
For QNX, maybe it's the changes to |
|
I found the cause of the Solaris 9 (and also 10) build failure, because Solaris 10 x86_64 with gcc 5.5.0 gave a much more informative error message. The extern generated by features/sys is wrong, so libast was quite simply conflicting with itself. The fix: diff -ur JohnoKing-ksh-1fb7048.orig/src/lib/libast/features/sys JohnoKing-ksh-1fb7048/src/lib/libast/features/sys
--- JohnoKing-ksh-1fb7048.orig/src/lib/libast/features/sys Sun Apr 5 20:32:15 2026
+++ JohnoKing-ksh-1fb7048/src/lib/libast/features/sys Sun Apr 5 21:31:59 2026
@@ -60,6 +60,6 @@
extern memalign void* (size_t, size_t)
extern strlcat size_t (char *restrict, const char *restrict, size_t)
extern strlcpy size_t (char *restrict, const char *restrict, size_t)
-extern stpcpy size_t (char *restrict, const char *restrict)
+extern stpcpy char* (char *restrict, const char *restrict)
print #include <stdarg.h> |
|
With that fix, this PR builds and has zero regression test failures on Solaris 10.1 x86_64. (Still waiting for my emulated Solaris 9/SPARC VM to finish building, I expect it'll be fine.) |
|
Unfortunately, reverting the iffe and features/common changes made no difference to the build failure on QNX. |
|
I figured out the cause of the build failure on QNX. I'd never noticed it has two C compilers: Unfortunately, though, the build now fails at a later stage: |
Nope. This now fails at the very end, at link time: |
|
Build now confirmed working on QNX 6.5.0, as well as Solaris 9, 10.1, and 11.3 with gcc, and illumos (OmniOS). But on Solaris 11.3 and 11.4, and illumos (but not on Solaris 9 or 10.1), there are five new math-related regression test failures: |
- The iswalpha function properly takes a wint_t, not a wchar_t. For this commit to work, <ast_wchar.h> must be included in <ast_std.h> to provide wint_t for the global struct with ast.mb.alpha, and the include directive must be used after all the FILE* macro hackery is dealt with. I still need to debug that Solaris issue, but my illumos install broke a while ago and I kept putting off fixing it. I'll have to get that working again first, so no ETA for that...
Author
|
I finally got illumos (OmniOS) to work/not lock up. (I also tried to test it on Solaris 11.4, but I can't get that OS to work; the install won't even connect to the internet.) |
|
I found the cause of the Solaris 10.3/10.4 regressions after hours of fruitless systematic elimination of code changes in this PR. The cause is not in the code. It's passing the In the mean time I've also found that I can reproduce the regression (if the It breaks on official Solaris releases, though, so we have to reckon with this. Do we really need to pass the |
|
I was wrong. It's more complicated. When I pass |
Author
|
Passing [*]: WIP megapatch Edit: I removed |
This small compiler test (used by bin/package to error out on a bad or outdated compiler) now includes a flexible array struct member, a function declaration with the restrict keyword, and a designated struct initialiser. IIRC, these are the C99 features we're currently starting to use.
...as -std=c99 may be needed. Also, remove the separate error message for a C++ compiler as the recompiling trick breaks if the C compiler supports flexible array members and designated initialisers but not the restrict keyword (as is the case on QNX 6.5.0 GCC 4.4.2 without passing the -std=c99 flag).
|
Unfortunately this PR still fails to build on QNX. We now need to pass Build log with IFFEFLAGS=-d1 |
|
I have not forgotten about this PR, but I think it's become too big and too complicated, making it difficult to figure out regressions. So I'm going to leave it open and gradually cherry-pick bits from it; your work won't be lost. |
Author
I agree, I made this PR far too large to begin with. |
|
I'm not in a big hurry to get absolutely all the C90 bits changed to C99, by the way. I would like the C99 change to only apply to the compiler and not to the operating system's libc. For example, I want ksh to continue to build on my Solaris 9 SPARC VM, because I like to occasionally test ksh on it; old and obscure systems have their own way of exposing bugs. I've got gcc 4 installed on it, which compiles C99 just fine, but the libc is only C90 compliant (with extensions). So, I think we should mostly leave the feature testing alone at this time, except that tests for C99 compiler features should of course be removed. After looking through this PR, this is my current idea of the priorities:
For the rest, I'm happy to leave well enough alone, but of course I could be missing some things. I'd welcome your thoughts. |
McDutchie
added a commit
that referenced
this pull request
Sep 7, 2026
We were already not C90-compliant as the code was using 'long long'
in a couple of places, so let's make it official now: we now
require a C99-ish compiler (gcc 2.95.3 is out, gcc 3.0+ should
still work).
The code now depends on the following C99 features:
- long long (as before)
- flexible array members (however, many instances of the 'struct
hack' are still left to be converted to these; this change has
some priority as the struct hack is considered UB)
- designated struct initialisers (init members by name)
- __func__ (name of current function)
- inline ('static inline' only for compat with gcc < 4.3)
- restrict (use of this optimisation keyword taken from #942 by
@JohnoKing; not detailed further below, just see the diff)
I like testing ksh on really old systems as they have their own way
of exposing bugs. To avoid large diffs and to keep this historic
code base appropriately old-fashioned and (hopefully) compatible
with GCC 3.0+, I'm intending to keep it C90-ish with the use of
select C99 features such as the above. We also shouldn't assume an
OS with a C99-compliant libc yet.
This commit also silences some compiler warnings on macOS with
clang (CCFLAGS='-O0 -g -funsigned-char -Wall -Wsign-compare
-Wshorten-64-to-32 -Wsign-conversion -Wimplicit-int-conversion').
The fixes I did to that end necessitated other fixes, and so they
cascaded and this commit got a bit large and messy; sorry about
that. (re: 6d13819, 217f0e2, 4fdda42)
src/cmd/INIT/hello.c:
- Rewrite the initial heuristic C compiler probe to require minimal
C99 features. By @JohnoKing (taken from #942). I added a quick
check for __func__.
src/cmd/INIT/mamake.c,
src/lib/libast/comp/assert.c,
src/lib/libast/std/assert.h,
src/lib/libast/misc/vmalloc.c:
- Remove code compiled when __STDC_VERSION__ < 199901L or for
compilers that don't have __func__.
src/lib/libast/features/common:
- Remove obsolete __INLINE__ macro, defined as __inline on non-gcc
_WIN32 only. Perhaps this was for UWIN. (re: 606ca5a, d9a85ca)
- Remove feature tests for __func__ and the __FUNCTION__ fallback.
src/lib/libast/include/ast_std.h:
- Assume we have the inline keyword; remove empty macro fallback.
src/lib/libast/include/sfio.h,
src/lib/libast/sfio/sfflsbuf.c:
- Bump Sfio version to 20260907.
- When not building Sfio itself (!_BLD_sfio), declare static inline
versions of the tiny sfput{d,l,u,m,c}, sfgetc, sf{d,l,u}len,
sffileno, sfeof, sferror, sfclrerr, sfstacked, sfvalue, sfslen,
and sfmaxr functions. These replace the macros or (theoretically,
on non-gcc _WIN32) the __INLINE__ versions.
- Restore the int type for sfputc (it returns the character written
or -1, so the ssize_t makes no sense). (re: 0458a8b)
- For the same reason, restore int type to 2nd arg of _sfflsbuf().
src/lib/libast/sfio/_sf{clrerr,dlen,eof,error,fileno}.c,
src/lib/libast/sfio/_sfget{c,l,u}.c,
src/lib/libast/sfio/_sf{l,s,u}len.c,
src/lib/libast/sfio/_sf{putc,stacked,value}.c,
src/lib/libast/sfio/sfput{d,l,m,u}.c:
- Removed. All of these were either external-linkage fallbacks for
non-static inline definitions (so were used on non-gcc _WIN32
only) or were completely unused due to obsolescence.
src/lib/libast/sfio/sfflsbuf.c,
src/lib/libast/sfio/sfreserve.c:
- Restore the int type to the second argument to _sfflsbuf(): this is
either negative (to return size of flushed data) or a byte,
typecast to unsigned char, to add to the flushed data. Thus,
ptrdiff_t makes no sense here.
- In sfreserve(), fix a bug exposed by a compiler warning triggered
by the change above. In one of the SFFLSBUF() macro expansions,
which calls _sfflsbuf(), the second argument was a data size,
iosz. This is incorrect (see above) and would have caused a
garbage byte to be added to the flushed data. Change to -1, which
is probably what was intended given other such calls that use -1.
src/lib/libast/sfio/sfvprintf.c:
- Delete small fallback macros that were only compiled on Amdahl
UTS, a Unix system for mainframes defunct since 2002 or before.
src/lib/libcmd/cat.c:
- regress(): The static inline function version of sfvalue() is not
an lvalue, so can't be assigned to; since this is a hack anyway,
just use the internal __sf_value() macro definition as an lvalue.
src/lib/libast/include/sfio_t.h,
src/lib/libast/misc/state.c,
src/lib/libast/comp/localeconv.c:
- Use designated struct initialisers.
src/lib/libast/regex/regcomp.c:
- Work around a compiler warning on macOS where MB_CUR_MAX is of
type int (contrarily to POSIX which specifies size_t).
Typecasting the right-hand side to the left-hand type (ptrdiff_t)
works regardless of the type of MB_CUR_MAX.
src/lib/libcmd/stty.c:
- Silence compiler warning on macOS and FreeBSD, where the
sethostname() definition differs: even in 2026, it still uses an
int as the second (name length) argument instead of size_t. All
other systems seem to have switched to size_t by now.
- Fix a bug: add braces around error()/UNREACHABLE() as they will
usually be conditional upon the preceding 'if', though the
somewhat ridiculous preprocessor stuff obscures this.
src/lib/libcmd/join.c,
src/cmd/ksh93/sh/macro.c:
- Introduce new variables instead of reusing existing ones, so each
one can have an appropriate type. This fixes compiler warnings.
src/lib/libast/features/api:
- Bump API version to 20260907.
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
McDutchie
added a commit
that referenced
this pull request
Sep 7, 2026
We were already not C90-compliant as the code was using 'long long'
in a couple of places, so let's make it official now: we now
require a C99-ish compiler (gcc 2.95.3 is out, gcc 3.0+ should
still work).
The code now depends on the following C99 features:
- long long (as before)
- flexible array members (however, many instances of the 'struct
hack' are still left to be converted to these; this change has
some priority as the struct hack is considered UB)
- designated struct initialisers (init members by name)
- __func__ (name of current function)
- inline ('static inline' only for compat with gcc < 4.3)
- restrict (use of this optimisation keyword taken from #942 by
@JohnoKing; not detailed further below, just see the diff)
I like testing ksh on really old systems as they have their own way
of exposing bugs. To avoid large diffs and to keep this historic
code base appropriately old-fashioned and (hopefully) compatible
with GCC 3.0+, I'm intending to keep it C90-ish with the use of
select C99 features such as the above. We also shouldn't assume an
OS with a C99-compliant libc yet.
This commit also silences some compiler warnings on macOS with
clang (CCFLAGS='-O0 -g -funsigned-char -Wall -Wsign-compare
-Wshorten-64-to-32 -Wsign-conversion -Wimplicit-int-conversion').
The fixes I did to that end necessitated other fixes, and so they
cascaded and this commit got a bit large and messy; sorry about
that. (re: 00ecebd, 95cacca, 33e66ea)
src/cmd/INIT/hello.c:
- Rewrite the initial heuristic C compiler probe to require minimal
C99 features. By @JohnoKing (taken from #942). I added a quick
check for __func__.
src/cmd/INIT/mamake.c,
src/lib/libast/comp/assert.c,
src/lib/libast/std/assert.h,
src/lib/libast/misc/vmalloc.c:
- Remove code compiled when __STDC_VERSION__ < 199901L or for
compilers that don't have __func__.
src/lib/libast/features/common:
- Remove obsolete __INLINE__ macro, defined as __inline on non-gcc
_WIN32 only. Perhaps this was for UWIN. (re: 5c16b51, 0ea97b9)
- Remove feature tests for __func__ and the __FUNCTION__ fallback.
src/lib/libast/include/ast_std.h:
- Assume we have the inline keyword; remove empty macro fallback.
src/lib/libast/include/sfio.h,
src/lib/libast/sfio/sfflsbuf.c:
- Bump Sfio version to 20260907.
- When not building Sfio itself (!_BLD_sfio), declare static inline
versions of the tiny sfput{d,l,u,m,c}, sfgetc, sf{d,l,u}len,
sffileno, sfeof, sferror, sfclrerr, sfstacked, sfvalue, sfslen,
and sfmaxr functions. These replace the macros or (theoretically,
on non-gcc _WIN32) the __INLINE__ versions.
- Restore the int type for sfputc (it returns the character written
or -1, so the ssize_t makes no sense). (re: fefe896)
- For the same reason, restore int type to 2nd arg of _sfflsbuf().
src/lib/libast/sfio/_sf{clrerr,dlen,eof,error,fileno}.c,
src/lib/libast/sfio/_sfget{c,l,u}.c,
src/lib/libast/sfio/_sf{l,s,u}len.c,
src/lib/libast/sfio/_sf{putc,stacked,value}.c,
src/lib/libast/sfio/sfput{d,l,m,u}.c:
- Removed. All of these were either external-linkage fallbacks for
non-static inline definitions (so were used on non-gcc _WIN32
only) or were completely unused due to obsolescence.
src/lib/libast/sfio/sfflsbuf.c,
src/lib/libast/sfio/sfreserve.c:
- Restore the int type to the second argument to _sfflsbuf(): this is
either negative (to return size of flushed data) or a byte,
typecast to unsigned char, to add to the flushed data. Thus,
ptrdiff_t makes no sense here.
- In sfreserve(), fix a bug exposed by a compiler warning triggered
by the change above. In one of the SFFLSBUF() macro expansions,
which calls _sfflsbuf(), the second argument was a data size,
iosz. This is incorrect (see above) and would have caused a
garbage byte to be added to the flushed data. Change to -1, which
is probably what was intended given other such calls that use -1.
src/lib/libast/sfio/sfvprintf.c:
- Delete small fallback macros that were only compiled on Amdahl
UTS, a Unix system for mainframes defunct since 2002 or before.
src/lib/libcmd/cat.c:
- regress(): The static inline function version of sfvalue() is not
an lvalue, so can't be assigned to; since this is a hack anyway,
just use the internal __sf_value() macro definition as an lvalue.
src/lib/libast/include/sfio_t.h,
src/lib/libast/misc/state.c,
src/lib/libast/comp/localeconv.c:
- Use designated struct initialisers.
src/lib/libast/regex/regcomp.c:
- Work around a compiler warning on macOS where MB_CUR_MAX is of
type int (contrarily to POSIX which specifies size_t).
Typecasting the right-hand side to the left-hand type (ptrdiff_t)
works regardless of the type of MB_CUR_MAX.
src/lib/libcmd/stty.c:
- Silence compiler warning on macOS and FreeBSD, where the
sethostname() definition differs: even in 2026, it still uses an
int as the second (name length) argument instead of size_t. All
other systems seem to have switched to size_t by now.
- Fix a bug: add braces around error()/UNREACHABLE() as they will
usually be conditional upon the preceding 'if', though the
somewhat ridiculous preprocessor stuff obscures this.
src/lib/libcmd/join.c,
src/cmd/ksh93/sh/macro.c:
- Introduce new variables instead of reusing existing ones, so each
one can have an appropriate type. This fixes compiler warnings.
src/lib/libast/features/api:
- Bump API version to 20260907.
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit is a compilation of many old patches of mine. It removes obsolete code and enforces C99 as a build requirement.
Changes:
<sys/types.h>, which had annoying special casing.) This included the "check for global headers" in the iffe script.fchmodis present and use it instead ofchmodin shcomp for very slightly better performance.dotpaths_lib(): Usefstatrather than stat for the PWD for better performance.- Scrap pre-C99 fallbacks foriswblank(3).timeofday()macro with the more portabletvgettime()function in libast, which itself uses the modernclock_gettimeinternally when it's available.strcopywith eitherstrcpyorstpcpyin cases where no buffer overlap occurs. There are perhaps more places where replacement is possible (ksh93v- and ksh2020 do this far more liberally), but I'm trying to be conservative with these changes to avoid accidentally introducing bugs.strncopywithstrlcpyinkeytrap()because the buffers don't overlap.strncpywithstrlcpy(vide ksh2020 commit f63ebd0 and associated issue Replace uses ofstrncpy()withstrlcpy()att/ast#266).str*copy's usage with overlapping buffers.stpcpyfallback.restrict, primarily for standards compliance in compatibility functions (though some regular functions likesfopen()will also use restrict now).__func__is available and drop now unnecessary C90 feature tests.iffeprobes for various C99 multibyte functions (excluding those that still need tests due to platform-specific bugs). Also removed other unnecessary probes for C99 and POSIX functionality.bzero,bcopy,memclearandmemzeroin favor of standardmemsetandmemcpy.getaddrinfoin io.c because that violates the POSIX standard.I have additional changes for using C99-style for loops and bools, but those have been excluded because this patch is big enough as it is. I also encountered crashing bugs when I attempted to remove widespread usage of the struct hack, so the struct hack was only removed if it was already disabled by a preexisting
__STDC_VERSION__check.Progresses #777 (this commit doesn't include or fix the removed
_c99_in_the_wildcode).