-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[Clang-Repl] Adds custom lambda in launchExecutor and PID retrieval #152562
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
Conversation
…into redirection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This patch broke the Solaris/amd64 bot. |
@kr-2003, we can probably disable the test for Solaris. |
I don't think this is right: the test |
Is there any way to reproduce this? |
This change is also breaking Bazel. After fixing the missing dependency in #152681, I'm now getting
which is raised in To reproduce, run |
I use this code for removing the extra dirs for calculating the executable paths:
here
ALso, I can see 2
So, for bazel, I think the folder structure is quite different than expected. |
It's likely adding the 2nd bin due to
I suspect? |
Ya changing that line to just llvm::sys::path::append(ExecutorPath, "llvm-jitlink-executor"); I then get
So still failing, but the expected path for bazel without the double |
So, its seems that llvm-project/clang/unittests/Interpreter/OutOfProcessInterpreterTests.cpp Lines 133 to 146 in 3ea76af
llvm-project/clang/unittests/Interpreter/CMakeLists.txt Lines 9 to 14 in 3ea76af
llvm-project/clang/unittests/Interpreter/CMakeLists.txt Lines 41 to 47 in 3ea76af
|
probably need to check for |
that file is part of the I'm not familiar really with what this PR is doing, could you take a look? |
If you aren't familiar with bazel I can assist |
Broadly speaking, bazel tests are much more explicit about paths, and you can make few assumptions about where things will be. A common fix is to have paths passed in as flags/env vars, or sometimes you can "copy" files (often just a symlink) so that it will be in the same dir. I think it's fine if we just skip this test for bazel for now. |
I've done a This is as wrong as possible:
I wonder how this test can ever have worked on any target but Linux/x86_64 (and macOS). The fact that the test doesn't fail on Please either remove the test or fix this ASAP: the builder has been red for 3 days now. |
Sure. I'll remove this test for Solaris for now. But, in the future work on including more platforms for JIT out-of-process execution. |
As I've explained, this is not a Solaris issue but one that will affect all non-macOS non-Linux/x86_64 targets. |
yeah. i actually meant to say that i will skip these tests for all non-macos and no-linux targets. |
The new check is certainly better since it explicitly excludes anything but macOS and Linux/x86_64 which are the only ones that are supported right now. However, the code still doesn't account for the two possible layouts of the runtime dir. Rather than hardcoding those things, it would be way better to just determine them at runtime just like |
There is no runtime path in Darwin because of the following: llvm-project/clang/lib/Driver/ToolChain.cpp Lines 1022 to 1024 in 48da848
So, we need to set it manually. if (SystemTriple.isOSBinFormatMachO()) {
llvm::sys::path::append(RuntimePath, "darwin", "liborc_rt_osx.a");
} else if (SystemTriple.isOSBinFormatELF()) {
llvm::sys::path::append(RuntimePath, "liborc_rt.a");
} |
This reverts commit eccc6e2.
This is still wrong: e.g. (as I wrote) Solaris uses |
…53180) Reverts #152562. Seems to be causing test failures on Solaris bot. Co-authored-by: kr-2003 <[email protected]>
This PR introduces: