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

[HIPIFY][build][clang][fix] Make hipify-clang buildable with an LLVM configured to link libLLVM.so #1840

Open
wants to merge 1 commit into
base: rocm-6.3.x
Choose a base branch
from

Conversation

stellaraccident
Copy link

@stellaraccident stellaraccident commented Jan 22, 2025

Without these changes, if building against an LLVM configured with -DLLVM_LINK_LLVM_DYLIB=ON, the resulting binary will have ODR violations resulting in the dread duplicate CL option registered errors at runtime.

In order to work in this case, executables must be uniformly linked in the same way that LLVM binaries themselves are: by setting LLVM_LINK_COMPONENTS vs depending directly on the (static) individual libraries. This should work in all linking modes.

Note that the problem originate because an LLVM configured in this way will configure any clang libraries to carry a transitive dep on libLLVM. Therefore, it is illegal to depend on clang (static) libraries and LLVM static libraries in combination.

There is a step further that could be taken to also support building against a shared libClang, but this is merely an optimization vs an error: since libClang is a leaf library, it is ok to depend on its static components. Perhaps total package size could be reduced by also supporting libClang, but this is left for the future.

Without these changes, if building against and LLVM configured with `-DLLVM_LINK_LLVM_DYLIB=ON`, the resulting binary will have ODR violations resulting in the dread duplicate CL option registered errors at runtime.

In order to work in this case, executables must be uniformly linked in the same way that LLVM binaries themselves are: by setting `LLVM_LINK_COMPONENTS` vs depending directly on the (static) individual libraries. This should work in all linking modes.

Note that the problem originate because an LLVM configured in this way will configure any clang libraries to carry a transitive dep on libLLVM. Therefore, it is illegal to depend on clang (static) libraries and LLVM static libraries in combination.

There is a step further that could be taken to also support building against a shared libClang, but this is merely an optimization vs an error: since libClang is a leaf library, it is ok to depend on its static components. Perhaps total package size could be reduced by also supporting libClang, but this is left for the future.
@stellaraccident stellaraccident changed the base branch from amd-staging to rocm-6.3.x January 22, 2025 01:45
@emankov emankov added fix It fixes bug clang clang compiler related issue or change build Building issue/fix compatibility Compatibility change or request labels Jan 22, 2025
@emankov emankov changed the title Make hipify-clang buildable with an LLVM configured to link libLLVM.so. [HIPIFY][build][clang][fix] Make hipify-clang buildable with an LLVM configured to link libLLVM.so Jan 22, 2025
@raramakr raramakr requested a review from lamb-j January 23, 2025 23:23
@emankov
Copy link
Collaborator

emankov commented Jan 27, 2025

Hi @stellaraccident,

First, thanks for your contribution to the project.
Second, we need some more time to check your fix.

Thanks!

@searlmc1
Copy link
Collaborator

Hi @stellaraccident ,

Posting the same comment from #1841 - is it critical to land this in the ROCm:rocm-6.3.x branch ? If so, we need to coordinate with ROCm-devOps as the source-of-truth for that branch is internal. Also, I assume this should land in the amd-staging branch. E.g., I assume this issue also exists in amd-staging / also needs to be fixed there.

Thanks much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Building issue/fix clang clang compiler related issue or change compatibility Compatibility change or request fix It fixes bug Under Investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants