lhmouse / mcfgthread

Cornerstone of the MOST efficient std::thread on Windows for mingw-w64
https://gcc-mcf.lhmouse.com/
Other
277 stars 28 forks source link

Clang pre-compiled header causes functions to be not exported from DLL #63

Closed lhmouse closed 2 years ago

lhmouse commented 2 years ago

TL; DR: Disable pre-compiled headers if you are using Clang:

./configure CC=clang --disable-pch

Details

We have a Makefile recipe for pre-compiled headers:

%.xi: %.i %reldir%/version.h config.h
    ${AM_V_CC}${LTCOMPILE} -ffreestanding -x c-header -Wno-error $< -o $@.o  \
      && rm -rf $@.gch  \
      && mkdir $@.gch  \
      && . $@.lo  \
      && (test "$${pic_object}" == none || mv -f %reldir%/$${pic_object} $@.gch/pic)  \
      && (test "$${non_pic_object}" == none || mv -f %reldir%/$${non_pic_object} $@.gch/non_pic)  \
      && rm -f $@.lo  \
      && echo '#error PCH unusable' > $@

By default, libtool generates two types of object files: one for shared libraries and one for static libraries. Because shared ones are built with -DDLL_EXPORT -fPIC -DPIC and static ones are built without such options, it is not feasible to share the same PCH for them. In this recipe we call libtool to compile the header precompiled.xi and generate two 'object files', which are moved into the directory precompiled.xi.gch. GCC is able to tell which .gch file matches the current configuration, and ignore the other.

Unfortunately, it looks like that Clang thinks the static PCH applies to shared builds and uses it to build object files for the DLL. Because the static PCH has been built without DLL_EXPORT, __MCF_DLLEXPORT is defined as empty (instead of the correct __declspec(dllexport)), which causes functions to be not exported.

mstorsjo commented 2 years ago

Sorry, I don't have much experience with PCH, other than I know that Qt uses them (at least in their old qmake build system), and it works for their use. I'm not sure what automatic mechanisms there are for picking up the right PCH from a directory containing more than one, but apparently GCC has that?

For this case - can you construct a small standalone reproducible example of how the PCHs work for you in this case, i.e. test input files and a series of commands that you execute, generating two configurations of PCH from the same source, and then how to invoke the compiler which should use the PCH, where GCC picks the right one but Clang doesn't?

lhmouse commented 2 years ago

For this case - can you construct a small standalone reproducible example of how the PCHs work for you in this case, i.e. test input files and a series of commands that you execute, generating two configurations of PCH from the same source, and then how to invoke the compiler which should use the PCH, where GCC picks the right one but Clang doesn't?

This is fairly easy. Source attached: pch-test-test.tar.gz

Basically we need a little project that uses libtool:

configure.ac

AC_INIT([pch-test], [test])
AC_LANG([C])
AC_CONFIG_SRCDIR([./test.c])
AC_CONFIG_HEADERS([config.h])
AC_CONFIG_AUX_DIR([build-aux])
AC_CONFIG_MACRO_DIR([m4])
AC_PROG_CC

LT_INIT([win32-dll])
AM_INIT_AUTOMAKE([foreign subdir-objects])

AC_CONFIG_FILES([Makefile])
AC_OUTPUT

Makefile.am

ACLOCAL_AMFLAGS = -I m4

lib_LTLIBRARIES = lib/libtest.la
lib_libtest_la_SOURCES = test.c precompiled.i
lib_libtest_la_CFLAGS = ${AM_CFLAGS} -Winvalid-pch -include precompiled.xi
BUILT_SOURCES = precompiled.xi

#### Write a custom recipe for the PCH.
%.xi: %.i config.h
    ${AM_V_CC}${LTCOMPILE} -x c-header -Wno-error $< -o $@.o  \
      && rm -rf $@.gch  \
      && mkdir $@.gch  \
      && . ./$@.lo  \
      && (test "$${pic_object}" == none || mv -f $${pic_object} $@.gch/pic)  \
      && (test "$${non_pic_object}" == none || mv -f $${non_pic_object} $@.gch/non_pic)  \
      && rm -f $@.lo  \
      && echo '#error PCH unusable' > $@

clean-local:
    -rm -rf precompiled.x*

precompiled.i, which is the header to be precompiled

#ifdef DLL_EXPORT
#  define PCH_DLL_EXPORT
#endif

test.c, which uses the precompiled header

/* `DLL_EXPORT` is defined by libtool, and `PCH_DLL_EXPORT` is defined by
 * the PCH. We shall see them both defined or both undefined.  */
#if defined(DLL_EXPORT) != defined(PCH_DLL_EXPORT)
#  error defined(DLL_EXPORT) != defined(PCH_DLL_EXPORT)
#endif

Building this tiny project with clang can immediately reveal the error:

$ autoreconf -i
$ ./configure CC=x86_64-w64-mingw32-clang
$ make
... ...
/bin/sh ./libtool  --tag=CC   --mode=compile x86_64-w64-mingw32-clang -DHAVE_CONFIG_H -I.     -Winvalid-pch -include precompiled.xi -g -O2 -MT lib_libtest_la-test.lo -MD -MP -MF .deps/lib_libtest_la-test.Tpo -c -o lib_libtest_la-test.lo `test -f 'test.c' || echo './'`test.c
libtool: compile:  x86_64-w64-mingw32-clang -DHAVE_CONFIG_H -I. -Winvalid-pch -include precompiled.xi -g -O2 -MT lib_libtest_la-test.lo -MD -MP -MF .deps/lib_libtest_la-test.Tpo -c test.c  -DDLL_EXPORT -DPIC -o .libs/lib_libtest_la-test.o
test.c:4:4: error: defined(DLL_EXPORT) != defined(PCH_DLL_EXPORT)
#  error defined(DLL_EXPORT) != defined(PCH_DLL_EXPORT)
   ^
1 error generated.
make[1]: *** [Makefile:496: lib_libtest_la-test.lo] Error 1
mstorsjo commented 2 years ago

Thanks, I've reproduced it with these input files (even though the attached source package link above doesn't seem to work?).

I also simplified the test sequence to get rid of autotools inbetween, into this:

#!/bin/sh

set -ex

rm -rf precompiled.xi*
mkdir precompiled.xi.gch
$CC -x c-header precompiled.i -o precompiled.xi.gch/non_pic
$CC -x c-header precompiled.i -DDLL_EXPORT -o precompiled.xi.gch/pic
echo '#error PCH unusable' > precompiled.xi
$CC -Winvalid-pch -include precompiled.xi -c test.c
$CC -Winvalid-pch -include precompiled.xi -c test.c -DDLL_EXPORT

And when run with GCC, it prints the -Winvalid-pch warning which shows how it picks between the PCH alternatives:

+ rm -rf precompiled.xi precompiled.xi.gch
+ mkdir precompiled.xi.gch
+ x86_64-w64-mingw32-gcc -x c-header precompiled.i -o precompiled.xi.gch/non_pic
+ x86_64-w64-mingw32-gcc -x c-header precompiled.i -DDLL_EXPORT -o precompiled.xi.gch/pic
+ echo #error PCH unusable
+ x86_64-w64-mingw32-gcc -Winvalid-pch -include precompiled.xi -c test.c
cc1: warning: ./precompiled.xi.gch/pic.exe: not used because `DLL_EXPORT' not defined [-Winvalid-pch]
+ x86_64-w64-mingw32-gcc -Winvalid-pch -include precompiled.xi -c test.c -DDLL_EXPORT
lhmouse commented 2 years ago

The warning about invalid PCH, which is disabled by default, is expected.

The pic file is used when building shared objects and the non_pic file is used when building static objects. As GCC has no knowledge about their contents it just picks one randomly. If one is mistaken (and ignored), the warning is issued, and the next is taken, which should succeed.

mstorsjo commented 2 years ago

This looks very much like a Clang bug in how it validates which PCH files in a directory are usable. See the FIXME comment here: https://github.com/llvm/llvm-project/commit/b63687519610a73dd565be1fec28332211b4df5b#diff-c61a3cce4bfa099b5af032fa83cbf1563f0af4bf58dc112b39571d74b6b681c1R263-R271

https://github.com/mstorsjo/llvm-project/commit/2c3b7d70a736f0743882d8faa6ee982465bfed5f looks like it could be a valid fix for the issue, but I've yet to make a testcase for it and see if it breaks some other existing testcase somehow.

mstorsjo commented 2 years ago

I forgot to clarify the impact of the bug; clang does distinguish between -DFOO=YES and -DFOO=NO when choosing which PCH file to pick. But it doesn’t skip a file if a define is set in one of them (command line or PCH) but not the other.

lhmouse commented 2 years ago

Thanks. Your work shall be appreciated. I am looking forward to the next LLVM release.

mstorsjo commented 2 years ago

I posted the fix for review at https://reviews.llvm.org/D126676. However, it does unfortunately seem like the fact that Clang isn't strict about such mismatches between defines when creating and using the PCH is an intentional behaviour - see https://github.com/llvm/llvm-project/commit/c379c072405f39bca1d3552408fc0427328e8b6d and https://github.com/llvm/llvm-project/commit/b63687519610a73dd565be1fec28332211b4df5b#diff-c61a3cce4bfa099b5af032fa83cbf1563f0af4bf58dc112b39571d74b6b681c1R267-R269, so there's a risk that this change is controversial (it breaks 84 testcases in Clang as things stand right now).

While looking into this, I did notice that the issue disappears if using -UDLL_EXPORT on the command line instead of leaving it out (for the static library version), but I guess it's not straightforward to make libtool pass that.

lhmouse commented 2 years ago

The current behavior really seems an oversight, probably because few people have been using libtool with PCHs so far. Without libtool, if both static and shared builds are desired, one may have to create separate build directories, where there won't be such problems.

lhmouse commented 2 years ago

Thanks for this work. This can be closed for now.