Skip to content

[clang-repl] Fix target creation in Wasm.cpp #130909

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

Merged
merged 3 commits into from
Mar 12, 2025
Merged

Conversation

anutosh491
Copy link
Member

@anutosh491 anutosh491 commented Mar 12, 2025

After #129868 went in, I realize some updates have been made to the Triple.

Not sure if @nikic overlooked including this change in his PR (hence I was having build issue when compiling clang against emscripten while building for wasm)

But yeah just like the change was addressed in other files, we need to make the change here too I think

const llvm::Target *Target = llvm::TargetRegistry::lookupTarget(
PTU.TheModule->getTargetTriple(), Error);
if (!Target)
return llvm::make_error<llvm::StringError>(std::move(Error),
std::error_code());
llvm::TargetOptions TO = llvm::TargetOptions();
llvm::TargetMachine *TargetMachine = Target->createTargetMachine(
PTU.TheModule->getTargetTriple().str(), TargetOpts.CPU, "", TO,

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 12, 2025

@llvm/pr-subscribers-clang

Author: Anutosh Bhat (anutosh491)

Changes

After #129868 went in, I realize some updates have been made to the Triple.

Not sure if @nikic overlooked including this change in his PR (hence I have having build issue when compiling clang against emscripten while building for wasm)

But yeah just like the change was addressed in other files, we need to make the change here too I think

const llvm::Target *Target = llvm::TargetRegistry::lookupTarget(
PTU.TheModule->getTargetTriple(), Error);
if (!Target)
return llvm::make_error<llvm::StringError>(std::move(Error),
std::error_code());
llvm::TargetOptions TO = llvm::TargetOptions();
llvm::TargetMachine *TargetMachine = Target->createTargetMachine(
PTU.TheModule->getTargetTriple().str(), TargetOpts.CPU, "", TO,


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

1 Files Affected:

  • (modified) clang/lib/Interpreter/Wasm.cpp (+2-2)
diff --git a/clang/lib/Interpreter/Wasm.cpp b/clang/lib/Interpreter/Wasm.cpp
index aa10b160ccf84..c12412c26d6e0 100644
--- a/clang/lib/Interpreter/Wasm.cpp
+++ b/clang/lib/Interpreter/Wasm.cpp
@@ -74,7 +74,7 @@ llvm::Error WasmIncrementalExecutor::addModule(PartialTranslationUnit &PTU) {
 
   llvm::TargetOptions TO = llvm::TargetOptions();
   llvm::TargetMachine *TargetMachine = Target->createTargetMachine(
-      PTU.TheModule->getTargetTriple(), "", "", TO, llvm::Reloc::Model::PIC_);
+      PTU.TheModule->getTargetTriple().str(), "", "", TO, llvm::Reloc::Model::PIC_);
   PTU.TheModule->setDataLayout(TargetMachine->createDataLayout());
   std::string ObjectFileName = PTU.TheModule->getName().str() + ".o";
   std::string BinaryFileName = PTU.TheModule->getName().str() + ".wasm";
@@ -146,4 +146,4 @@ llvm::Error WasmIncrementalExecutor::cleanUp() {
 
 WasmIncrementalExecutor::~WasmIncrementalExecutor() = default;
 
-} // namespace clang
\ No newline at end of file
+} // namespace clang

Copy link

github-actions bot commented Mar 12, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@nikic nikic merged commit 369c0a7 into llvm:main Mar 12, 2025
9 of 10 checks passed
@anutosh491 anutosh491 deleted the target_wasm branch March 12, 2025 09:28
@anutosh491
Copy link
Member Author

/cherry-pick 369c0a7

@anutosh491
Copy link
Member Author

anutosh491 commented Mar 12, 2025

Cherry picking this as it ends up being a major blocker for running clang-repl in the browser while using llvm 20. Would be great to have this picked up and addressed by 20.0.2 (whenever that is out)

@nikic
Copy link
Contributor

nikic commented Mar 12, 2025

The relevant change is only in LLVM 21, so I have no idea how this can possibly be a blocker for LLVM 20. Are you testing the correct version?

@llvmbot
Copy link
Member

llvmbot commented Mar 12, 2025

/cherry-pick 369c0a7

Error: Command failed due to missing milestone.

@anutosh491
Copy link
Member Author

so I have no idea how this can possibly be a blocker for LLVM 20. Are you testing the correct version?

Oops yes, I ended up missing that completely ! Was testing the wrong version.
Yes, should be good to go with respect to llvm 20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants