ponylang / ponyc

Pony is an open-source, actor-model, capabilities-secure, high performance programming language
http://www.ponylang.io
BSD 2-Clause "Simplified" License
5.71k stars 415 forks source link

LLVM compiler abort for FFI call to pony_alloc. #388

Closed jemc closed 8 years ago

jemc commented 8 years ago

My pony-sodium package (FFI wrappers for the libsodium crypto library) was working with ponyc version 0.1.7, but with latest ponyc master, I get LLVM compiler abort when trying to compile the tests. I think this error is probably related to an FFI call, but I'm not sure.

Building . -> /home/jemc/1/code/hg/pony-sodium/sodium/test
Building builtin -> /usr/local/lib/pony/0.2.1-123-g766a2d8/packages/builtin
Building ponytest -> /usr/local/lib/pony/0.2.1-123-g766a2d8/packages/ponytest
Building .. -> /home/jemc/1/code/hg/pony-sodium/sodium
Generating
ponyc: Instructions.cpp:276: void llvm::CallInst::init(llvm::Value *, ArrayRef<llvm::Value *>, const llvm::Twine &): Assertion `(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && "Calling a function with bad signature!"' failed.
./run.sh: line 3:  9522 Aborted                 (core dumped) ponyc --debug

I haven't been able to whittle this down to a smaller example, but you can reproduce by cloning my pony-sodium repository and compiling the sodium/test package:

git clone https://github.com/jemc/pony-sodium.git
cd pony-sodium
cd sodium/test && ponyc
jemc commented 8 years ago

I've narrowed it to a more minimal reproduce that doesn't require the full package.

You should be able to reproduce with this snipped (provided you have libsodium installed).

I'm on Fedora 22 Linux, if that helps.


use "lib:sodium"
use "ponytest"

primitive CryptoHash
  fun tag sha512_size(): USize => @crypto_hash_sha512_bytes[USize]().usize()

  fun tag _make_buffer(size: USize): String iso^ =>
    recover String.from_cstring(@pony_alloc[Pointer[U8]](size), size) end

  fun tag sha512(m: String): String =>
    let buf = _make_buffer(sha512_size())
    @crypto_hash_sha512[U32](buf.cstring(), m.cstring(), m.size())
    consume buf

class CryptoHashTest is UnitTest
  new iso create() => None
  fun name(): String => "sodium.CryptoHash"

  fun apply(h: TestHelper): TestResult =>
    let message = "My message to be hashed by the hashing function."
    let sha512_hash = CryptoHash.sha512(message)
    true

actor Main is TestList
  new create(env: Env) => PonyTest(env, this)
  new make() => None

  fun tag tests(test: PonyTest) =>
    test(CryptoHashTest)
jemc commented 8 years ago

Strike that, it appears that it's breaking on the call to pony_alloc, which means you don't need libsodium at all to reproduce the issue:

actor Main
  new create(env: Env) =>
    let size: USize = 10
    let buf: String = recover
      String.from_cstring(@pony_alloc[Pointer[U8]](size), size)
    end
Building builtin -> /home/jemc/1/code/gitx/ponyc/packages/builtin
Building . -> /home/jemc/1/code/gitx/ponyc/test/jemc
Generating
ponyc: Instructions.cpp:276: void llvm::CallInst::init(llvm::Value *, ArrayRef<llvm::Value *>, const llvm::Twine &): Assertion `(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && "Calling a function with bad signature!"' failed.
./run.sh: line 6: 14148 Aborted                 (core dumped) ../../build/debug/ponyc --debug
sylvanc commented 8 years ago

Two things:

  1. pony_alloc takes two arguments, not one. That's why it's crashing. The first argument is a pony_ctx_t which you can get by calling @pony_ctx_t[Pointer[None]]().
  2. Why are you doing String.from_cstring on an undefined buffer? It won't necessarily be null terminated, so this is likely to crash eventually.

I think what you actually want is:

primitive CryptoHash
  fun tag sha512_size(): USize => @crypto_hash_sha512_bytes[USize]().usize()

  fun tag _make_buffer(size: USize): String iso^ =>
    recover String.reserve(size) end

  fun tag sha512(m: String): String =>
    let len = sha512_size()
    let buf = _make_buffer(len)
    @crypto_hash_sha512[U32](buf.cstring(), m.cstring(), m.size())
    buf.truncate(len)
    consume buf

Of course, you are then treating the SHA512 as a String, which I suspect isn't right. You may want instead to do this:

primitive CryptoHash
  fun tag sha512_size(): USize => @crypto_hash_sha512_bytes[USize]().usize()

  fun tag _make_buffer(size: USize): Array[U8] iso^ =>
    recover Array[U8].undefined(size) end

  fun tag sha512(m: ByteSeq): Array[U8] iso^ =>
    let buf = _make_buffer(sha512_size())
    @crypto_hash_sha512[U32](buf.cstring(), m.cstring(), m.size())
    consume buf
jemc commented 8 years ago

pony_alloc takes two arguments, not one. That's why it's crashing. The first argument is a pony_ctx_t which you can get by calling @pony_ctx_t[Pointer[None]]().

Ah, I see. I wrote this code before this change, which added the second argument: https://github.com/CausalityLtd/ponyc/commit/24a58e0387392a2d6bef16a543979d47f3812341

Why are you doing String.from_cstring on an undefined buffer? It won't necessarily be null terminated, so this is likely to crash eventually.

It's okay that the buffer is undefined - The crypto_hash_sha512 function fills this buffer, expecting it to be freshly allocated.

I'm not sure what your point about the null terminator is. The String class works fine with non-terminated data because it is length-specified (tracks the size as a separate field), and everywhere I pass it to FFI, the function I pass it to has a separate parameter for size, so it's always length-specified as well (including here).

The reason I used pony_alloc instead of creating a new String or Array[U8] val of a given size, is because I wanted to avoid the overhead of zeroing the memory when I knew I was filling the buffer with the crypto function before returning it to the caller. This doesn't matter much for the SHA512 buffer, which will be small, but for some of the other functions which take large chunks of application data (like CryptoBox), the difference could be significant.

As far as using Array[U8] val instead of String - I don't remember exactly what the issue was, but there was some problem that made the Array[U8] val harder to work with down the line. I opted to use String instead because it was easier to use when it came to that problem, and should work fine as a byte buffer, given that it works with length-specified (and not necessarily null-terminated) bytes.

jemc commented 8 years ago

All that said, adding the first argument as suggested doesn't actually fix the problem:

actor Main
  new create(env: Env) =>
    let size: USize = 10
    let buf: String = recover
      String.from_cstring(@pony_alloc[Pointer[U8]](@pony_ctx_t[Pointer[None]](), size), size)
    end
Building builtin -> /home/jemc/1/code/gitx/ponyc/packages/builtin
Building . -> /home/jemc/1/code/gitx/ponyc/test/jemc
Generating
ponyc: Instructions.cpp:281: void llvm::CallInst::init(llvm::Value *, ArrayRef<llvm::Value *>, const llvm::Twine &): Assertion `(i >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType()) && "Calling a function with a bad signature!"' failed.
./run.sh: line 6: 23779 Aborted                 (core dumped) ../../build/debug/ponyc --debug
andymcn commented 8 years ago

The Array.undefined() constructor explicitly does not zero its contents and is provided exactly to avoid that overhead in situations like this.

The call to @pony_ctx_t should actually be @pony_ctx, @sylvanc typed it wrong. With this fix in @jemc's last example works for me (and without it I just get a pony_ctx_t not found link error).

Can you check your compiler build is up to date, what version of LLVM you've got, etc.

jemc commented 8 years ago

The Array.undefined() constructor explicitly does not zero its contents and is provided exactly to avoid that overhead in situations like this.

Right, I now remember using that originally. But since I'm aiming for a String here, I would need a way to convert the final Array[U8] into a string in a zero-copy way. String.from_cstring won't work because I would need to get my hands on the Pointer[U8] ref of the Array[U8], which I can't get from outside the builtin package.

The call to @pony_ctx_t should actually be @pony_ctx, @sylvanc typed it wrong. With this fix in @jemc's last example works for me (and without it I just get a pony_ctx_t not found link error).

Yes, that works now. Thanks.

Can you check your compiler build is up to date, what version of LLVM you've got, etc.

 ponyc --version
0.2.1-183-g26b6b38

 llvm-config36 --version
3.6.1