[BOLT] Fix LLVM_APPEND_VC_REV support

[paschalis-mpeis]

The CMake flag LLVM_APPEND_VC_REV can be passed when building BOLT a BOLT to prevent including a VC Revision. This patch enables this functionality.

Usage: -DLLVM_APPEND_VC_REV=OFF when running CMake.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Paschalis Mpeis (paschalis-mpeis)

Changes

The CMake flag LLVM_APPEND_VC_REV can be passed when building BOLT a BOLT to prevent including a VC Revision. This patch enables this functionality.

Usage: -DLLVM_APPEND_VC_REV=OFF when running CMake.

---

Full diff: https://github.com/llvm/llvm-project/pull/142410.diff

1 Files Affected:

• (modified) bolt/lib/Utils/CMakeLists.txt (+14-1)

diff --git a/bolt/lib/Utils/CMakeLists.txt b/bolt/lib/Utils/CMakeLists.txt
index efba6d54449d3..812c199846080 100644
--- a/bolt/lib/Utils/CMakeLists.txt
+++ b/bolt/lib/Utils/CMakeLists.txt
@@ -6,12 +6,25 @@ set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 
 set(generate_vcs_version_script "${LLVM_CMAKE_DIR}/GenerateVersionFromVCS.cmake")
 
+if(llvm_vc AND LLVM_APPEND_VC_REV)
+  set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+endif()
+if (LLVM_VC_REPOSITORY AND LLVM_VC_REVISION)
+  set(llvm_source_dir ${LLVM_SOURCE_DIR})
+  set(llvm_vc_repository ${LLVM_VC_REPOSITORY})
+  set(llvm_vc_revision ${LLVM_VC_REVISION})
+endif()
+if(bolt_vc AND LLVM_APPEND_VC_REV)
+  set(bolt_source_dir ${BOLT_SOURCE_DIR})
+endif()
+
 # Create custom target to generate the VC revision include.
 add_custom_command(OUTPUT "${version_inc}"
   DEPENDS "${llvm_vc}" "${bolt_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=BOLT"
+                           "-DLLVM_SOURCE_DIR=${llvm_source_dir}"
+                           "-DBOLT_SOURCE_DIR=${bolt_source_dir}"
                            "-DHEADER_FILE=${version_inc}"
-                           "-DBOLT_SOURCE_DIR=${BOLT_SOURCE_DIR}"
                            "-DLLVM_VC_REPOSITORY=${llvm_vc_repository}"
                            "-DLLVM_VC_REVISION=${llvm_vc_revision}"
                            "-DLLVM_FORCE_VC_REVISION=${LLVM_FORCE_VC_REVISION}"

[paschalis-mpeis]

Details regarding the NFC checks (llvm-zorg):

One of the nfc-test issues is due to a BOLT SHA mismatch. The problem isn't actually in the bolt_info section. That section does not end up in the final build/bin/llvm-bolt, regardless of whether LLVM_APPEND_VC_REV is used. For example:

# section not present
$ llvm-objdump -s -j .note.bolt_info build/bin/llvm-bolt


llvm-objdump: warning: .. not found in any input file

However, there is a mismatch , in a .rodata literal. This patch should fix it.

@aaupov, in the coming days I'll publish more patches, adjusting both bolt and llvm-zorg.

[aaupov]

We're supposed to set this flag on NFC builder:
https://github.com/llvm/llvm-zorg/blob/e7c3dc5f8ead5ba5ea8dd819dee243d87f65b375/buildbot/osuosl/master/config/builders.py#L2949

At a time of enabling, I verified that if the changes are not affecting BOLT, it's not rebuilt (which ensures that NFC checks are not triggered). Perhaps it was broken at some point, causing changes in the bolt binary.

[paschalis-mpeis]

Hey Amir,

We're supposed to set this flag on NFC builder:
https://github.com/llvm/llvm-zorg/blob/e7c3dc5f8ead5ba5ea8dd819dee243d87f65b375/buildbot/osuosl/master/config/builders.py#L2949

Yes, thanks a lot! That's what brought me to this patch.

At a time of enabling, I verified that if the changes are not affecting BOLT, it's not rebuilt (which ensures that NFC checks are not triggered). Perhaps it was broken at some point, causing changes in the bolt binary.

I believe this is what happened too. At some point there was a broader CMake change (the one that introduced lowercase llvm_vc_revision), that didn't make it properly to BOLT. And unfortunately, there is no straightforward way for us to add a test to cover for this.

Could you consider accepting this PR? Below I list some commands to help with reviewing:

---

gh pr checkout 142410 # get this patch

Before this patch:

git checkout HEAD~2

# emits sha (correct)
cmake -DLLVM_APPEND_VC_REV=ON ..
bolt --version | grep revision
BOLT revision b71255705bab3bba231c045a9fd101df438a7a72

# emits sha (wrong)
cmake -DLLVM_APPEND_VC_REV=OFF ..
bolt --version | grep revision
BOLT revision b71255705bab3bba231c045a9fd101df438a7a72

With this path:

git checkout 8c0ef06a044b9aa9e7c8c79d9331dc2a0a4fe37a

 # emits sha (correct)
cmake -DLLVM_APPEND_VC_REV=ON ..
bolt --version | grep revision
BOLT revision 8c0ef06a044b9aa9e7c8c79d9331dc2a0a4fe37a

 # emits unknown (correct)
cmake -DLLVM_APPEND_VC_REV=OFF ..
bolt --version | grep revision
BOLT revision <unknown>

[paschalis-mpeis]

This appears to have happened around #97130. Shortly before, around commit b037d0f this flag was respected.

There is no reliable way to check this within LLVM itself, but I will add a validation step on the buildbot to track whether the NFC comparisons are working as expected (ie the commit SHA as well the GNU IDs).
That could issue a warning (and not flunk). In terms of time it'll be essentially free.

[aaupov]

Approving to unblock. Please wait a couple of days if folks more familiar with VC_REV functionality want to chime in: @RossComputerGuy, @h-vetinari, @nico, @kwk

[h-vetinari]

Please wait a couple of days if folks more familiar with VC_REV functionality want to chime in:

Did you intend to ping me? I'm not familiar with that. 👀

In fact, I first completely misinterpreted this as refering to VC the MSVC toolchain (and wondering whether bolt started supporting windows), but it seems VC is version control:

llvm-project/llvm/cmake/modules/AddLLVM.cmake

Lines 2593 to 2600 in 6c1091e

	# If the sources at the given `path` are under version control, set `out_var`
	# to the the path of a file which will be modified when the VCS revision
	# changes, attempting to create that file if it does not exist; if no such
	# file exists and one cannot be created, instead set `out_var` to the
	# empty string.
	#
	# If the sources are not under version control, do not define `out_var`.
	function(find_first_existing_vc_file path out_var)

[paschalis-mpeis]

Thanks Amir. Sure, I'll give it a couple of more days before merging in case there are any further comments.

[kwk]

Approving to unblock. Please wait a couple of days if folks more familiar with VC_REV functionality want to chime in: @RossComputerGuy, @h-vetinari, @nico, @kwk

@h-vetinari did you intend to ping me?

[h-vetinari]

I didn't ping you @kwk; I'm in the same boat as you (i.e. I don't know why I was pinged, see my comment above)

[paschalis-mpeis]

Hey folks,

Let's wait for @aaupov to clear up the confusion.
Not sure about @h-vetinari, but I believe @kwk the rest were tagged as they have originally reviewed:

• [BOLT] Enable standalone build #97130

That PR brought some extra CMake configuration but inadvertently broke the LLVM_APPEND_VC_REV flag.
This patch fixes that.

EDIT: I see that @h-vetinari participated in the discussion of the above PR, so probably that is the reason why.

[aaupov]

Right, I've included the reviewers of #97130

[paschalis-mpeis]

Any objections to this PR?

I will add a validation step on the buildbot to track whether the NFC comparisons are working as expected (ie the commit SHA as well the GNU IDs).

Also, this is implemented by:

• [BOLT] Add nfc-check-validation step llvm-zorg#462

[aaupov]

I believe it's good to go since there were no objections in a week.