[compiler-rt] Fix stale incremental builds when using `LLVM_BUILD_EXTERNAL_COMPILER_RT=ON`.

[delcypher]

Summary

[compiler-rt] Fix stale incremental builds when using LLVM_BUILD_EXTERNAL_COMPILER_RT=ON.

When building with LLVM_BUILD_EXTERNAL_COMPILER_RT=ON (e.g. Swift does
this) we do an "external" build of compiler-rt where we build
compiler-rt with the just built clang.

Unfortunately building in this mode had a bug where compiler-rt would
not get rebuilt if compiler-rt sources changed. This is problematic
for incremental builds because it meant that the compiler-rt binaries
were stale.

The fix is to use the BUILD_ALWAYS ExternalProject_Add option which
means the build command for compiler-rt is always run.

In principle if all of the following are true:

• compiler-rt has already been built.
• there are no compiler-rt source changes.
• the compiler hasn't changed.
• ninja is being used as the generator for the compiler-rt build.

then the overhead for always running the build command for incremental
builds is negligible.

However, in practice clang gets rebuilt every time the HEAD commit
changes (due to commit hash being embedded in the output of --version)
which means all of compiler-rt will be rebuilt every time this happens.
While this is annoying it's better to do the slow but correct thing
rather than the fast but incorrect thing.

rdar://75150660

[delcypher]

• update description

[kubamracek]

When using Ninja, does this mean running a null build is no longer a null build, but it at least always invokes this sub-build? Not saying that's bad, just want to understand the change. What does the output of just "ninja" say in that case?

[beanz]

So... this patch is fine. (marked as approved). That said, we should really be working to remove the LLVM_BUILD_EXTERNAL_COMPILER_RT option entirely in favor of just using LLVM_ENABLE_RUNTIMES=compiler-rt. That build process does support invoking the sub-project build on build invocation to make sure builds get re-run, and it supports better understanding of the dependencies between runtimes.

Have you considered using that instead of LLVM_BUILD_EXTERNAL_COMPILER_RT?

[delcypher]

In D98291#2615590, @kubamracek wrote:

When using Ninja, does this mean running a null build is no longer a null build, but it at least always invokes this sub-build? Not saying that's bad, just want to understand the change. What does the output of just "ninja" say in that case?

Yes the sub-build will always be invoked. That being said in the sub-build if nothing has changed it will be extremely fast because ninja is fast.

The output looks something like this

$ ninja
[0/3] Performing build step for 'compiler-rt'
ninja: no work to do.
[1/3] No install step for 'compiler-rt'
[3/3] Completed 'compiler-rt'

This being said I'd expect very few people to actually run into this because this build configuration (LLVM_BUILD_EXTERNAL_COMPILER_RT=ON) is not common because it is not the default. The default if you enable compiler-rt is for it to be built as part of the main build using the host compiler, which is all kinds of wrong but is very convenient.

Swift does use this build configuration but I doubt anyone would complain about the extra console noise given how noisy build-script is.

[delcypher]

In D98291#2615593, @beanz wrote:

So... this patch is fine. (marked as approved). That said, we should really be working to remove the LLVM_BUILD_EXTERNAL_COMPILER_RT option entirely in favor of just using LLVM_ENABLE_RUNTIMES=compiler-rt. That build process does support invoking the sub-project build on build invocation to make sure builds get re-run, and it supports better understanding of the dependencies between runtimes.

Have you considered using that instead of LLVM_BUILD_EXTERNAL_COMPILER_RT?

@beanz It's something I've thought about but haven't had the time to do. The fact that we have 3 different ways to build compiler-rt in LLVM makes me sad. There should only be 1. I don't have time to do it now but I've filed a radar to do this work at some point (rdar://75248447).

[beanz]

In D98291#2615663, @delcypher wrote:

The fact that we have 3 different ways to build compiler-rt in LLVM makes me sad. There should only be 1. I don't have time to do it now but I've filed a radar to do this work at some point (rdar://75248447).

+1,000

The runtimes directory is really the only "correct" way to build the runtime libraries. Someday I hope we can get everything building through it.

In the meantime since reality is a thing... this patch is a good bug fix.

[kubamracek]

Looks good!

By the way, Apple Clang releases also build compiler-rt this way, so it would be worth checking with @arphaman that he's okay with the change.

[delcypher]

In D98291#2616865, @kubamracek wrote:

Looks good!

By the way, Apple Clang releases also build compiler-rt this way, so it would be worth checking with @arphaman that he's okay with the change.

@kubamracek @arphaman Ah sorry I landed the change before I saw this message. It looks like the LLVM_BUILD_EXTERNAL_COMPILER_RT option is used as part of the 2-stage boot strap for AppleClang as it's set in clang/cmake/caches/Apple-stage2.cmake. I'll follow up offline.

[beanz]

This should not impact the Apple Clang releases since those are always clean builds.

clang/runtime/CMakeLists.txt

CMAKE_ARGS ${CLANG_COMPILER_RT_CMAKE_ARGS}
                -DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET}
                -DCMAKE_OSX_SYSROOT:PATH=${CMAKE_OSX_SYSROOT}
                ${COMPILER_RT_PASSTHROUGH_VARIABLES}
     INSTALL_COMMAND ""
     STEP_TARGETS configure build
     USES_TERMINAL_CONFIGURE 1
     USES_TERMINAL_BUILD 1
     USES_TERMINAL_INSTALL 1
+    # Always run the build command so that incremental builds are correct.
+    BUILD_ALWAYS 1
     )
 
   get_ext_project_build_command(run_clean_compiler_rt clean)
   ExternalProject_Add_Step(compiler-rt clean
     COMMAND ${run_clean_compiler_rt}
     COMMENT "Cleaning compiler-rt..."
     DEPENDEES configure
     DEPENDERS build