TheDan64 / inkwell

It's a New Kind of Wrapper for Exposing LLVM (Safely)
https://thedan64.github.io/inkwell/
Apache License 2.0
2.36k stars 228 forks source link

Extern functions are not detected by Kaleidoscope JIT #74

Closed OlegTheCat closed 5 years ago

OlegTheCat commented 5 years ago

Describe the Bug As stated in the comment extern functions defined in Rust code are not detected by Kaleidoscope JIT.

To Reproduce

  1. Run Kaleidoscope REPL.
  2. Type extern printd(x)
  3. Type printd(10)
  4. Function returns NaN

Expected Behavior printd function should have returned passed value and printed it.

LLVM Version (please complete the following information):

Desktop (please complete the following information):

Additional Context After a bit of investigation, it seems that there are a couple of problems here:

  1. rustc will mangle the names of those function unless #[no_mangle] attribute is applied.
  2. Unless those functions are used in the Rust code, the compiler will remove them. To avoid this they should be called somewhere or put into a static array with #[used] attribute like here.
  3. Currently, body that returns NaN is set for extern functions. This causes that functions that are defined in Rust are being overridden by those that return NaN (strangely, but functions like sin and cos are not being overridden). It seems that if we got extern definition, we should not attach any body to it.

I kinda fixed these issues in my fork, but it will segfault if extern function, that is not actually defined, is called. I'm not sure what is desired behavior here. Let me know what do you think about this.

TheDan64 commented 5 years ago

@71 As the original author of this code, any thoughts on this?

71 commented 5 years ago

I remember struggling to make extern functions work, before giving up after trying several different things. I do remember trying to add #[no_mangle], but am not sure I took the necessary steps to ensure the functions were not removed by the Rust compiler. I'm glad you managed to make it work!

I feel like the segfault behavior is more of an Inkwell problem than a Kaleidoscope problem, but even then finding a suitable solution is hard. Maybe documenting some function, type or module suitably to explain the different steps that must be taken to make extern functions work will be a good first step, since wrapping user code to avoid segfaults in practically impossible.

Regarding the fix itself, I think the choice to make body an Option is the right one.

TheDan64 commented 5 years ago

Shouldn't Kaleidoscope be using External Linkage?

https://github.com/TheDan64/inkwell/blob/4630dd2a5b214fc879dfa43e56eedbb9de875432/src/module.rs#L82-L84

I don't believe it does

TheDan64 commented 5 years ago

@OlegTheCat Any luck with your fork? We'd be happy to accept a PR if you can get it fully working

TheDan64 commented 5 years ago

Thanks for taking a look at this! Did you try the C version of kaleidoscope?

On Apr 19, 2019, 8:05 AM, at 8:05 AM, Oleh Palianytsia notifications@github.com wrote:

So, the C++ version seems not to segfault, but the issue is that it uses different API for JITting (which is old and deprecated right not, btw). I'm currently struggling to map it to the new API through inkwell <-> llvm-sys <-> llvm-c <-> llvm-c++ chain. It seems I'll need more time to investigate this.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/TheDan64/inkwell/issues/74#issuecomment-484873417

OlegTheCat commented 5 years ago

I'm pasting my previous comment here. Sorry for the confusion.

So, the C++ version seems not to segfault, but the issue is that it uses different API for JITting (which is old and deprecated right not, btw). I'm currently struggling to map it to the new API through inkwell <-> llvm-sys <-> llvm-c <-> llvm-c++ chain. It seems I'll need more time to investigate this.

OlegTheCat commented 5 years ago

Regarding the C version of Kaleidoscope, I didn't know it exists, to be honest. Can you point me where to look? As far as I can see there are two official implementations of Kaleidoscope: in C++ and in OCaml.

TheDan64 commented 5 years ago

Sorry, it seems you're right that there isn't an official C Kaleidoscope - however I was thinking there's gotta be some out there that might be worth checking out? In which case we could get a close comparison to inkwell since we're also using the C api.

Maybe this one? https://github.com/benbjohnson/llvm-c-kaleidoscope

TheDan64 commented 5 years ago

Also, I'm guessing the OCaml version uses the C api behind the scenes?

OlegTheCat commented 5 years ago

Yeah, right. I'll check the OCaml version then.

OlegTheCat commented 5 years ago

Hi, sorry for the delay. I've finally got to this.

I've examined the official OCaml version and it turned out that it is heavily outdated. I've given up after several hours of trying to compile it.

Also, I've tried the C implementation you suggested, and it segfaults on unimplemented function too:

$ ./kaleidoscope
ready > extern foo()

declare double @foo()
ready > foo()

define double @0() {
entry:
  %calltmp = call double @foo()
  ret double %calltmp
}
Segmentation fault: 11

So, it seems to be expected behavior.

TheDan64 commented 5 years ago

Alright, thanks for taking a look!

TheDan64 commented 5 years ago

Fixed by #76

naalit commented 4 years ago

This seems to be a problem again, at least on LLVM 10.0.0. When I try

?> extern printd(x)

?> printd(10)

in Kaleidoscope, it segfaults. nm doesn't see the printd symbol, but even when I add #[inline(never)] and call it from main(), so nm sees it again, it still segfaults.

Edit: it seems using ExecutionEngine::add_global_mapping() does work, though.