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

libllvm: update from 9.0.1 to 10.0.0 #5136

Closed
wants to merge 2 commits into from
Closed

Conversation

finagolfin
Copy link
Member

Took the opportunity to clean up a bit and reorganize which packages come with what, see comments below. I only built for aarch64, going to try installing locally and building with these packages now.

Disabled the build in this pull for now, until all the tweaks are reviewed and I get the lldb portion done.

bin/clang-offload-bundler
bin/git-clang-format
bin/macho-dump
bin/clang-offload-wrapper
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These clang binaries might be useful, while macho-dump isn't built anymore and the last wrapper is new and isn't in the clang package for Arch linux, so left it out.

@@ -96,16 +89,21 @@ termux_step_pre_configure() {
termux_error_exit "Invalid arch: $TERMUX_ARCH"
fi
# see CMakeLists.txt and tools/clang/CMakeLists.txt
TERMUX_PKG_EXTRA_CONFIGURE_ARGS+=" -DLLVM_DEFAULT_TARGET_TRIPLE=$LLVM_DEFAULT_TARGET_TRIPLE"
Copy link
Member Author

@finagolfin finagolfin Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set to LLVM_HOST_TRIPLE by default, so no need to set it separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host and target are different. They have to be explicitly set as the same. This is due to jit compiler. Last time it wasn't set like that jit compiled and tried to run x86_64 code on aarch64. It then fails...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean, the host triple is also set to this just two lines below. The LLVM CMake config then sets the default target triple to be the same as the host triple on Android.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jit compiles code then runs because the default target and host can be different, say default target is i686 and the host is x86_64. This means jit will use x86_64 and because it needs to run on host. Jit code is more like a script that then compiles it first time you run it.
For us last time the build host remained default target were different causing the jit to use x86_64. Since that was the host it was built on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, but the host triple is set to Android just two lines below, and then LLVM sets the default target triple to the host triple in the code I linked in my original comment, so they should always be the same now too.

-e "s|@TERMUX_ARCH@|$TERMUX_ARCH|g" > $TERMUX_PREFIX/bin/llvm-config
chmod 755 $TERMUX_PREFIX/bin/llvm-config
cp $TERMUX_PKG_HOSTBUILD_DIR/bin/llvm-tblgen $TERMUX_PREFIX/bin
cp $TERMUX_PKG_HOSTBUILD_DIR/bin/clang-tblgen $TERMUX_PREFIX/bin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@its-pointless, you added these two, but as above, they're not packaged since they aren't in the massage dir (plus, these are the host binaries built for x86_64, probably a typo). I notice that an AArch64 llvm-tblgen is already packaged currently, but clang-tblgen isn't installed by the LLVM config by default: would you like me to add it?

Copy link
Member

@Grimler91 Grimler91 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we added the fast-build -i option we were having problems with packages trying to execute llvm-tblgen and clang-tblgen, the reason for the host binaries being installed here is probably related to that. I think we solved it some other way do so can probably remove it now though.

Edit: we need it for dependent packages:

mv llvm-tblgen $TERMUX_PREFIX/bin

would be nice to solve it in another way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is that why the llvm-config script is there too, not for the LLVM packages but for other packages that need to call llvm-config on the host linux build server? I just checked and I see that the rust and lldb package build scripts call it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need it for dependent packages

No, that's downloaded externally, see the two lines above the one you pasted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I guess its-pointless put the needed files in a tar and uploaded them to his repo. Maybe updated version is needed for lldb to build with llvm 10

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason to wait, the NDK sysroot and clang are independent. In fact, I was surprised to find yesterday that this repo goes out of its way not to use the NDK sysroot package when cross-compiling Termux packages, unlike for on-device builds.

Copy link
Contributor

@its-pointless its-pointless Apr 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More about bandwidth management.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it becomes a more significant issue since rust will also need to be updated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I don't know how big a deal updating Rust is, I don't use it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried updating the LLVM that the Rust package links against, let's see if it works.

lib/clang
lib/cmake/clang
lib/libclang*so
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scan-build and scan-view binaries are normally packaged with clang and git-clang-format could be useful. The omp.h header should be included here, since the library is too.

The libclang*so shared libraries were previously included with the libllvm package, and there are still 34 libclang*a static libraries included with libllvm-static. I can keep them there, include them here, or create a separate libclang-static, whatever is wanted.

-e "s|@LLVM_DEFAULT_TARGET_TRIPLE@|$LLVM_DEFAULT_TARGET_TRIPLE|g" \
-e "s|@TERMUX_ARCH@|$TERMUX_ARCH|g" > $TERMUX_PREFIX/bin/llvm-config
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored and updated the llvm-config script and noticed that these two *_ARCH variables are unused now.

version=@TERMUX_PKG_VERSION@
prefix=@TERMUX_PREFIX@
has_rtti=NO
CPPFLAGS="-I${prefix}/include -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS"
CFLAGS="${CPPFLAGS} ${CFLAGS} -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS"
Copy link
Member Author

@finagolfin finagolfin Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the second CFLAGS is to get those flags if they're set in the environment? It works for that, but I checked and the llvm-config binary doesn't do that. No idea why the *_MACROS flags were repeated here, considering they were already added by the prepended CPPFLAGS.

CFLAGS="${CPPFLAGS} ${CFLAGS} -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS"
CXXFLAGS="${CFLAGS} -fvisibility-inlines-hidden -Wcast-qual -Wnon-virtual-dtor -std=c++11 -fno-exceptions"
CFLAGS="${CPPFLAGS} ${CFLAGS}"
CXXFLAGS="${CFLAGS} -std=c++14 -fno-exceptions"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The llvm-config binary doesn't have those three flags I removed, let me know if any should be kept.

@finagolfin
Copy link
Member Author

I just used the LLVM packages from this pull to build the entire Swift 5.2.1 package on-device without a problem, looks good. I'll get lldb built next and this should be ready to go. In the meantime, I'd appreciate any review on this libllvm package.

-DLLVM_ENABLE_TERMINFO=1
-DLLVM_LINK_LLVM_DYLIB=ON
-DLLVM_DIR=$TERMUX_PREFIX/lib/cmake/llvm
-DClang_DIR=$TERMUX_PREFIX/lib/cmake/clang
-DLLVM_NATIVE_BUILD=$TERMUX_PREFIX/bin
-DLLVM_TABLEGEN=$TERMUX_PKG_HOSTBUILD_DIR/llvm/bin/llvm-tblgen
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first three DISABLE_ flags are ignored just like LLVM_CONFIG, it figures out Clang_DIR based on the LLVM_DIR, and specifying LLVM_TABLEGEN replaces LLVM_NATIVE_BUILD.


cd ..
cmake -G Ninja $TERMUX_PKG_SRCDIR
ninja -j $TERMUX_MAKE_PROCESSES lldb-tblgen
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than pre-building and downloading these, just build them from scratch.

}

termux_step_make() {
ninja -w dupbuild=warn -j $TERMUX_MAKE_PROCESSES all docs-lldb-man
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, it doesn't build the man page by default.

@finagolfin
Copy link
Member Author

Got lldb built for AArch64 and seems to be working for me locally. I tried bumping the Rust package to use this, let's see if it works.

@finagolfin
Copy link
Member Author

While lldb built fine for me on my Arch linux build server using this pull, it wouldn't build the host lldb-tblgen on this Ubuntu CI, probably because it was linking against the host LLVM 9, so I added another commit to link against the just-built libraries from LLVM 10 that accompany llvm-tblgen, which made that earlier flag I added unnecessary. However, that caused problems because the lldb standalone config looks for clang, even though that isn't needed for lldb-tblgen, so I just patched those clang references out for the host build.

libllvm and lldb are building fine now, all that remains is rust.

@finagolfin
Copy link
Member Author

I don't know what to make of the rust error, something about llvm-config --version not working (which I'm not able to reproduce), everything else builds fine.

@finagolfin
Copy link
Member Author

Looks like it was finally working, it just ran out of space somewhere in the Rust build. I removed the Rust commit: what we could try is merging this llvm/lldb pull alone once it passes the CI and then I'll submit the same Rust commit in a separate pull, so it can finish building this time.

Once the Rust package is built, you can verify that the Rust package works and either merge, or revert this LLVM 10 pull till later. Let me know.

@finagolfin
Copy link
Member Author

Thought of another way to test Rust, now that both the llvm and lldb updates built fine. I simply added two more commits, one to revert the lldb commit, another to update Rust. I don't think the CI will build lldb anymore and hopefully it'll clear up enough disk space for the Rust build to finish. If those Rust packages built by the CI work fine, then the commit reverting lldb can be removed and everything is ready to go.

@finagolfin
Copy link
Member Author

Heh, the LLVM/Rust combo ran out of disk space on the CI too.

@peter9811
Copy link

It's possible get a update to version 11?

else
cp ../src/projects/openmp/runtime/exports/common.min.ompt.optional/include/omp.h $TERMUX_PREFIX/include
fi
cp $TERMUX_PKG_SRCDIR/projects/openmp/runtime/exports/common.min.ompt.optional/include/omp.h $TERMUX_PREFIX/include
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am getting an error here when I try to build locally for aarch64. omp.h is at
/home/builder/.termux-build/libllvm/src/projects/openmp/runtime/exports/common.ompt.optional/include/omp.h
instead of
/home/builder/.termux-build/libllvm/src/projects/openmp/runtime/exports/common.min.ompt.optional/include/omp.h
Seems to work for the CI though, weird

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed nothing for this other than adding the prefix $TERMUX_PKG_SRCDIR to use the absolute path. I only built this package for AArch64 on Arch linux x86_64, didn't build for ARM myself to make sure that separate path was right.

Copy link
Contributor

@its-pointless its-pointless May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i have compiled with both. On old docker image it worked, on new it doesn't. So its nobodies fault.

Basically removing the min part on both paths fixes it.

@its-pointless
Copy link
Contributor

Updated docker image might be the cause

@Grimler91
Copy link
Member

@buttaface could you rebase against master, and fix the common.min.ompt.optional->common.ompt.optional since it seems to be required for new docker image?

@its-pointless
Copy link
Contributor

I have already merged his libllvm and the fix into my r21b pull

@finagolfin
Copy link
Member Author

Sounds good, you can just merge it there.

@finagolfin finagolfin closed this May 9, 2020
@Grimler91
Copy link
Member

@its-pointless sorry, didn't see that, great!

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

Successfully merging this pull request may close these issues.

4 participants