-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[clang-repl] Fix inconsistent flushing between in-process and out-of-process #166368
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-clang Author: Anutosh Bhat (anutosh491) ChangesFixes #166120 I could think of two approaches here
Yes this would make sense and work too Here's an unpolished diff where
Downsides of this approach is we end up parsing and executing the above block for each input and then undoing the execution too everytime. Which just looks heavy ! Full diff: https://github.com/llvm/llvm-project/pull/166368.diff 1 Files Affected:
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index cde354c9cd8d1..f3927fc341341 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -351,6 +351,15 @@ const char *const Runtimes = R"(
EXTERN_C void __clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType, ...);
)";
+const char *const OOPRuntimes = R"(
+ #include <stdio.h>
+ __attribute__((constructor))
+ static void __clang_repl_ioinit(void) {
+ setvbuf(stdout, NULL, _IONBF, 0);
+ setvbuf(stderr, NULL, _IONBF, 0);
+ }
+)";
+
llvm::Expected<std::pair<std::unique_ptr<llvm::orc::LLJITBuilder>, uint32_t>>
Interpreter::outOfProcessJITBuilder(JITConfig Config) {
std::unique_ptr<llvm::orc::ExecutorProcessControl> EPC;
@@ -463,6 +472,11 @@ Interpreter::create(std::unique_ptr<CompilerInstance> CI, JITConfig Config) {
if (auto E = Interp->ParseAndExecute(Runtimes))
return std::move(E);
+ if (Config.IsOutOfProcess) {
+ if (auto E = Interp->ParseAndExecute(OOPRuntimes))
+ return std::move(E);
+ }
+
Interp->markUserCodeStart();
return std::move(Interp);
|
|
Approach 2: As what the PR has, configures the executor’s output streams to be unbuffered once at startup, ensuring immediate flushing for all subsequent inputs. |
|
Some outputs The only difference here is the You can see 2 "clang-repl>"'s here. This is a separate in-process thing in itself (which needs to be looked into in the future) |
|
Hey @vgvassilev @Vipul-Cariappa , I guess we can start with something like this on the tool side. Added simple test logs at the start and the end of the file to confirm consistent flushing. So now we have the following (which is what in-process has too) instead of the following on master The test passes as expected |
| Interp = ExitOnErr(clang::Interpreter::create(std::move(CI), Config)); | ||
| } | ||
|
|
||
| if (Config.IsOutOfProcess) { |
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.
Should we condition this on out-of-process?
I am not sure, asking for an opinion.
I guess I am fine with everything else.
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.
Hmm, removing the condition would introduce some consistency (otherwise oop is maintaining 1 more PTU .... PTU#2 after the inital runtime).
Fixes #166120
I could think of two approaches here
Yes this would make sense and work too
Here's an unpolished diff where
Downsides of this approach is we end up parsing and executing the above block for each input and then undoing the execution too everytime. Which just looks heavy !