Skip to content

[CMake] Respect LLVM instrumentation variables in AddPureSwift.cmake #71135

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 31, 2024
Merged

[CMake] Respect LLVM instrumentation variables in AddPureSwift.cmake #71135

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cmake/modules/AddPureSwift.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,23 @@ function(add_pure_swift_host_library name)
)
endif()

# This replicates he code existing in LLVM llvm/cmake/modules/HandleLLVMOptions.cmake
# The second part of the clause replicates the LINKER_IS_LLD_LINK of the
# original.
if(LLVM_BUILD_INSTRUMENTED AND NOT (SWIFT_COMPILER_IS_MSVC_LIKE AND SWIFT_USE_LINKER STREQUAL "lld"))
Copy link
Member

Choose a reason for hiding this comment

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

Why is LLD a requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code does not explain why those conditions are needed. The original commit that introduced the conditions doesn't explain it either: swiftlang/llvm-project@a84b200

The stable/20230725 code is in https://github.com/apple/llvm-project/blob/stable/20230725/llvm/cmake/modules/HandleLLVMOptions.cmake#L1095-L1133

The linker options are only added for cases that are NOT LINKER_IS_LLD_LINK

The values of LINKER_IS_LLD_LINK comes from https://github.com/apple/llvm-project/blob/stable/20230725/llvm/cmake/modules/HandleLLVMOptions.cmake#L20-L30

If I had read the things correctly, the requirements for LINKER_IS_LLD_LINK is that we are using MSVC (either clang-cl or the actual cl) and using LLD to link, which if I had translated correctly should happen when SWIFT_COMPILER_IS_MSVC_LIKE and when SWIFT_USE_LINKER is lld.

Copy link
Member

Choose a reason for hiding this comment

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

Am I just failing to apply DeMorgan's Law? NOT (MSVC AND LLD) -> NOT MSVC OR NOT LLD. It would make sense as AND (NOT MSVC OR LLD).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see if I remember doing these things:

MSVC_LIKE = SWIFT_COMPILER_IS_MSVC_LIKE
LLD = SWIFT_USE_LINKER STREQUAL "lld"
| MSVC_LIKE | LLD | NOT (MSVC AND LLD) | NOT MSVC OR LLD |
| T         | T   | F                  | T               |
| T         | F   | T                  | F               |
| F         | T   | T                  | T               |
| F         | F   | T                  | T               |

For the version that I think I implemented, the only F is when MSVC and LLD are both T, which I think is the case in the original code. For the proposed version the case when MSVC and LLD is T, and the case of using MSVC but not LLD is F, which I think is the opposite of what we want.

PD: logic is hard, let's go shopping, and all the normal disclosures when doing booleans.

string(TOUPPER "${LLVM_BUILD_INSTRUMENTED}" uppercase_LLVM_BUILD_INSTRUMENTED)
if(LLVM_ENABLE_IR_PGO OR uppercase_LLVM_BUILD_INSTRUMENTED STREQUAL "IR")
target_link_options(${name} PRIVATE
"SHELL:-Xclang-linker -fprofile-generate=\"${LLVM_PROFILE_DATA_DIR}\"")
elseif(uppercase_LLVM_BUILD_INSTRUMENTED STREQUAL "CSIR")
target_link_options(${name} PRIVATE
"SHELL:-Xclang-linker -fcs-profile-generate=\"${LLVM_CSPROFILE_DATA_DIR}\"")
else()
target_link_options(${name} PRIVATE
"SHELL:-Xclang-linker -fprofile-instr-generate=\"${LLVM_PROFILE_FILE_PATTERN}\"")
endif()
endif()

# Export this target.
set_property(GLOBAL APPEND PROPERTY SWIFT_EXPORTS ${name})
endfunction()
Expand Down