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

Fix CMakeLists.txt to allow building when LLVM backend is configured … #200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hrtan99
Copy link

@hrtan99 hrtan99 commented Jun 22, 2024

…other than x86 and arm.

Although llvm-mctoll currently only support x86 and arm, it should be built correctly when the backend of llvm project is configured of architectures other than x86 and arm.

Due to the following codes in mctoll, the linker will report undefined symbol for the MC layer components of other machine arch.

  // Initialize targets and assembly printers/parsers.
  llvm::InitializeAllTargets();
  llvm::InitializeAllTargetInfos();
  llvm::InitializeAllTargetMCs();
  llvm::InitializeAllDisassemblers();

We can use ${LLVM_TARGETS_TO_BUILD} in CMakeLists file to avoid the error, though it includes unnecessary components of other architectures.

@hrtan99 hrtan99 requested a review from bharadwajy as a code owner June 22, 2024 09:08
Copy link
Contributor

@aaronsm aaronsm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bharadwajy bharadwajy left a comment

Choose a reason for hiding this comment

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

There is no value in building llvm-mctoll for targets other than those it intends to support. Configuring as such in the publicly available repo would indicate that it is supported for targets other than x64 and ARM. Additionally, as is pointed out this change would include unnecessary components in the resulting binary and the needless complexity of adding ${LLVM_TARGETS_TO_BUILD} for no additional benefit.

The change in this PR can be made locally by anyone that wishes to build it on unsupported targets - and does so with an understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants