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

Redirect calls to printf and fprintf #315

Merged
merged 1 commit into from Mar 19, 2020
Merged

Redirect calls to printf and fprintf #315

merged 1 commit into from Mar 19, 2020

Conversation

hahnjo
Copy link
Contributor

@hahnjo hahnjo commented Mar 19, 2020

When the interpreted code calls an external function, LLVM will use
dlsym() to find the target in the current process. This works well
for functions from libc which are always loaded.
However this also outputs the result of printf() and fprintf() to the
terminal and does not forward it to the user. This is very unintuitive
because values piped into std::cout and std::cerr correctly appear in
the browser.

As a solution, use llvm::sys::DynamicLibrary::AddSymbol() to inject
custom implementations into the process of finding external functions.
Once hooked up, the implementations format the passed arguments into
a C++ string and forward the result to the C++ streams that correctly
forward to the user.

Fixes #112

When the interpreted code calls an external function, LLVM will use
dlsym() to find the target in the current process. This works well
for functions from libc which are always loaded.
However this also outputs the result of printf() and fprintf() to the
terminal and does not forward it to the user. This is very unintuitive
because values piped into std::cout and std::cerr correctly appear in
the browser.

As a solution, use llvm::sys::DynamicLibrary::AddSymbol() to inject
custom implementations into the process of finding external functions.
Once hooked up, the implementations format the passed arguments into
a C++ string and forward the result to the C++ streams that correctly
forward to the user.

Fixes #112
@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Mar 19, 2020

This is great!

@SylvainCorlay SylvainCorlay merged commit cf704c6 into jupyter-xeus:master Mar 19, 2020
2 checks passed
@JohanMabille
Copy link
Member

@JohanMabille JohanMabille commented Mar 19, 2020

Thanks a lot!

@hahnjo hahnjo deleted the jit-printf branch Mar 19, 2020
@SimeonEhrig
Copy link
Contributor

@SimeonEhrig SimeonEhrig commented Mar 23, 2020

@hahnjo
Sorry for the delay. I was very busy last week. Great Work!

From PR #293

As far as I know, it's undefined behavior to redefine standard functions like printf. Instead I've independently developed a different solution in #315 that injects custom implementations (very similar to this PR) into the process of calling external functions from interpreted code.

Yes, it's similar to my idea, but you solved my last problem, the os dependency, in a really clever way.

I have also a possible improvement for your implementation. When, I use fprintf(stdout, "Hello World!\n"); it prints to the terminal, but I would expect, that it prints to the browser. It's just a small fix.

Thank you very much for your work. It helps me a lot with our old school developers ;-)

@hahnjo
Copy link
Contributor Author

@hahnjo hahnjo commented Mar 23, 2020

@SimeonEhrig

I have also a possible improvement for your implementation. When, I use fprintf(stdout, "Hello World!\n"); it prints to the terminal, but I would expect, that it prints to the browser. It's just a small fix.

Yes, the code in fprintf_jit should likely handle the case of stream == stdout, good thought.

@hahnjo
Copy link
Contributor Author

@hahnjo hahnjo commented Mar 23, 2020

I have also a possible improvement for your implementation. When, I use fprintf(stdout, "Hello World!\n"); it prints to the terminal, but I would expect, that it prints to the browser. It's just a small fix.

Yes, the code in fprintf_jit should likely handle the case of stream == stdout, good thought.

See #320 for the obvious follow-up.

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.

4 participants