Jit oop exec dummy #5
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ///\returns 0 on success, non-zero on failure. | ||
| CPPINTEROP_API int Undo(unsigned N = 1); | ||
|
|
||
| CPPINTEROP_API pid_t GetExecutorPID(); |
There was a problem hiding this comment.
warning: no header providing "pid_t" is directly included [misc-include-cleaner]
include/CppInterOp/CppInterOp.h:21:
- #include <vector>
+ #include <sys/types.h>
+ #include <vector>|
|
||
| #include "llvm/Support/Error.h" | ||
|
|
||
| #include <vector> |
There was a problem hiding this comment.
warning: #includes are not sorted properly [llvm-include-order]
#include <vector>
^this fix will not be applied because it overlaps with another fix
|
|
||
| #include <vector> | ||
| #include <unistd.h> | ||
|
|
There was a problem hiding this comment.
warning: included header unistd.h is not used directly [misc-include-cleaner]
|
|
||
| namespace compat { | ||
|
|
||
| static int m_child_stdout_fd = -1; |
There was a problem hiding this comment.
warning: variable 'm_child_stdout_fd' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static int m_child_stdout_fd = -1;
^| namespace compat { | ||
|
|
||
| static int m_child_stdout_fd = -1; | ||
| static int m_child_stderr_fd = -1; |
There was a problem hiding this comment.
warning: variable 'm_child_stderr_fd' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static int m_child_stderr_fd = -1;
^| Interpreter(std::unique_ptr<clang::Interpreter> CI) : inner(std::move(CI)) {} | ||
|
|
||
| static void drainChildOutput() { | ||
| char buffer[4096]; |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
char buffer[4096];
^|
|
||
| static void drainChildOutput() { | ||
| char buffer[4096]; | ||
| ssize_t bytes_read; |
There was a problem hiding this comment.
warning: no header providing "ssize_t" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOpInterpreter.h:25:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <sys/types.h>
+ #if CLANG_VERSION_MAJOR >= 19|
|
||
| if (compat::m_child_stdout_fd != -1) { | ||
| int flags = fcntl(compat::m_child_stdout_fd, F_GETFL, 0); | ||
| fcntl(compat::m_child_stdout_fd, F_SETFL, flags | O_NONBLOCK); |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
fcntl(compat::m_child_stdout_fd, F_SETFL, flags | O_NONBLOCK);
^| int flags = fcntl(compat::m_child_stdout_fd, F_GETFL, 0); | ||
| fcntl(compat::m_child_stdout_fd, F_SETFL, flags | O_NONBLOCK); | ||
|
|
||
| while ((bytes_read = read(compat::m_child_stdout_fd, buffer, |
There was a problem hiding this comment.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
while ((bytes_read = read(compat::m_child_stdout_fd, buffer,
^|
|
||
| while ((bytes_read = read(compat::m_child_stdout_fd, buffer, | ||
| sizeof(buffer))) > 0) { | ||
| fwrite(buffer, 1, bytes_read, stdout); |
There was a problem hiding this comment.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
fwrite(buffer, 1, bytes_read, stdout);
^
| ```bash | ||
| mkdir build | ||
| cd build | ||
| cmake -DLLVM_ENABLE_PROJECTS="clang;compiler-rt \ |
There was a problem hiding this comment.
| cmake -DLLVM_ENABLE_PROJECTS="clang;compiler-rt \ | |
| cmake -DLLVM_ENABLE_PROJECTS="clang;compiler-rt" \ |
| mkdir build | ||
| cd build | ||
| cmake -DLLVM_ENABLE_PROJECTS="clang;compiler-rt \ |
| ../llvm | ||
| # For Linux | ||
| cmake --build . --target clang clang-repl llvm-jitlink-executor orc_rt --parallel $(nproc --all) |
There was a problem hiding this comment.
In my system (Linux - X86-64) the target is:
| cmake --build . --target clang clang-repl llvm-jitlink-executor orc_rt --parallel $(nproc --all) | |
| cmake --build . --target clang clang-repl llvm-jitlink-executor orc_rt-x86_64 --parallel $(nproc --all) |
|
|
||
| CPPINTEROP_API pid_t GetExecutorPID(); | ||
|
|
||
| // CPPINTEROP_API void SendInputToChild(const std::string& input); |
There was a problem hiding this comment.
If this is not yet implemented, it should not be part of the PR. Even as a comment.
|
|
||
| EPC = ExitOnError( | ||
| launchExecutor(OOPExecutor, UseSharedMemory, SlabAllocateSizeString, | ||
| /*stdin_pipe[0]*/0, stdout_pipe[1], stderr_pipe[1])); |
There was a problem hiding this comment.
Why are you not yet using the stdin pipe?
| +void drainChildOutput() { | ||
| + char buffer[4096]; | ||
| + ssize_t bytes_read; | ||
| + | ||
| + // Drain stdout pipe | ||
| + if (g_child_stdout_fd != -1) { | ||
| + // Make non-blocking to avoid hanging if no data | ||
| + int flags = fcntl(g_child_stdout_fd, F_GETFL, 0); | ||
| + fcntl(g_child_stdout_fd, F_SETFL, flags | O_NONBLOCK); | ||
| + | ||
| + while ((bytes_read = read(g_child_stdout_fd, buffer, sizeof(buffer))) > 0) { | ||
| + fwrite(buffer, 1, bytes_read, stdout); | ||
| + } | ||
| + fflush(stdout); | ||
| + } | ||
| + | ||
| + // Drain stderr pipe | ||
| + if (g_child_stderr_fd != -1) { | ||
| + int flags = fcntl(g_child_stderr_fd, F_GETFL, 0); | ||
| + fcntl(g_child_stderr_fd, F_SETFL, flags | O_NONBLOCK); | ||
| + | ||
| + while ((bytes_read = read(g_child_stderr_fd, buffer, sizeof(buffer))) > 0) { | ||
| + fwrite(buffer, 1, bytes_read, stderr); | ||
| + } | ||
| + fflush(stderr); | ||
| + } | ||
| +} |
There was a problem hiding this comment.
Why is this defined here? I believe this should be part of CppInterOp and not LLVM. Am I missing something?
| + std::cout << "OOPExecutor: " << OOPExecutor << std::endl; | ||
| + std::cout << "UseSharedMemory: " << UseSharedMemory << std::endl; | ||
| + std::cout << "SlabAllocateSizeString: " << SlabAllocateSizeString << std::endl; |
| + int stdout_pipe[2], stderr_pipe[2]; | ||
| + if (pipe(stdout_pipe) != 0 || pipe(stderr_pipe) != 0) { | ||
| + llvm::errs() << "Failed to create pipes for child process I/O\n"; | ||
| + return 0; | ||
| + } | ||
| + EPC = ExitOnErr( | ||
| + launchExecutor(OOPExecutor, UseSharedMemory, SlabAllocateSizeString, 0, stdout_pipe[1], stderr_pipe[1])); | ||
| + |
There was a problem hiding this comment.
We should not create any pipes for the clang-repl case. It should just follow the defaults.
| } | ||
|
|
||
| Input = ""; | ||
| + drainChildOutput(); |
There was a problem hiding this comment.
You will need to regenerate this patch file by taking a diff with master, as some of these are merged.
|
You should also consider the clang-tidy (github-actions bot) comments. |
2 participants
Description
Please include a summary of changes, motivation and context for this PR.
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist