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

Replace build-ldc-runtime.sh script by D tool #2253

Merged
merged 12 commits into from
Aug 5, 2017

Conversation

kinke
Copy link
Member

@kinke kinke commented Jul 30, 2017

Primarily for portability (Windows) and to pave the way for a simpler interface. Already comes with ninja and testrunners support and a consistent command-line interface wrt. D/C/linker flags.

To be used like this:

bin\ldc-build-runtime [--ldcSrcDir=..\ldc] [--ninja] [-j<N>] [--testrunners]
[--dFlags=-mtriple=x86_64-pc-windows-msvc;...] [--cFlags=...] [--linkerFlags=...]
[CMAKE_VAR=value ...]

@kinke kinke force-pushed the build-ldc-runtime branch 5 times, most recently from 7f53cef to bd01264 Compare July 30, 2017 19:11
@kinke kinke force-pushed the build-ldc-runtime branch 2 times, most recently from a264bfd to 296c75b Compare July 30, 2017 21:41
@joakim-noah
Copy link
Contributor

Heh, very nice, looking forward to trying this.

Primarily for portability (Windows) and to pave the way for a simpler
interface. Already comes with ninja and testrunners support and a
consistent command-line interface wrt. D/C/linker flags.

To be used like this:

bin\build-ldc-runtime [--ldcSrcDir=..\ldc] [--ninja] [--testrunners]
[--dFlags=-mtriple=x86_64-pc-windows-msvc;...] [--cFlags=...]
[--linkerFlags=...] [CMAKE_VAR=value ...]
The main object apparently needs to come first for TLS on Android.
@JohanEngelen
Copy link
Member

Testing it now, works well!
./build-ldc-runtime --ldcSrcDir=/Users/johan/ldc/johan --dFlags="-fsanitize=address,fuzzer;-disable-fp-elim" BUILD_SHARED_LIBS=OFF

I like that there is a --help. My only complaint is that the help output should mention the defaults for --ldc, --buildDir, and --ldcSrcDir.

@JohanEngelen
Copy link
Member

Trying to help: I've added the defaults to the help message.

You can merge this if you are happy with it. (how about starting the name of the program with "ldc", like "ldc-profdata", "ldc2", "ldc-prune-cache", so they are nicely grouped in file listings?)

@joakim-noah
Copy link
Contributor

joakim-noah commented Aug 3, 2017

Alright, finally tried this out in a git repo after building a cross-compiler for Android/ARM. All the D files built fine with the following commands, confirmed by comparing md5 hashes with my normal build:

make build-ldc-runtime
CC=/home/joakim/ndk-r15b/toolchains/llvm/prebuilt/linux-x86_64/bin/clang ./bin/build-ldc-runtime
--ldcSrcDir=/home/joakim/ldc BUILD_SHARED_LIBS=OFF C_SYSTEM_LIBS="m;c"
--cFlags="-gcc-toolchain;/home/joakim/ndk-r15b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64;
-fpic;-ffunction-sections;-funwind-tables;-fstack-protector-strong;-Wno-invalid-command-line-argument;
-Wno-unused-command-line-argument;-no-canonical-prefixes;-fno-integrated-as;-target;armv7-none-linux-androideabi;
-march=armv7-a;-mfloat-abi=softfp;-mfpu=vfpv3-d16;-mthumb;-Os;-g;-DNDEBUG;-fomit-frame-pointer;
-fno-strict-aliasing;-DANDROID;-Wa,--noexecstack;-Wformat;-Werror=format-security;
-isystem;/home/joakim/ndk-r15b/platforms/android-21/arch-arm/usr/include"

I only had to make two changes to the runtime CMake config, removing the core.stdc.tgmath module and commenting out the line that sets RT_CFLAGS to an empty string. You'll need to fix the latter by removing that line in this PR, or RT_CFLAGS will never be set properly.

I initially tried running the build-ldc-runtime executable with no flags, and it tries to get ldc with a particular git hash and untar it, then fails because github doesn't return a tarfile. Seems like you don't handle that case, may need to add some code for that.

I'm able to set the C flags, but I notice that the C defines and includes are empty. Here's what they are for my default build of the druntime-ldc target for Android/ARM:

C_DEFINES = -DLDC_LLVM_SUPPORTED_TARGET_ARM=1
C_INCLUDES = -I/home/joakim/ldc/. -I/home/joakim/ldc/ddmd -I/home/joakim/ldc/ddmd/root
-I/home/joakim/ldc/build14/ddmd -I/home/joakim/ldc -isystem /home/joakim/llvm-4.0.1.src/include

Not sure if that matters, but the md5 hashes of the resulting C object files differ.

Finally, the two assembly files, threadasm.S and eh_asm.S, from druntime aren't built with your approach, not sure why. I tried setting CMAKE_ASM_COMPILER to CMAKE_C_COMPILER from both the command-line and in the runtime CMake config, no difference. Let me know if they're built for you.

One last request, it'd be nice to be able to supply specific targets to make, rather than only all and all-test-runners.

@kinke
Copy link
Member Author

kinke commented Aug 3, 2017

I've added the defaults to the help message.

Thanks Johan, I'll have to correct them though. ;)

commenting out the line that sets RT_CFLAGS to an empty string. You'll need to fix the latter by removing that line in this PR, or RT_CFLAGS will never be set properly.

Thanks, will do.

it tries to get ldc with a particular git hash and untar it, then fails because github doesn't return a tarfile. Seems like you don't handle that case, may need to add some code for that.

I was too lazy to handle this case (non-packaged tool and not built manually from tagged source) so far.

I'm able to set the C flags, but I notice that the C defines and includes are empty.

That's basically CMake's job. I guess using a gcc toolchain (still via CC env variable) instead of a clang cross-compiler (requiring -gcc-toolchain /home/joakim/ndk-r15b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 etc. which CMake knows nothing about when initially probing the C compiler) makes things considerably easier; your list of C flags is pretty huge. ;)
I guess that would also get rid of the asm issues, although I would have guessed that it could be worked around via CMAKE_ASM_COMPILER. It's definitely working for native builds (for Johan on OSX and me on Windows).

One last request, it'd be nice to supply specific targets to make, rather than only all and all-test-runners.

Hmm, if you come up with a simple interface, feel free to add it to your PR then. For non-trivial use cases, running the tool once and then executing cd <build dir> && make/ninja <my targets> should be pretty straightforward.

@joakim-noah
Copy link
Contributor

I was too lazy to handle this case (non-packaged tool and not built manually from tagged source) so far.

Not a big deal, just thought I'd let you know.

That's basically CMake's job. I guess using a gcc toolchain (still via CC env variable) instead of a clang cross-compiler (requiring -gcc-toolchain /home/joakim/ndk-r15b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 etc. which CMake knows nothing about when initially probing the C compiler) makes things considerably easier; your list of C flags is pretty huge. ;)

Looking further, it appears these C defines/includes are set from the top-level CMake config for ldc, then passed down to the runtime config also. I stripped both versions of the druntime C object files and then compared the md5 hashes and there's no difference, as we'd expect for all those includes of ddmd/ldc/llvm directories, so these defines/includes likely don't matter.

I'll look into the assembly issue now.

@joakim-noah
Copy link
Contributor

Found it, I needed to add this line to the runtime's CMake Config to get it to include the assembly files. It automatically uses the C compiler for ASM too, was probably only setting that manually before because I wasn't using CC to set the C compiler, so there was some ambiguity. Stripped and md5 hashed the two asm object files, same as my normal build.

@kinke
Copy link
Member Author

kinke commented Aug 3, 2017

Perfect. I don't think we need asm for LDC itself, so maybe just moving it to the runtime's CMake script will do, I'll check.

Wrt. core.stdc.tgmath, I think the better way to approach this is to just version it out in druntime for Android, so that it's clear what to do if someone relies on it. Burying the exception in the CMake script makes it hard to track down otherwise.

@joakim-noah
Copy link
Contributor

Yeah, don't worry about core.stdc.tgmath, I'll figure something out for that in my PR. That's why I didn't say you should do anything about it earlier.

@joakim-noah
Copy link
Contributor

Trying out --testrunners now, going slow because it doesn't build in parallel by default, maybe add a flag to force parallel compilation at some number of jobs?

@kinke
Copy link
Member Author

kinke commented Aug 3, 2017

[Ninja exploits parallelization out of the box.] Updated and tried to incorporate all most suggestions.

@joakim-noah
Copy link
Contributor

OK, I did see that about ninja, as I tried porting it to run on Android, but it relies on posix_spawn, which isn't there in Bionic. I found a half-implemented version in another AOSP repo, but it wasn't enough for ninja, so I left it there for now.

I just installed ninja in my linux/x64 VPS where I'm cross-compiling for Android/ARM, right before you mentioned it, looking forward to trying it out.

This PR is looking good, thanks for adding the parallel jobs flag. I'll let you know the results of cross-compiling the test runners to Android/ARM soon.

@joakim-noah
Copy link
Contributor

joakim-noah commented Aug 4, 2017

All tests from druntime and phobos pass when cross-compiled to Android/ARM (except for the one from std.random that fails unrelated to this), using this new binary and verified with both make and ninja.

@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

Perfect, so this is good to go, right? I guess you had to add some --linkerFlags in your command line, anything else?

@joakim-noah
Copy link
Contributor

This is the full command I ran to build the test runners using ninja:

CC=/home/joakim/ndk-r15b/toolchains/llvm/prebuilt/linux-x86_64/bin/clang ./bin/ldc-build-runtime --ldcSrcDir=/home/joakim/ldc BUILD_SHARED_LIBS=OFF C_SYSTEM_LIBS="m;c" --cFlags="-gcc-toolchain;/home/joakim/ndk-r15b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64;-fpic;-ffunction-sections;-funwind-tables;-fstack-protector-strong;-Wno-invalid-command-line-argument;-Wno-unused-command-line-argument;-no-canonical-prefixes;-fno-integrated-as;-target;armv7-none-linux-androideabi;-march=armv7-a;-mfloat-abi=softfp;-mfpu=vfpv3-d16;-mthumb;-Os;-g;-DNDEBUG;-fomit-frame-pointer;-fno-strict-aliasing;-DANDROID;-Wa,--noexecstack;-Wformat;-Werror=format-security;-isystem;/home/joakim/ndk-r15b/platforms/android-21/arch-arm/usr/include" --testrunners --dFlags="-w" --linkerFlags="-Wl,-z,nocopyreloc;--sysroot=/home/joakim/ndk-r15b/platforms/android-21/arch-arm;-lgcc;-gcc-toolchain;/home/joakim/ndk-r15b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64;-target;armv7-none-linux-androideabi;-no-canonical-prefixes;-fuse-ld=bfd;-Wl,--fix-cortex-a8;-Wl,--no-undefined;-Wl,-z,noexecstack;-Wl,-z,relro;-Wl,-z,now;-Wl,--warn-shared-textrel;-Wl,--fatal-warnings;-fPIE;-pie" --ninja

I want to have a list of supported platforms in the runtime CMake config, so that we can stick all these flags in there and people can just build the runtime by setting the target OS and Arch. Now that you got the linker changes in, that's basically all my Android PR will consist of. @redstar can add his flags for linux/PPC and so on, let me know what you think.

As for this PR, I want to try it with a cross-compiler build of ldc 1.3, the one I put up releases for. I'll have to modify the runtime CMake config a bit to apply parts of this patch, but I'll get that running today and get back to you with the results.

@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

I was thinking about putting up a Wiki page about this tool and how to use it to a) generate LTO-able runtime libs for best performance and b) cross-compile them for other targets. I hope a few compact examplary command lines on the Wiki page are enough for people to get it to work with their particular setup.

Have you tried compiling via your cross-gcc or is using Clang mandatory? I think the list of flags could at least be halved by using gcc.

As for this PR, I want to try it with a cross-compiler build of ldc 1.3

Not sure what this is supposed to prove; you'll probably need a bunch of more runtime/CMakeLists.txt patches included in earlier PRs to get it to work.

@joakim-noah
Copy link
Contributor

A wiki page would be good, but there's no reason not to stick all these flags we're using in the runtime CMake config and let people use them easily with just two config options, target OS and arch. That way, they can easily cross-compile, but override the flags if wanted.

I haven't used gcc in years. The Android NDK and Termux use clang and are switching to libc++, and the NDK will remove gcc next year. Some flags might not be needed with gcc but most affect codegen, so you'd want them regardless.

I want to try this PR with ldc 1.3 to see how well it works with a release, as opposed to from git. Probably the same, but testing will verify that. I may have to pull in earlier PRs of yours too, let's see.

@JohanEngelen
Copy link
Member

(just popping in here to say that having this tool already has been very useful for me, being able to rebuild the runtime with specific D options (-fsanitize), great stuff.)

arraySep = ";";
auto helpInformation = getopt(
args,
"j", "Number of parallel build jobs", &config.numBuildJobs,
Copy link
Contributor

@joakim-noah joakim-noah Aug 4, 2017

Choose a reason for hiding this comment

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

When viewing this in the console, it looks like this:

-j                      Number of parallel build jobs
                  --ldc Path to LDC executable (default: '/home/joakim/ldc/build14/bin/ldc2')
             --buildDir Path to temporary build directory (default: './ldc-build-runtime.tmp')
---snip---
          --linkerFlags C linker flags for shared libraries and testrunner executables (separated by ';')
-h               --help This help information.

Apparently, single-character flags go all the way on the left, while full-word flags go in the middle. It looks weird like this: maybe move -j to the end before -h, so the single-character ones are on the bottom.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I found it pretty weird too; I just put it at the top as it's probably the most important switch when using make. ;) It'd probably look a bit better below.

@joakim-noah
Copy link
Contributor

joakim-noah commented Aug 4, 2017

OK, trying this PR with my ldc 1.3 cross-compiler, by simply building a ldc-build-runtime that's configured for 1.3/2.073 and copying the runtime CMake config from master with this patch applied over to the ldc-src of 1.3, after it's downloaded and fails with its own CMake config. I hit an issue where it complains about LDC_EXE not being defined because of this dependency, and make errors out with this:

make[2]: *** No rule to make target 'ldc-src/runtime/LDC_EXE-NOTFOUND', needed by 'objects-debug/etc/c/curl.o'.  Stop.

Sure enough, CMakeCache.txt shows LDC_EXE:FILEPATH=LDC_EXE-NOTFOUND, though it's not mentioned at all in the CMake cache when I ran it from the git repo and that worked fine. Commenting out that dependency from the runtime CMake config gets everything to work again, it's the only change I had to make other than removing core.stdc.tgmath.

Not sure why this fails when run with an ldc binary as opposed to a source install, some CMake magic? Just tried Ninja, it spits out a long warning about this, but puts in a phony rule and goes ahead and builds anyway.

@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

I was aware of LDC_EXE being NOT set when running the runtime project alone and that it'd be a null-dependency in that case (just for the top-level build target; the file dependency is still in place), which doesn't hurt (and I've seen no warnings about it).
But there have been other weird issues. For example, at the beginning, (accidentally) using the append() CMake helper function from the top-level CMakeLists.txt wasn't a problem, and later it became one. So I strongly suspect the top-level CMake script to interfere somehow. So maybe try copying over the top-level CMake script to the 1.3 ldc-src as well.

@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

Ah wait, so the script first ran CMake with the original CMake script, failed, and then you replaced the CMake script and ran the tool again? That's not good; you should remove everything but the (patched) ldc-src dir from the build dir before running the tool a 2nd time. CMake's cache can be tricky, resetting the build dir sometimes works wonders.

@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

[A --reset switch may come in handy for automating this...]

@joakim-noah
Copy link
Contributor

Yep, thats it: LDC_EXE isn't a problem after all, just the fact that I was running cmake twice with two different configs. I deleted all CMake buildfiles after swapping in the config, as you directed, and everything works after that.

I ran all the ldc 1.3 stdlib tests built with your binary, both release and debug, with make and ninja, on Android/ARM: everything passes.

"ninja", "Use Ninja as CMake build system", &config.ninja,
"testrunners", "Build the testrunner executables too", &config.buildTestrunners,
"dFlags", "LDC flags for the D modules (separated by ';')", &config.dFlags,
"cFlags", "C compiler flags for the C modules (separated by ';')", &config.cFlags,
Copy link
Contributor

Choose a reason for hiding this comment

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

C has modules? ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that it's used for the assembly files too, "C/ASM compiler flags for the handful of C/ASM files in the stdlib" or something like that.

Copy link
Contributor

@joakim-noah joakim-noah left a comment

Choose a reason for hiding this comment

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

Two tiny help doc nits, but nothing else: nice work.

@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

Thanks a lot for testing. Updated.

@joakim-noah
Copy link
Contributor

Three new commits LGTM, will merge in an hour if nobody objects.

@joakim-noah
Copy link
Contributor

Semaphore and Travis CI are failing because ldc-build-runtime doesn't compile.

@kinke
Copy link
Member Author

kinke commented Aug 5, 2017

Fixed. Too bad these methods' constness depends on the OS, but oh well.

@joakim-noah joakim-noah merged commit 953e560 into ldc-developers:master Aug 5, 2017
@kinke kinke deleted the build-ldc-runtime branch August 5, 2017 16:39
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.

3 participants