-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Fix the build logic for LLVM libcxx #21792
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
Conversation
deps/llvm.mk
Outdated
LLVM_LDFLAGS += -Wl,-R$(build_libdir) | ||
LLVM_CXXFLAGS += -I$(build_includedir) | ||
LLVM_CPPFLAGS += -I$(build_includedir) | ||
LLVM_CFLAGS += -I$(build_includedir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really have to add -I$(build_includedir)
to all of them? Can't we just put it into CPPFLAGS
? 🐼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need it in all of them, I was just playing it safe because FreeBSD sucks at figuring out where to search for things, plus I figured it wouldn't hurt. I'll try paring it down to just CPPFLAGS
.
As an aside, what's the difference between CPPFLAGS
and CXXFLAGS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPP
stands for C PreProcessor
, not C Plus Plus
. CXX
on the other hand, is C++
, but with the +
symbols turned on their side.
What on God's Green Earth were the programmers thinking.
deps/llvm.mk
Outdated
LLVM_CXXFLAGS += -I$(build_includedir) | ||
LLVM_CPPFLAGS += -I$(build_includedir) | ||
LLVM_CFLAGS += -I$(build_includedir) | ||
LLVM_LIBCXX_FLAGS := -lc++ -lc++abi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are just LDFLAGS
, can we stick them onto LLVM_LDFLAGS
right here? If you don't want to do that, then let's named them LLVM_LIBCXX_LDFLAGS
to make it explicit that these are linker flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, reading further on ahead, we keep them separate so we can bootstrap libcxx
without having it link to itself. Nevermind this then. I would just add the LD
into the name to make it easy to understand what it is right off the bat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably add a comment regarding why the flags are separate further up here in the file.
deps/llvm.mk
Outdated
else | ||
BUILT_UNWIND := | ||
endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these values actually used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack! Yes, the change that includes the use accidentally got lost while I was trying to separate out the changes I made to get FreeBSD working into separate PRs. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
deps/llvm.mk
Outdated
ifeq ($(BUILD_CUSTOM_LIBCXX),1) | ||
LLVM_LDFLAGS += -Wl,-R$(build_libdir) | ||
LLVM_CPPFLAGS += -I$(build_includedir) | ||
# We don't want to link to libc++ while trying to build it, so if we defined these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why if? isn't this describing what you're doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes? It's just how I worded the rationale. I can change it if it's confusing.
deps/llvm.mk
Outdated
$(LLVM_SRC_DIR)/projects/libcxx/.git/HEAD: | $(LLVM_SRC_DIR)/projects/libcxx/.git/HEAD | ||
$(LLVM_SRC_DIR)/projects/libcxxabi: $(LLVM_LIBCXXABI_TAR) | $(LLVM_SRC_DIR)/source-extracted | ||
$(LLVM_SRC_DIR)/projects/libcxx/.git/HEAD: | $(LLVM_SRC_DIR)/projects/libcxx | ||
$(LLVM_SRC_DIR)/projects/libcxxabi: $(LLVM_LIBCXXABI_TAR) | $(LLVM_SRC_DIR)/source-extracted $(BUILD_UNWIND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUILT
@staticfloat Looking better now? |
deps/llvm.mk
Outdated
ifeq ($(USEICC),1) | ||
LIBCXX_EXTRA_FLAGS := -Bstatic -lirc -Bdynamic | ||
endif | ||
|
||
$(LLVM_SRC_DIR)/projects/libcxx: $(LLVM_LIBCXX_TAR) | $(LLVM_SRC_DIR)/source-extracted | ||
# These libraries require unwind.h from the libunwind dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't supposed to be using llvm's version of libunwind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably I don't think it matters; I'm not sure where LLVM puts its unwind.h, plus it builds fine for me both with libunwind and with the system's unwind.h.
@jlbuild !flags=BUILD_CUSTOM_LIBCXX=1 !nuke |
The patchelf build is failing claiming that the C compiler can't create executables... I would think this shouldn't affect patchelf, since we're only passing the libc++ flags to LLVM. |
deps/llvm.mk
Outdated
$(LLVM_BUILD_DIR)/libcxxabi-build/lib/libc++abi.so.1.0: $(LLVM_BUILD_DIR)/libcxxabi-build/Makefile $(LLVM_SRC_DIR)/projects/libcxxabi/.git/HEAD | ||
$(MAKE) -C $(LLVM_BUILD_DIR)/libcxxabi-build | ||
# The installed headers are not needed after building and can interfere with other dependencies | ||
# FIXME: Find a better way to address this | ||
-rm -rf $(build_includedir)/c++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone know of a better way to accomplish this?
@jlbuild !flags=BUILD_CUSTOM_LIBCXX=1 !nuke |
I don't think I've ever actually seen an Looking at the
@ararslan do you have docker on any of your local machines? |
Ah, because we can't set that flag until after we've built libc++. Hmmmmmm... No, I don't have Docker installed. Should be available from Homebrew on my Mac, but I don't know how to use it. |
Do you need |
Don't think so.
I'll give that a shot. Thanks! |
I still have to wait until after patchelf is built to add in |
Okay, looking into this further, what I was proposing actually exists. It appears that |
@jlbuild !flags=BUILD_CUSTOM_LIBCXX=1 !nuke |
Also, if you do end up installing docker (just use the installer from the docker website), you can recreate this locally by running:
That will open up a shell within the same build environment as what the |
deps/patchelf.mk
Outdated
@@ -9,7 +9,7 @@ $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted: $(SRCDIR)/srccache | |||
touch -c $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/configure # old target | |||
echo 1 > $@ | |||
|
|||
$(BUILDDIR)/patchelf-$(PATCHELF_VER)/build-configured: $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted | $(LIBCXX_DEPENDENCY) | |||
$(BUILDDIR)/patchelf-$(PATCHELF_VER)/build-configured: $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted $(LIBCXX_DEPENDENCY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't need to rebuild patchelf anytime LIBCXX_DEPENDENCY is rebuilt (|
means that it only depends on existence, not timestamp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh shoot, sorry, my bad. I just realized I have always misunderstood what the |
means in Makefiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for the correction. I put that change in a separate commit, so I'll just rebase out that one commit. Do you have a better idea for how to deal with this, @vtjnash?
Jameson is right, Furthermore, the error from |
I had a thought. We restrict |
@jlbuild !flags=BUILD_CUSTOM_LIBCXX=1 !nuke |
deps/llvm.mk
Outdated
$(LLVM_SRC_DIR)/projects/libcxx/.git/HEAD: | $(LLVM_SRC_DIR)/projects/libcxx/.git/HEAD | ||
$(LLVM_SRC_DIR)/projects/libcxxabi: $(LLVM_LIBCXXABI_TAR) | $(LLVM_SRC_DIR)/source-extracted | ||
$(LLVM_SRC_DIR)/projects/libcxx/.git/HEAD: | $(LLVM_SRC_DIR)/projects/libcxx | ||
$(LLVM_SRC_DIR)/projects/libcxxabi: $(LLVM_LIBCXXABI_TAR) | $(LLVM_SRC_DIR)/source-extracted $(BUILT_UNWIND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes a make BUILD_CUSTOM_LIBCXX=1 -C deps get-llvm
to actually build libunwind, which is a no-no. We need to depend on libunwind for when we actually build libcxx
, but not when we check the source out. Let's move this dependency down to the target that invokes cmake
just below this.
So I was playing around with this this afternoon. First of all, to play around with this in FROM staticfloat/julia_workerbase_centos6_9:x64
RUN mkdir /src/julia
WORKDIR /src/julia
# Clone @ararslan's branch of Julia, manually storing the details about the
# branch_head.json so that if a new commit gets pushed the docker cache is
# invalidated from here on out
ADD https://api.github.com/repos/JuliaLang/julia/git/refs/heads/aa/libcxx /branch_head.json
RUN git clone -b aa/libcxx https://github.com/JuliaLang/julia .
# Do some steps here first like downloading our prerequisites so that
# they are cached within Docker layers and not getting redownloaded
# every time we try to build LLVM
RUN make BUILD_CUSTOM_LIBCXX=1 -C deps -j3 get-llvm
RUN make MARCH=x86-64 BUILD_CUSTOM_LIBCXX=1 -C deps -j3 install-llvm
#RUN make MARCH=x86-64 BUILD_CUSTOM_LIBCXX=1 -C deps install-patchelf The last line is commented out, but after running
This is because patchelf is accessing the wrong headers. The following patch helps a lot: $ git diff
diff --git a/Make.inc b/Make.inc
index 05ef56699c..8c0df8248f 100644
--- a/Make.inc
+++ b/Make.inc
@@ -1065,6 +1065,7 @@ endif
# Custom libcxx
ifeq ($(BUILD_CUSTOM_LIBCXX),1)
LDFLAGS += -L$(build_libdir)
+CXXFLAGS += -nostdinc++ -I$(build_includedir)/c++/v1
CXXLDFLAGS += -L$(build_libdir) -lc++abi -lc++
ifeq ($(USECLANG),1)
CXXLDFLAGS += -stdlib=libc++
diff --git a/deps/patchelf.mk b/deps/patchelf.mk
index 066fc2ebd8..cc54f212d0 100644
--- a/deps/patchelf.mk
+++ b/deps/patchelf.mk
@@ -12,7 +12,7 @@ $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted: $(SRCDIR)/srccache
$(BUILDDIR)/patchelf-$(PATCHELF_VER)/build-configured: $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted | $(LIBCXX_DEPENDENCY)
mkdir -p $(dir $@)
cd $(dir $@) && \
- $(dir $<)/configure $(CONFIGURE_COMMON) LDFLAGS="$(CXXLDFLAGS)" CPPFLAGS="$(CPPFLAGS)"
+ $(dir $<)/configure $(CONFIGURE_COMMON) LDFLAGS="$(CXXLDFLAGS)" CPPFLAGS="$(CPPFLAGS)" CXXFLAGS="$(CXXFLAGS)"
echo 1 > $@
$(BUILDDIR)/patchelf-$(PATCHELF_VER)/build-compiled: $(BUILDDIR)/patchelf-$(PATCHELF_VER)/build-configured But we now get errors such as the following:
This is because the linker invocation is all wrong; we're specifying the input (
|
Okay, so as it stands on master, this flag works neither with Clang nor with GCC. I have no idea how to get the GCC flags into the proper order to get it working on GCC. I'd like to propose that for now we restrict Does this seem reasonable? |
Some flag we're passing on macOS is keeping it from building there. So far I've only been able to get this to work on FreeBSD, which is an interesting reversal. |
My only use case for this is to get Julia building on FreeBSD. Allow me to revise my proposal: I think we should
|
as long as this PR doesn't make the flag work any worse on non freebsd |
deps/patchelf.mk
Outdated
@@ -12,7 +12,7 @@ $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted: $(SRCDIR)/srccache | |||
$(BUILDDIR)/patchelf-$(PATCHELF_VER)/build-configured: $(SRCDIR)/srccache/patchelf-$(PATCHELF_VER)/source-extracted | $(LIBCXX_DEPENDENCY) | |||
mkdir -p $(dir $@) | |||
cd $(dir $@) && \ | |||
$(dir $<)/configure $(CONFIGURE_COMMON) LDFLAGS="$(CXXLDFLAGS)" CPPFLAGS="$(CPPFLAGS)" | |||
$(dir $<)/configure $(CONFIGURE_COMMON) LDFLAGS="$(CXXLDFLAGS)" CPPFLAGS="$(CPPFLAGS)" CXXFLAGS="$(CXXFLAGS)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need cxxflags or do you need to be splitting cxxflags from cppflags better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was part of something I was trying for getting GCC to work. I still need go through and clean up the changes and commits.
01a26bc
to
01e6ac2
Compare
Okay, cleaned up the changes and successfully rebuilt on FreeBSD. |
64-bit Windows failure is unrelated, so AFAICT this is good to go. |
@@ -1069,8 +1069,14 @@ endif | |||
# Custom libcxx | |||
ifeq ($(BUILD_CUSTOM_LIBCXX),1) | |||
LDFLAGS += -L$(build_libdir) | |||
CXXLDFLAGS += -L$(build_libdir) -lc++abi -stdlib=libc++ -lc++ | |||
CPPFLAGS += -I$(build_includedir)/c++/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where'd this go then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including those headers makes the other dependencies, e.g. OpenBLAS, fail to build.
CXXLDFLAGS += -stdlib=libc++ | ||
else | ||
ifeq ($(USEGCC),1) | ||
$(error BUILD_CUSTOM_LIBCXX is currently only supported with Clang. Try setting BUILD_CUSTOM_LIBCXX=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're sure this limitation was the case before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically no, but in practicality I believe so. According to their documentation, libc++/libc++abi supports building on Linux with GCC. However, I don't see how it would be possible for us to build it with GCC given the way our linker flags are set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this was a functional limitation before. I've never successfully built a custom libcxx with gcc. I tried once when the option was first introduced, but figured that there wasn't much point trying to push it forward since it was most commonly used with clang
.
This allows libc++ and libc++abi to be built on FreeBSD. However, these libraries still cannot be built with GCC nor on macOS, though that condition predates this change.
I'd like to get this merged. Are there any further comments? |
There have been no further comments and Elliot gave his okay offline, so I'm going to go ahead and merge this. Thanks everyone for your help and feedback! |
With this change, libc++ and libc++abi build for me. Without it I was getting errors since
-lc++ -lc++abi
was passed in too soon.