Page MenuHomeFreeBSD

Allow bootstrapping llvm-tblgen on macOS and Linux
ClosedPublic

Authored by arichardson on Jul 5 2021, 5:17 PM.

Details

Summary

This is needed in order to build various LLVM binutils (e.g. addr2line)
as well as clang/lld/lldb.

Co-authored-by: Jessica Clarke <jrtc27@FreeBSD.org>

Test Plan

compiles on ubuntu 18.04 (and worked on macOS when I last tried)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Errors
SeverityLocationCodeMessage
Errortools/build/make.py:122E302PEP8 E302
Errortools/build/make.py:132E302PEP8 E302
Errortools/build/make.py:259E501PEP8 E501
Unit
No Unit Test Coverage
Build Status
Buildable 40543
Build 37432: arc lint + arc unit

Event Timeline

lib/libc/db/db/db.c
58 ↗(On Diff #91810)

This change is needed to make cross-build/include/linux/fcntl.h compatible with LLVM since that uses ::open(...) which doesn't work with the statement macro.
Can also split this into a separate commit?

https://reviews.freebsd.org/P512 is a more complete version I wrote, though presumably will fail on Linux currently due to the sysctl and fcntl changes you had to make. __has_include is probably a nicer way to do it than I did, too; I just compared FreeBSD, macOS and Linux config.h files.

https://reviews.freebsd.org/P512 is a more complete version I wrote, though presumably will fail on Linux currently due to the sysctl and fcntl changes you had to make. __has_include is probably a nicer way to do it than I did, too; I just compared FreeBSD, macOS and Linux config.h files.

Ah nice, I didn't think about adding LDFLAGS+= -Wl,-dead_strip. Will try to merge your changes into this.

merge @jrtc27's changes to the config file (I didn't include the LLDB changes since we never bootstrap it)

Looks OK to me but will defer to @dim for questions about maintainability / expected impact on future clang/llvm updates

merge @jrtc27's changes to the config file (I didn't include the LLDB changes since we never bootstrap it)

Yeah, the lldb and lldb-server change aren't needed as those aren't bootstrap tools, but what about lld?

dim requested changes to this revision.Jul 6 2021, 7:27 PM

Looks OK to me but will defer to @dim for questions about maintainability / expected impact on future clang/llvm updates

I'm not too troubled by the various hacks in the config.h files, as we already have a few of those. On the other hand, if the ifdef maze becomes too large, then it might start to make sense to have different pregenerated .h files for different OS/arch combinations.

That said, all the other stuff with sysctl.h and fcntl.h seems very ugly to me, and I'd rather leave it out if at all possible. Or at least, separate it out to a separate commit.

lib/clang/libllvmminimal/Makefile
84 ↗(On Diff #91879)

I think this is all quite ugly, why don't we solve this at another level? I.e for the bootstrap tools phase, just don't use the tools/build/cross-build/include/common/sys/sysctl.h file at all?

lib/libc/db/db/db.c
58 ↗(On Diff #91810)

Isn't it easier to work around this problem by defining O_EXLOCK and friends to 0 in the compat fcntl.h instead? Then you could leave this file alone.

tools/build/cross-build/include/common/sys/sysctl.h
40

As mentioned above, I'd rather solve this on a different level, but maybe it's not that easy...

usr.bin/clang/clang.prog.mk
23

Hm, we can't do LIBADD += ncurses here? Or do we miss a definition in bsd.libnames.mk (or whatever it's called)?

This revision now requires changes to proceed.Jul 6 2021, 7:27 PM
usr.bin/clang/clang.prog.mk
23

ncurses has a build-tools target so can't be built early enough to provide a bootstrap ncurses for the bootstrap-tools tblgen

lib/libc/db/db/db.c
58 ↗(On Diff #91810)

Yes it would be, but then we get silent success without a lock if one of the bootstrap tools uses these flags. I think not defining it in that case is safer.

tools/build/cross-build/include/common/sys/sysctl.h
40

In general we want to make sure that bootstrap tools don't use sysctl/sysctlbyname() since they generally expect FreeBSD sysctl and then might not work as expected. However, in the case of LLVM we do need the sysctls() on macOS so we need some way of opting into using the real sysctl definitions.

usr.bin/clang/clang.prog.mk
23

It might be possible to build the bootstrap tools without linking ncurses. Will look into that.

usr.bin/clang/clang.prog.mk
23

That's what my patch did, worked just fine on macOS. On Linux I made it use -ltinfo since that's what LLVM's CMakeLists.txt auto-detects, though you might be able to get away without that too, I didn't try.

lib/clang/include/llvm/Config/config.h
252–253

This is why ncurses is not needed for macos but is on FreeBSD/Linux. Will upload a new patch that avoids terminfo for bootstrap tools.

More complete version. Now builds Clang+LLD successfully on Ubuntu and llvm-tblgen on macOS (clang probably works but I don't want to overheat my laptop)

lib/clang/include/llvm/Config/config.h
246

I think we should still enable it on FreeBSD, it works fine there already so this could potentially regress colours in certain weird cases. Simplifies the Makefiles too, you can use the same condition for execinfo as for ncursesw.

lib/clang/llvm.build.mk
102

Shouldn't this already be included thanks to src.opts.mk -> bsd.compiler.mk -> bsd.linker.mk? I didn't need it in my patch.

tools/build/make.py
170
lib/clang/include/llvm/Config/config.h
246

That's fine too. Since ncurses is assumed to be available unconditionally on the host that seems fine.

The only problem I can think of is that it might cause issues for shared builds after upgrading ncurses soversion. Is that something we should worry about?

lib/clang/llvm.build.mk
102

I seem to recall that I got build errors without it, will see if I can drop it.

lib/clang/include/llvm/Config/config.h
246

Well that already needs to be handled carefully today, not a new issue. Plus bumping the ncurses soname is a rare event...

arichardson marked 6 inline comments as done.

Address latest @jrtc27 feedback

contrib/llvm-project/lld/tools/lld/lld.cpp
149–150

Given this is the vendored source, why not just #if 1 this? The condition should always be true in every situation we build this. Then (combined with my sysctl.h suggestion) you don't need LLVM_BOOTSTRAPPING.

lib/libc/db/db/db.c
58 ↗(On Diff #91810)

This diff can be much more minimal if you just do #ifndef/#define 0 like for O_CLOEXEC above (i.e., that's all you need to do, then the existing code here just works). Not in the header, just this file, so it's still an error to try to use one of the missing flags. And yes, this should probably be a separate commit.

tools/build/cross-build/include/common/sys/sysctl.h
39

Hmm. You could make this a BOOTSTRAPPING_WANT_NATIVE_SYSCTL define? That way it's both clearer and cleaner IMO.

tools/build/cross-build/include/common/sys/sysctl.h
39

Yeah that's what I had initially (with a different spelling), will update to use yours.

arichardson marked 4 inline comments as done.

Address more comments

Couple of comments, otherwise looks good from my perspective.

lib/clang/libllvm/Makefile
721

Why's this commented-out? It should build fine during buildworld?

lib/clang/llvm.build.mk
116

This no longer will be passed when building the cross-toolchain on FreeBSD, which I would guess risks breaking some old system GCCs from when libstdc++ was used. I suggest adding the old .if ${.MAKE.OS} == "FreeBSD" || !defined(BOOTSTRAPPING) to this rather than putting it in the .else.

lib/clang/libllvm/Makefile
721

It is fine during buildworld (but not during bootstrap). However, we don't build clangd/clang-query so it just adds to the build time unnecessarily.

lib/clang/llvm.build.mk
116

Sounds good will fix.

arichardson marked an inline comment as done.

address latest review feedbkac

LGTM

tools/build/make.py
259

Pedantic note, maybe chop off one character more to have the PEP checker stop complaining? :)

This revision is now accepted and ready to land.Aug 2 2021, 11:32 AM
tools/build/make.py
259

Will push a pep8 fix commit for that script shortly.

This revision was landed with ongoing or failed builds.Aug 2 2021, 1:38 PM
This revision was automatically updated to reflect the committed changes.
markj added inline comments.
usr.bin/clang/llvm.prog.mk
24

A dumb question: when building bootstrap-tools during buildworld, llvm-tblgen is built and libllvmminimal.a is linked in. The latter contains references to backtrace() since config.h defines HAVE_BACKTRACE unconditionally and llvm::sys::PrintStackTrace() uses it, so the build fails since libexecinfo isn't linked. BOOTSTRAPPING is defined to 0 by the top-level makefile, so how is this supposed to work?

usr.bin/clang/llvm.prog.mk
24

glibc defines it, or at least it does in the 2.27 version present in Ubuntu 18.04, and macOS defines it in libsystem_c.dylib

usr.bin/clang/llvm.prog.mk
24

I see, thanks.