Page MenuHomeFreeBSD

Fix compilation with upstream clang builtin headers
ClosedPublic

Authored by arichardson on Sep 3 2018, 11:59 AM.

Details

Summary

By using -nobuiltininc and adding the clang builtin headers resource dir to
the end of the compiler header search path, we can still find headers such
as immintrin.h but find the FreeBSD version of stddef.h/stdarg.h/.. first.

This currently only works if the -print-resource-dir flag is supported by
clang which was introduced with clang 5.0.

Test Plan

build worked for me with upstream clang. Would be nice to fix
clang/FreeBSD headers to be compatible instead, but for now this works.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

arichardson created this revision.Sep 3 2018, 11:59 AM

Fix inverted condition

Remove debug .info

arichardson updated this revision to Diff 47615.Sep 3 2018, 1:01 PM

Fix typo in path

jhb added a comment.Sep 13 2018, 6:51 PM

Do we know what incompatibilities there are currently? For external GCC we are always using the compiler-provided headers and then fixing any issues we run into in the code base.

bdrewery requested changes to this revision.Sep 13 2018, 8:13 PM
bdrewery added inline comments.
share/mk/bsd.compiler.mk
202–204 ↗(On Diff #47615)

Please also add COMPILER_RESOURCE_DIR to the _TOOLCHAIN_METADATA_VARS list in Makefile.inc1 or this lookup will execute in parts of the build where it should already be known/cached.

This revision now requires changes to proceed.Sep 13 2018, 8:13 PM
In D17002#365778, @jhb wrote:

Do we know what incompatibilities there are currently? For external GCC we are always using the compiler-provided headers and then fixing any issues we run into in the code base.

There were a few issues last time I tried:

  • stddef.h: Clang uses _WCHAR_T instead of _WCHAR_T_DECLARED (and similar for size_t and ptrdiff_t, etc.) so we get typedef redefinitions because the clang builtin headers are parsed before the ones in $WORLDTMP.
  • stdarg.h: uses _VA_LIST instead of _VA_LIST_DECLARED so some headers that check for the define instead of including stdarg.h break.
  • stdint.h: uses macro redefinitions without #undef before, but for some reason is not treated as a system header so I got -Werror failures.
  • some other errors where I don't remember the details.
  • stdint.h: uses macro redefinitions without #undef before, but for some reason is not treated as a system header so I got -Werror failures.

Turns out we are compiling some files with -Wsystem-headers so that explains the errors. I can submit a patch to clang to fix those warnings but until then something like this will be needed.

We might even be able to fix this without parsing the compiler resource dir. If we add -isystem ${WORLDTMP}/usr/include it should come before the builtins instead after in the search path.
I'll try that approach since it will avoid another fork() + more complexity in bsd.compiler.mk.

  • stdint.h: uses macro redefinitions without #undef before, but for some reason is not treated as a system header so I got -Werror failures.

Turns out we are compiling some files with -Wsystem-headers so that explains the errors. I can submit a patch to clang to fix those warnings but until then something like this will be needed.

We might even be able to fix this without parsing the compiler resource dir. If we add -isystem ${WORLDTMP}/usr/include it should come before the builtins instead after in the search path.
I'll try that approach since it will avoid another fork() + more complexity in bsd.compiler.mk.

I just tried this approach and it works for C but breaks all C++ files:

.if ${MK_CLANG_BOOTSTRAP} == "no" && !defined(BOOTSTRAPPING) && !empty(SYSROOT)
CFLAGS+= -isystem ${SYSROOT}/usr/include
.endif

This changes the #include search to

#include <...> search starts here:
 /exports/users/alr48/sources/cheribsd/contrib/cheri-libunwind/include
 /exports/users/alr48/sources/cheribsd/lib/libgcc_eh
 /home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include
 /home/alr48/cheri/output/sdk/lib/clang/7.0.0/include

This means stddef.h is now taken from FreeBSD instead of the clang headers and everything compiles fine.

However, when compiling a C++ file we get the following:

#include <...> search starts here:
 /exports/users/alr48/sources/cheribsd/contrib/cheri-libunwind/include
 /exports/users/alr48/sources/cheribsd/lib/libgcc_eh
 /home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include
 /home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include/c++/v1
 /home/alr48/cheri/output/sdk/lib/clang/7.0.0/include
End of search list.
In file included from /exports/users/alr48/sources/cheribsd/contrib/cheri-libunwind/src/libunwind.cpp:18:
In file included from /home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include/c++/v1/new:89:
In file included from /home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include/c++/v1/exception:81:
In file included from /home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include/c++/v1/cstddef:44:
/home/alr48/cheri/output/sdk/lib/clang/7.0.0/include/stddef.h:124:9: error: 'offsetof' macro redefined [-Werror,-Wmacro-redefined]
#define offsetof(t, d) __builtin_offsetof(t, d)
        ^
/home/alr48/obj/build/cheribsd-obj-128/exports/users/alr48/sources/cheribsd/mips.mips64/tmp/usr/include/stddef.h:75:9: note: previous definition is here
#define offsetof(type, field)   __offsetof(type, field)
        ^
1 error generated.

This is because cstddef uses #include_next and then finds stddef.h in the clang builtin includes directory.
I'll see if I can get this to work without using -idirafter for the resource dir, but it will probably be even more hacky.

arichardson updated this revision to Diff 48933.Oct 9 2018, 2:34 PM

Since I can't come up with a better solution update this patch

  • Add to TOOLCHAIN_METADATA_VARS
  • Fix typo in comment
brooks accepted this revision.Nov 1 2018, 10:44 PM

@bdrewery is this okay to commit now?

What case does this fix exactly?

Also is there any chance that SYSTEM_COMPILER is a problem here? Does /usr/bin/cc need to have a certain directory path being used that matches the bootstrap compiler? Said another way, are we planning to change the directory used here such that /usr/bin/cc may do the wrong thing? SYSTEM_COMPILER depends on /usr/bin/cc matching the behavior of what is to be built as the bootstrap compiler. --sysroot is always added to accomplish that.

This change is required to build with an unmodified upstream LLVM since the default upstream clang builtin headers are incompatible with the ones in /usr/bin/include (see my earlier comments in this review).

--sysroot only affects the normal include paths, but does not change the search path for compiler-provided headers.
SYSTEM_COMPILER compiler should not make any difference here and will work as expected with /usr/bin/cc:

/usr/bin/cc -print-resource-dir
/usr/lib/clang/6.0.1

/usr/lib/clang/6.0.1/include contains all the necessary compiler-builtin headers (but we don't install the stddef.h/etc. headers that are incompatible)

This change is required to build with an unmodified upstream LLVM since the default upstream clang builtin headers are incompatible with the ones in /usr/bin/include (see my earlier comments in this review).

--sysroot only affects the normal include paths, but does not change the search path for compiler-provided headers.
SYSTEM_COMPILER compiler should not make any difference here and will work as expected with /usr/bin/cc:

/usr/bin/cc -print-resource-dir
/usr/lib/clang/6.0.1

/usr/lib/clang/6.0.1/include contains all the necessary compiler-builtin headers (but we don't install the stddef.h/etc. headers that are incompatible)

All the devel/llvm* ports work around this issue by not installing a subset of headers (requiring a patch to a somewhat frequently changing file). It would be nice to not have to do that and this change allows us to avoid needing to make that change (once it's merged to all supported releases).

jhb added a comment.EditedNov 15 2019, 9:51 PM

Does this work with GCC? Seems no:

> mips-unknown-freebsd12.1-gcc -print-resource-dir
mips-unknown-freebsd12.1-gcc: error: unrecognized command line option '-print-resource-dir'; did you mean '-print-search-dirs'?
mips-unknown-freebsd12.1-gcc: fatal error: no input files
compilation terminated.
arichardson added a comment.EditedNov 15 2019, 9:53 PM
In D17002#489675, @jhb wrote:

Does this work with GCC? Seems no:

> mips-unknown-freebsd12.1-gcc -print-resource-dir
mips-unknown-freebsd12.1-gcc: error: unrecognized command line option '-print-resource-dir'; did you mean '-print-search-dirs'?
mips-unknown-freebsd12.1-gcc: fatal error: no input files
compilation terminated.

That should be fine since the GCC provided stddef.h supports every macro ever defined by any stddef.h implementation so we don't need to skip it.
The ${COMPILER_RESOURCE_DIR} != "unknown" check should ensure that nothing changes for GCC

https://github.com/gcc-mirror/gcc/blob/master/gcc/ginclude/stddef.h#L243

Ping? Can we merge this or do I have to convince upstream LLVM to add checks for the FreeBSD _FOO_DECLARED macros to their stddef.h/stdarg.h? I'm not sure how willing they would be to make that change though.

swills added a subscriber: swills.Feb 29 2020, 10:51 PM
dim added a comment.Mar 3 2020, 6:09 PM

Ping? Can we merge this or do I have to convince upstream LLVM to add checks for the FreeBSD _FOO_DECLARED macros to their stddef.h/stdarg.h? I'm not sure how willing they would be to make that change though.

This should be solved upstream, once and for all. There is a reason we don't install those headers, the port shouldn't install them either.

In D17002#526349, @dim wrote:

Ping? Can we merge this or do I have to convince upstream LLVM to add checks for the FreeBSD _FOO_DECLARED macros to their stddef.h/stdarg.h? I'm not sure how willing they would be to make that change though.

This should be solved upstream, once and for all. There is a reason we don't install those headers, the port shouldn't install them either.

I think they should be installed. It allows e.g. building binaries for a bare metal target. Fixing it in upstream llvm would be ideal but we could also change the FreeBSD headers to check for the llvm macros?

This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2020, 11:38 PM
This revision was automatically updated to reflect the committed changes.