[cmake] Serialize native builds for Make generator #121021

arthurqiu · 2024-12-24T02:18:58Z

The build system is fragile by allowing multiple invocation of subprocess builds in the native folder for Make generator.

For example, during sub-invocation of the native llvm-config, llvm-min-tblgen is also built. If there is another sub-invocation of the native llvm-min-tblgen build running in parallel, they may overwrite each other's build results, and may lead to errors like "Text file busy".

This patch adds a cmake script that uses file lock to serialize all native builds for Make generator.

joker-eph

Isn't this fragile? The build system allows multiple invocation of subprocess builds in the native folder.
Isn't there a way to present this?

arthurqiu · 2024-12-24T08:27:13Z

@joker-eph this is indeed fragile. Following previous discussion in https://reviews.llvm.org/D149072, it shouldn't be a problem for Ninja generator because of the use of console tool and for Visual Studio because of the introduction of dependency chain. It mentioned job server for GNU make, but I don't see it's being used anywhere.

I think it's possible to leverage job server to serialize the sub-make invocation. More specifically, below setup is needed if I understand it correctly:
Step 1. create a named pipe
Step 2. generate 1 one-byte token into the pipe
Step 3. pass the named pipe via --jobserver-auth option for the sub-make invocation

Alternatively, shall we simply apply the dependency chain to GNU make generator as well? It's mentioned in https://reviews.llvm.org/D149072 that it would regress Makefile build time. However, I don't feel it would regress much compared with the job server solution.

--
EDIT: After a second thought, I think both mentioned solutions would regress the build time significantly.

joker-eph · 2024-12-24T11:13:31Z

I think you should pull in the folks from the review you mentioned.

arthurqiu · 2024-12-25T05:39:07Z

Inspired by https://discourse.cmake.org/t/good-way-of-running-custom-commands-serially/8412, I implemented serial execution of sub-make invocations via cmake file(lock). The alternative is to use flock, but flock is not available on all platforms.

arthurqiu · 2024-12-25T05:40:53Z

@llvm-beanz @aganea @smeenai @mstorsjo @chapuni Please kindly review.

arthurqiu · 2025-01-03T01:53:57Z

Ping

llvm-beanz

I think this mostly looks fine. One small suggestion.

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

arthurqiu · 2025-01-08T09:44:47Z

@joker-eph @llvm-beanz The PR is ready for merge. I don't have commit access to llvm-project repo. Could you please help merge if it looks all good.

arthurqiu · 2025-01-13T03:45:54Z

@joker-eph @llvm-beanz The PR is ready for merge. I don't have commit access to llvm-project repo. Could you please help merge if it looks all good.

Ping. Sorry for bothering.

smeenai · 2025-01-13T21:55:22Z

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

+    set(make_cmd "$(MAKE)" "-C" "${bin_dir}" "${target}")
+    set(file_lock_script "${LLVM_CMAKE_DIR}/FileLock.cmake")
+    set(${out_var} ${CMAKE_COMMAND} "-DLOCK_FILE_PATH=${stamp_dir}/cmake.lock"
+                                    "-DCOMMAND='${make_cmd}'"


We might be better off using something like | instead of a semicolon-separated list when constructing make_cmd above; and then replacing | with ; inside FileLock.cmake. Using \; or $<SEMICOLON> (since all places this will be used support generator expressions) would also work. You can then remove the separate_arguments inside FileLock.cmake and have things still work correctly for paths with spaces.

The single quotes here will also likely cause issues on Windows; you can use \" instead so that the command ends up with double quotes instead.

@smeenai Thanks for reviewing. I've made changes according to your suggestions. Please take a look.

I found that semicolon-separated list just happened to work because VERBATIM was not used in add_custom_command, even that the build cmd was indeed problematic.

I did a quick test and it looks like ExternalProject_Add_Step would wrap command with sh -c, i.e. it has the same behavior as add_custom_command without VERBATIM, and unfortunately ExternalProject_Add_Step does not have a VERBATIM argument. So it would break code here if either | or \; is used as separator because they have special meaning in shell.

For now, I use @ as separator. We can choose something different if @ is too common. I'm not familiar with how to test CMake flow here. So I just ran some CMake unit test to verify cmd parsing works as expected with/without VERBATIM mode on both linux/windows.

smeenai

Good point. @ seems good to me.

The build system is fragile by allowing multiple invocation of subprocess builds in the native folder for Make generator. For example, during sub-invocation of the native llvm-config, llvm-min-tblgen is also built. If there is another sub-invocation of the native llvm-min-tblgen build running in parallel, they may overwrite each other's build results, and may lead to errors like "Text file busy". This patch adds a cmake script that uses file lock to serialize all native builds for Make generator.

llvm-ci · 2025-01-16T12:30:41Z

LLVM Buildbot has detected a new failure on builder clang-ppc64-aix running on aix-ppc64 while building llvm at step 3 "clean-build-dir".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/1992

Here is the relevant piece of the build log for the reference

Step 3 (clean-build-dir) failure: Delete failed. (failure) (timed out)
Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang :: ClangScanDeps/verbose.test' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: rm -rf /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp
+ rm -rf /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp
RUN: at line 2: split-file /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/ClangScanDeps/verbose.test /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp
+ split-file /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/ClangScanDeps/verbose.test /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp
RUN: at line 3: sed -e "s|DIR|/home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp|g" /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/cdb.json.in > /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/cdb.json
+ sed -e 's|DIR|/home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp|g' /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/cdb.json.in
RUN: at line 5: /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/clang-scan-deps -compilation-database /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/cdb.json -v -o /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/result.json 2>&1 | /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/FileCheck /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/ClangScanDeps/verbose.test
+ /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/clang-scan-deps -compilation-database /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/cdb.json -v -o /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/ClangScanDeps/Output/verbose.test.tmp/result.json
+ /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/FileCheck /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/ClangScanDeps/verbose.test
/home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/ClangScanDeps/verbose.test:6:11: error: CHECK: expected string not found in input
// CHECK: *** Virtual File System Stats:
          ^
<stdin>:1:1: note: scanning from here
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
^
<stdin>:1:8: note: possible intended match here
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
       ^

Input file: <stdin>
Check file: /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/test/ClangScanDeps/verbose.test

-dump-input=help explains the following input dump.

Input was:
<<<<<<
           1: PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. 
check:6'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:6'1            ?                                                                                                     possible intended match
>>>>>>

--

********************

The build system is fragile by allowing multiple invocation of subprocess builds in the native folder for Make generator. For example, during sub-invocation of the native llvm-config, llvm-min-tblgen is also built. If there is another sub-invocation of the native llvm-min-tblgen build running in parallel, they may overwrite each other's build results, and may lead to errors like "Text file busy". This patch adds a cmake script that uses file lock to serialize all native builds for Make generator.

joker-eph approved these changes Dec 24, 2024

View reviewed changes

llvmbot added the cmake Build system in general and CMake in particular label Dec 25, 2024

arthurqiu force-pushed the aq/fix_native_llvm-config branch from b0dc686 to e6c555e Compare December 25, 2024 10:43

arthurqiu changed the title ~~[cmake] Fix overlapping native llvm-min-tblgen builds~~ [cmake] Serialize native builds for Make generator Dec 25, 2024

arthurqiu force-pushed the aq/fix_native_llvm-config branch from e6c555e to d3c3711 Compare December 25, 2024 10:55

arthurqiu requested a review from joker-eph January 2, 2025 01:55

llvm-beanz approved these changes Jan 7, 2025

View reviewed changes

llvm/cmake/modules/LLVMExternalProjectUtils.cmake Outdated Show resolved Hide resolved

arthurqiu force-pushed the aq/fix_native_llvm-config branch from e419a54 to add0b79 Compare January 8, 2025 08:15

smeenai reviewed Jan 13, 2025

View reviewed changes

arthurqiu force-pushed the aq/fix_native_llvm-config branch from add0b79 to 34023d6 Compare January 14, 2025 07:35

FileLock.cmake to serialize sub-make build for native targets

56be0d9

arthurqiu force-pushed the aq/fix_native_llvm-config branch from 34023d6 to 56be0d9 Compare January 14, 2025 08:32

arthurqiu requested a review from smeenai January 15, 2025 01:15

smeenai approved these changes Jan 15, 2025

View reviewed changes

smeenai merged commit 6655c53 into llvm:main Jan 15, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmake] Serialize native builds for Make generator #121021

[cmake] Serialize native builds for Make generator #121021

arthurqiu commented Dec 24, 2024 •

edited

Loading

joker-eph left a comment

arthurqiu commented Dec 24, 2024 •

edited

Loading

joker-eph commented Dec 24, 2024

arthurqiu commented Dec 25, 2024 •

edited

Loading

arthurqiu commented Dec 25, 2024

arthurqiu commented Jan 3, 2025

llvm-beanz left a comment

arthurqiu commented Jan 8, 2025

arthurqiu commented Jan 13, 2025

smeenai Jan 13, 2025

arthurqiu Jan 14, 2025 •

edited

Loading

arthurqiu Jan 14, 2025

smeenai left a comment

llvm-ci commented Jan 16, 2025

[cmake] Serialize native builds for Make generator #121021

[cmake] Serialize native builds for Make generator #121021

Conversation

arthurqiu commented Dec 24, 2024 • edited Loading

joker-eph left a comment

Choose a reason for hiding this comment

arthurqiu commented Dec 24, 2024 • edited Loading

joker-eph commented Dec 24, 2024

arthurqiu commented Dec 25, 2024 • edited Loading

arthurqiu commented Dec 25, 2024

arthurqiu commented Jan 3, 2025

llvm-beanz left a comment

Choose a reason for hiding this comment

arthurqiu commented Jan 8, 2025

arthurqiu commented Jan 13, 2025

smeenai Jan 13, 2025

Choose a reason for hiding this comment

arthurqiu Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

arthurqiu Jan 14, 2025

Choose a reason for hiding this comment

smeenai left a comment

Choose a reason for hiding this comment

llvm-ci commented Jan 16, 2025

arthurqiu commented Dec 24, 2024 •

edited

Loading

arthurqiu commented Dec 24, 2024 •

edited

Loading

arthurqiu commented Dec 25, 2024 •

edited

Loading

arthurqiu Jan 14, 2025 •

edited

Loading