Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ksh fails to compile with tcc #232

Closed
JohnoKing opened this issue Mar 19, 2021 · 18 comments
Closed

ksh fails to compile with tcc #232

JohnoKing opened this issue Mar 19, 2021 · 18 comments
Labels
buildfail Does not compile

Comments

@JohnoKing
Copy link

This bug was reported to me by Chase (one of the CDE contributers) via email:

Secondly, I was wondering if you maybe had any insight into why ksh fails to compile with tcc on linux. First, when running CC=tcc ./bin/package make it says it doesn't like the preprocessor macro #define onexit on_exit in src/lib/libast/comp/atexit.c. When that is fixed (I made an ugly fix by substituting the underscore for a preprocessor macro UNDERSCORE and tcc's cpp seemed to like that), iffe fails saying that tcc -c doesn't accept libraries. Since it is only a compiler check testing if int i=1 compiles, I simply took out the -lm the Mamfile was asking for. The last problem is the one that stumps me, pty fails after this and give no helpful debugging output.

I've attempted to fix this bug in the patch below, with limited success. It only fixes the compile on Linux and requires a relatively recent version of tcc to build (tcc commit dd60b20c or later). Even with the patch below tcc fails to compile ksh on FreeBSD and it breaks pty on illumos:

diff --git a/src/cmd/INIT/iffe.sh b/src/cmd/INIT/iffe.sh
index 9c0426e5..3647bc43 100644
--- a/src/cmd/INIT/iffe.sh
+++ b/src/cmd/INIT/iffe.sh
@@ -31,10 +31,22 @@ AIX)	unset LIBPATH ;;
 esac
 
 command=iffe
-version=2021-02-03 # update in USAGE too #
+version=2021-03-19 # update in USAGE too #
 
 compile() # $cc ...
 {
+	# tcc can't combine -l* and -c
+	if echo "$@" | grep ' -c' | grep -q ' -l'
+	then
+		for arg
+		do	shift
+			case $arg in
+			-l*) : ;;
+			*) set -- "$@" "$arg"
+			esac
+		done
+	fi
+
 	"$@" 2>$tmp.err
 	_compile_status=$?
 	if	test -s $tmp.err
@@ -753,7 +765,7 @@ set=
 case `(getopts '[-][123:xyz]' opt --xyz; echo 0$opt) 2>/dev/null` in
 0123)	USAGE=$'
 [-?
-@(#)$Id: iffe (ksh 93u+m) 2021-02-03 $
+@(#)$Id: iffe (ksh 93u+m) 2021-03-19 $
 ]
 [-author?Glenn Fowler <[email protected]>]
 [-author?Phong Vo <[email protected]>]
diff --git a/src/lib/libast/features/lib b/src/lib/libast/features/lib
index 5a9a67b2..de9410b1 100644
--- a/src/lib/libast/features/lib
+++ b/src/lib/libast/features/lib
@@ -28,7 +28,7 @@ lib	getopt,getsubopt,getopt_long,getopt_long_only
 lib	glob,index,iswblank,iswctype,killpg,link,localeconv,madvise
 lib	mbtowc,mbrtowc,memalign,memchr,memcpy,memdup,memmove,memset
 lib	mkdir,mkfifo,mktemp,mktime
-lib	mount,on_exit,onexit,opendir,pathconf
+lib	mount,onexit,opendir,pathconf
 lib	readlink,remove,rename,rewinddir,rindex,rmdir,setlocale
 lib	setpgid,setpgrp,setpgrp2,setreuid,setsid,setuid,sigaction
 lib	sigprocmask,sigsetmask,sigunblock,sigvec,socketpair
@@ -56,6 +56,21 @@ sys	socket,stream,systeminfo,universe,vfork
 typ	ino64_t,off64_t
 typ	struct.dirent64 dirent.h
 
+tst note{ on_exit works and accepts one argument }end output{
+	#include <stdlib.h>
+
+	static void success(void)
+	{
+		printf("#define _lib_on_exit\t1\n");
+	}
+
+	int main(void)
+	{
+		on_exit(success);
+		exit(0);
+	}
+}end
+
 tst	tst_errno note{ errno can be assigned }end link{
 	_BEGIN_EXTERNS_
 	#define error		______error
@McDutchie McDutchie added the buildfail Does not compile label Mar 19, 2021
@McDutchie
Copy link

It would be nice if this worked, but I don't see it as a very high priority. As far as I know there are no systems that have tcc as their standard compiler. And isn't tcc designed more for quickly compiling and running small programs on the fly?

@avih
Copy link

avih commented Mar 21, 2021

isn't tcc designed more for quickly compiling and running small programs on the fly?

It can do that, but its goal is to be a small and relatively simple c99 compliant compiler, currently also with some c11 features.

It can build big projects, with varying degrees of efforts (some of those to work around gnu-ism), like the linux kernel, and apparently ksh is not too far off either.

@JohnoKing
Copy link
Author

JohnoKing commented Mar 21, 2021

With the current patch the latest version of tcc fails to build ksh on FreeBSD, producing the following errors:

tcc: error: undefined symbol '__builtin_isgreater'
tcc: error: undefined symbol '__builtin_isgreaterequal'
tcc: error: undefined symbol '__builtin_isless'
tcc: error: undefined symbol '__builtin_islessequal'
tcc: error: undefined symbol '__builtin_islessgreater'
tcc: error: undefined symbol '__builtin_isunordered'

As it turns out tcc pretends to be GCC on FreeBSD (on Linux it doesn't). This causes the FreeBSD math.h header to define macros that call the GCC/Clang builtin versions of math functions. Since tcc doesn't have those builtins, the compile fails:
https://github.com/freebsd/freebsd-src/blob/098dbd7ff7f3da9dda03802cdb2d8755f816eada/lib/msun/src/math.h#L41-L43
https://github.com/freebsd/freebsd-src/blob/098dbd7ff7f3da9dda03802cdb2d8755f816eada/lib/msun/src/math.h#L117-L132

I'm not sure if this can be cleanly fixed. The best way would be to have tcc avoid trying to imitate GCC on FreeBSD, although I'm not sure why tcc imitates GCC on FreeBSD and not Linux.

JohnoKing added a commit to JohnoKing/ksh that referenced this issue Mar 21, 2021
This allows ksh to be compiled with versions of tcc that define
__dso_handle in libtcc1.a[*], although only on Linux. The two
problems that remain are the following:

1) Older versions of tcc still fail to compile ksh, although now
   they fail after reaching the libdll feature test. I'm not sure
   if fixing that is feasible since even if I hack out the failing
   libdll feature test, ksh fails to link with a '__dso_handle'
   error.
2) tcc fails to build ksh on FreeBSD. For some reason tcc imitates
   GCC on FreeBSD but not on Linux. As a result the FreeBSD math.h
   header tries to define macros that call GCC/Clang builtins.
   This results in undefined symbol errors since tcc doesn't have
   those builtins.

src/cmd/INIT/iffe.sh: compile():
- tcc forbids combining the -c compiler flag with -l* linker flags.
  Strip all -l* flags when -c was passed to the compiler. This is
  only done when tcc is the compiler to prevent build failures on
  some platforms (such as illumos).

src/lib/libast/comp/atexit.c,
src/lib/libast/features/lib,
src/lib/libast/vmalloc/vmexit.c:
- From what I've been able to gather the only OSes with support
  for on_exit are Linux and SunOS 4. However, on_exit takes two
  arguments, so the macro that defines it as taking one argument
  is incorrect. Since Solaris (SunOS 5) no longer has this call
  and the macro breaks on Linux, the clean fix is to remove it
  (atexit(3) is used instead).

[*]: https://repo.or.cz/tinycc.git/commit/dd60b20c6e0d01df8e332b0234125abaa964d324

Progresses: ksh93#232
@JohnoKing
Copy link
Author

On *BSD systems tcc explicitly defines __GNUC__ and other GCC macros:
https://repo.or.cz/tinycc.git/blob/82b0af74501bf46b16bc2a4a9bd54239aa7b7127:/include/tccdefs.h#l92

Disabling these macros with -U* options doesn't work (it causes the compile to fail earlier with an error in sys/types.h) , so that's out of the question.

@McDutchie
Copy link

Does tcc define its own macro, something like __TCC__? If so, you could try adding something like && !__TCC__ to the gcc ifdefs that get in the way.

@JohnoKing
Copy link
Author

I tried that before, but it only seems to work when applied to /usr/include/math.h. On FreeBSD 12.2 the following diff works around the build failure and allows tcc to build ksh:

--- a/usr/include/math.h
+++ b/usr/include/math-copy.h
@@ -38,7 +38,7 @@ extern const union __nan_un {
 #define	__MATH_BUILTIN_CONSTANTS
 #endif
 
-#if __GNUC_PREREQ__(3, 0) && !defined(__INTEL_COMPILER)
+#if __GNUC_PREREQ__(3, 0) && !defined(__INTEL_COMPILER) && !defined(__TINYC__)
 #define	__MATH_BUILTIN_RELOPS
 #endif
 

@McDutchie
Copy link

Got tcc working on FreeBSD. That took some doing. The ports package does nothing but crash, so I got the latest git code. It requires gcc and gmake to compile. :-/

Anyway, I tried simply undefining __GNUC__ in ast.h. The reason why FreeBSD pretends to be gcc on FreeBSD quickly became clear: the system headers break if it doesn't. Got an obscure syntax error in ctype.h that is probably the result of some botched macro definition.

But if I'm reading your diff above right, it looks like those math builtins are used on gcc 3.0 or later. So that made me think of the following...

--- a/src/lib/libast/include/ast.h
+++ b/src/lib/libast/include/ast.h
@@ -78,6 +78,19 @@ struct _sfio_s;
 #endif
 #endif
 
+/*
+ * tcc on FreeBSD: Avoid using nonexistent math
+ * builtins by pretending to be an ancient gcc.
+ */
+#if __TINYC__ && __GNUC__ >= 3 && __FreeBSD__
+#undef __GNUC__
+#undef __GNUC_MINOR__
+#undef __GNUC_PATCHLEVEL__
+#define __GNUC__ 2
+#define __GNUC_MINOR__ 95
+#define __GNUC_PATCHLEVEL__ 3
+#endif
+
 /*
  * exit() support -- this matches shell exit codes
  */

And presto, when applying this along with your PR, it now compiles on FreeBSD. No regression test failures.

@McDutchie
Copy link

McDutchie commented Mar 23, 2021

Here is a better fix for the illumos build failure to replace the one that broke tcc. The -lm flag can be added to the two tests themselves instead of to the iffe invocation in the Mamfile. With this, iffe does not need the hack to remove -l flags on tcc.

--- a/src/cmd/builtin/Mamfile
+++ b/src/cmd/builtin/Mamfile
@@ -37,7 +37,7 @@ make install
 					meta FEATURE/pty features/%>FEATURE/% features/pty pty
 					make features/pty
 					done features/pty
-					exec - iffe ${IFFEFLAGS} -v -c "${CC} ${mam_cc_FLAGS} ${KSH_RELFLAGS} ${CCFLAGS} ${LDFLAGS} -lm" ref ${mam_cc_L+-L${INSTALLROOT}/lib} -I${PACKAGE_ast_INCLUDE} -I${INSTALLROOT}/include ${mam_libast} ${mam_libcmd} : run features/pty
+					exec - iffe ${IFFEFLAGS} -v -c "${CC} ${mam_cc_FLAGS} ${KSH_RELFLAGS} ${CCFLAGS} ${LDFLAGS}" ref ${mam_cc_L+-L${INSTALLROOT}/lib} -I${PACKAGE_ast_INCLUDE} -I${INSTALLROOT}/include ${mam_libast} ${mam_libcmd} : run features/pty
 				done FEATURE/pty generated
 				make ${PACKAGE_ast_INCLUDE}/ast_time.h implicit
 				done ${PACKAGE_ast_INCLUDE}/ast_time.h
diff --git a/src/cmd/builtin/features/pty b/src/cmd/builtin/features/pty
index 612609b..ab4a041 100755
--- a/src/cmd/builtin/features/pty
+++ b/src/cmd/builtin/features/pty
@@ -13,7 +13,7 @@ lib	openpty,_getpty,ptsname -lutil
 lib	grantpt,unlockpt,posix_openpt stdlib.h
 lib	cfmakeraw termios.h
 
-tst - output{
+tst - -lm output{
 	#include	<fcntl.h>
 	#if _lib_ptsname
 	#include	<stdlib.h>
diff --git a/src/lib/libdll/Mamfile b/src/lib/libdll/Mamfile
index 0ef420e..29d9ddb 100644
--- a/src/lib/libdll/Mamfile
+++ b/src/lib/libdll/Mamfile
@@ -154,7 +154,7 @@ make install
 							done features/dll
 							bind -ldl dontcare
 							bind -last
-							exec - iffe ${IFFEFLAGS} -v -c "${CC} ${mam_cc_FLAGS} ${KSH_RELFLAGS} ${CCFLAGS} ${LDFLAGS} -lm" ref ${mam_cc_L+-L.} ${mam_cc_L+-L${INSTALLROOT}/lib} -I${PACKAGE_ast_INCLUDE} -I${INSTALLROOT}/include ${mam_libdl} ${mam_libast} : run features/dll
+							exec - iffe ${IFFEFLAGS} -v -c "${CC} ${mam_cc_FLAGS} ${KSH_RELFLAGS} ${CCFLAGS} ${LDFLAGS}" ref ${mam_cc_L+-L.} ${mam_cc_L+-L${INSTALLROOT}/lib} -I${PACKAGE_ast_INCLUDE} -I${INSTALLROOT}/include ${mam_libdl} ${mam_libast} : run features/dll
 						done FEATURE/dll generated
 						exec - cmp 2>/dev/null -s FEATURE/dll dlldefs.h || { rm -f dlldefs.h; silent test -d . || mkdir .; cp FEATURE/dll dlldefs.h; }
 					done dlldefs.h generated
diff --git a/src/lib/libdll/features/dll b/src/lib/libdll/features/dll
index e5d5790..86163b4 100644
--- a/src/lib/libdll/features/dll
+++ b/src/lib/libdll/features/dll
@@ -74,7 +74,7 @@ tst	run{
 	esac
 	echo "#define _DLL_NEXT_PATH	\"$lib\""
 }end
-tst	- output{
+tst	- -lm output{
 	#include <math.h>
 	#if defined(__MVS__) && !defined(__SUSV3)
 	#define __SUSV3		1

@JohnoKing
Copy link
Author

JohnoKing commented Mar 23, 2021

Now the only problem is getting versions of tcc older than dd60b20c to compile ksh (if feasible). I've found that ksh93q 2005-02-02 can be compiled on Linux with the "stable" version of tcc (version 0.9.27) using a minimal patch that removes usage of on_exit:

--- a/src/lib/libast/comp/atexit.c
+++ b/src/lib/libast/comp/atexit.c
@@ -34,6 +34,8 @@ NoN(atexit)
 
 #else
 
+#undef _lib_on_exit
+
 #if _lib_onexit || _lib_on_exit
 
 #if !_lib_onexit
# The fmtdev change is just to fix a macro related problem with glibc
--- a/src/lib/libast/string/fmtdev.c
+++ b/src/lib/libast/string/fmtdev.c
@@ -30,6 +30,9 @@
 #include <ast.h>
 #include <ctype.h>
 #include <ls.h>
+#ifdef __linux__
+#include <sys/sysmacros.h>
+#endif
 
 char*
 fmtdev(struct stat* st)

The libdll feature test doesn't start failing until ksh93r 2006-01-24. Importing the ksh93q version of iffe fixes the libdll failure (after changing one inc in src/lib/libast/features/tmx to include), so the bug that introduced the build failure could be identified in ksh93r's version of iffe.

@McDutchie
Copy link

McDutchie commented Mar 23, 2021

Here is the result of a 20-iteration shbench test on my FreeBSD 12.2 VPS on ksh 93u+m versions compiled with all three compilers (clang 10.0.1, gcc 9.3.0, tcc latest git) with flags -Os -g -D_std_malloc. The speed differences are all over the place, and much more radical than I expected.

---------------------------------------------------------------------------------
name             ./ksh.clang          ./ksh.gcc               ./ksh.tcc
---------------------------------------------------------------------------------
braces.ksh       2.188 [2.119-2.371]  0.900 [0.705-0.952]     2.081 [2.004-2.331]
charsplit.ksh    2.443 [2.310-2.762]  1.389 [1.290-1.535]     2.420 [2.302-2.567]
extglob.ksh      1.458 [1.349-1.627]  0.971 [0.955-1.024]     1.469 [1.356-1.594]
fibonacci.ksh    2.066 [2.013-2.279]  0.898 [0.697-0.956]     2.077 [2.003-2.270]
gsub.ksh         2.453 [2.315-2.598]  6.885 [6.697-7.028]     2.414 [2.300-2.565]
hanoi.ksh        1.189 [1.125-1.373]  0.656 [0.508-0.753]     1.217 [1.148-1.554]
numfor.ksh       3.194 [3.097-3.659]  1.312 [1.234-1.472]     3.191 [3.094-3.389]
numloop.ksh      2.751 [2.559-2.936]  1.046 [1.004-1.238]     2.743 [2.557-2.948]
parens.ksh       1.479 [1.347-1.623]  0.806 [0.642-0.866]     1.425 [1.306-1.763]
piln2.ksh        5.904 [5.664-5.996]  2.328 [2.211-2.534]     5.999 [5.877-6.168]
plus-equals.ksh  3.857 [3.636-4.032]  19.312 [19.011-19.742]  3.894 [3.641-4.124]
printf.ksh       1.579 [1.421-1.714]  1.017 [0.999-1.064]     1.583 [1.408-1.948]
qsort.ksh        3.558 [3.379-3.748]  2.292 [2.192-2.462]     3.553 [3.364-3.699]
rand.ksh         5.005 [4.555-5.699]  1.906 [1.641-2.748]     4.680 [4.502-4.856]
rand2.ksh        3.094 [3.019-3.317]  1.361 [1.264-1.535]     3.088 [2.980-3.300]
sample.ksh       4.537 [4.357-4.842]  2.228 [2.150-2.448]     4.543 [4.323-4.777]
shuffle.ksh      4.386 [4.234-4.782]  1.853 [1.646-1.950]     4.323 [4.177-4.506]
sieve.ksh        4.188 [4.067-4.453]  1.593 [1.426-1.737]     4.161 [4.045-4.848]
---------------------------------------------------------------------------------

@JohnoKing
Copy link
Author

The shbench results are interesting, but the main reason to use tcc is for how fast it compiles (which allows patches to be tested quickly). These are the results I get from a timed bin/package make on Linux (CCFLAGS='-Os -D_std_malloc', the time keyword also has added microsecond precision):

tcc-git:
real	0m35.921415s
user	0m21.270799s
sys	0m10.041324s

gcc 10.2.0:
real	2m05.242312s
user	1m38.251274s
sys	0m21.462510s

clang 11.1.0:
real	3m20.491931s
user	2m35.355988s
sys	0m39.498149s

@avih
Copy link

avih commented Mar 23, 2021

Now the only problem is getting versions of tcc older than dd60b20c to compile ksh

If this tcc commit introduces some real issue, then you should report it to the tcc mailing list.

The speed differences are all over the place

Dunno, clang and tcc seems about the same (not highly surprising considering no speed optimizations are enabled), and gcc is somewhat faster usually, but not so much different than the others.

To me this indicates more than anything that these bench scripts are bound by things other than pure code execution. However, I didn't look at what these bench scripts do, so it is possible that they test pure execution performance (e.g. if there are not many subshells and IO).

EDIT - looking at the clang/tcc numbers again, are you sure you didn't accidentally use the same binary? they're really close to eachother - more than I'd think they would be.

@avih
Copy link

avih commented Mar 23, 2021

These are the results I get from a timed bin/package make on Linux ...

I presume the build is not parallel? otherwise tcc is usually about 10x faster than gcc, while your results are less than 4x.

@JohnoKing
Copy link
Author

If this tcc commit introduces some real issue, then you should report it to the tcc mailing list.

The current problem is that versions of tcc older than the referenced commit can't compile ksh because they don't provide the __dso_handle symbol (commit dd60b20c is the one that adds __dso_handle to tcc). The latest release of tcc (version 0.9.27) does not provide __dso_handle because it's older than that commit. Currently, ksh needs __dso_handle to compile; fixing the build failure with older versions of tcc will require ksh to build without that symbol or provide __dso_handle itself.

I presume the build is not parallel? otherwise tcc is usually about 10x faster than gcc, while your results are less than 4x.

The current mamake build system doesn't have parallel execution. To implement that, the current build system would need to be switched out with a different one with parallelization (which isn't going to happen anytime soon).

@avih
Copy link

avih commented Mar 23, 2021

Right, I misunderstood then. I thought you meant one needs a tcc version older than dd60b20c in order to build ksh. Thanks.

fixing the build failure with older versions of tcc will require ...

Not worth it IMHO. 0.9.27 is fairly old now, and the next version will probably be released at some near-ish stage. There has been a LOT of progress in tcc since 0.9.27.

@McDutchie
Copy link

I personally tend to agree that targeting tcc 0.9.27 and earlier is probably not worth the effort. There have been a lot of changes in iffe between 2006-01-24 and the previous version in the ksh multishell repo, so finding a workaround would be a challenge. I'm happy to merge this as it is now. @JohnoKing, how do you feel about that?

@JohnoKing
Copy link
Author

I have to agree that finding a workaround isn't going to be worthwhile (errors related to __dso_handle can be quite difficult to fix). Since the latest version of tcc is now compiling ksh on Linux and FreeBSD without issues, I'm fine with merging the current set of fixes.

@avih
Copy link

avih commented Mar 23, 2021

The current mamake build system doesn't have parallel execution. To implement that, the current build system would need to be switched out with a different one with parallelization

In this case, the smaller-than-expected speedup while building with tcc might also be attributed to inefficiencies of the build system. I've seen it elsewhere too, where using tcc starts exposing bottlenecks of the build system (specifically, waf, while building mpv).

Not suggesting it should be improved though. Just a data point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildfail Does not compile
Projects
None yet
Development

No branches or pull requests

3 participants