Failing FFI example

[lePereT]

Hi! Thanks for this lib. Here's some code that I'm porting that implements a ring buffer. It errors on cffi but not luajit/other pure lua ffi implementations:

-- Use of this source code is governed by the Apache 2.0 license; see COPYING.

-- Ring buffer for bytes

local test_ffi_lib = 'ffi' -- 'cffi' or 'ffi'

local ffi = require(test_ffi_lib)
local bit = require("bit32")

local band = bit.band

ffi.cdef [[
   typedef struct {
      uint32_t read_idx, write_idx;
      uint32_t size;
      uint8_t buf[?];
   } buffer_t;
]]

local function to_uint32(n)
   return ffi.new('uint32_t[1]', n)[0]
end

local function new(size)
   local ret = buffer_t(size)
   ret:init(size)
   return ret
end

local buffer = {}
buffer.__index = buffer

function buffer:init(size)
   assert(size ~= 0 and band(size, size - 1) == 0, "size not power of two")
   self.size = size
   return self
end

function buffer:reset()
   self.write_idx, self.read_idx = 0, 0
end

function buffer:is_empty()
   return self.write_idx == self.read_idx
end
function buffer:read_avail()
   return to_uint32(self.write_idx - self.read_idx)
end
function buffer:is_full()
   return self:read_avail() == self.size
end
function buffer:write_avail()
   return self.size - self:read_avail()
end

function buffer:write_pos()
   return band(self.write_idx, self.size - 1)
end
function buffer:rewrite_pos(offset)
   return band(self.read_idx + offset, self.size - 1)
end
function buffer:read_pos()
   return band(self.read_idx, self.size - 1)
end

function buffer:advance_write(count)
   self.write_idx = self.write_idx + count
end
function buffer:advance_read(count)
   self.read_idx = self.read_idx + count
end

function buffer:write(bytes, count)
   if count > self:write_avail() then error('write xrun') end
   local pos = self:write_pos()
   local count1 = math.min(self.size - pos, count)
   ffi.copy(self.buf + pos, bytes, count1)
   ffi.copy(self.buf, bytes + count1, count - count1)
   self:advance_write(count)
end

function buffer:rewrite(offset, bytes, count)
   if offset + count > self:read_avail() then error('rewrite xrun') end
   local pos = self:rewrite_pos(offset)
   local count1 = math.min(self.size - pos, count)
   ffi.copy(self.buf + pos, bytes, count1)
   ffi.copy(self.buf, bytes + count1, count - count1)
end

function buffer:read(bytes, count)
   if count > self:read_avail() then error('read xrun') end
   local pos = self:read_pos()
   local count1 = math.min(self.size - pos, count)
   ffi.copy(bytes, self.buf + pos, count1)
   ffi.copy(bytes + count1, self.buf, count - count1)
   self:advance_read(count)
end

function buffer:drop(count)
   if count > self:read_avail() then error('read xrun') end
   self:advance_read(count)
end

function buffer:peek()
   local pos = self:read_pos()
   return self.buf + pos, math.min(self:read_avail(), self.size - pos)
end

buffer_t = ffi.metatype("buffer_t", buffer)

local function selftest()
   print('selftest: lib.buffer')
   local function assert_throws(f, ...)
      local success, ret = pcall(f, ...)
      assert(not success, "expected failure but got "..tostring(ret))
   end
   local function assert_avail(b, readable, writable)
      assert(b:read_avail() == readable)
      assert(b:write_avail() == writable)
   end
   local function write_str(b, str)
      local scratch = ffi.new('uint8_t[?]', #str)
      ffi.copy(scratch, str, #str)
      b:write(scratch, #str)
   end
   local function read_str(b, count)
      local scratch = ffi.new('uint8_t[?]', count)
      b:read(scratch, count)
      return ffi.string(scratch, count)
   end

   assert_throws(new, 10)
   local b = new(16)
   assert_avail(b, 0, 16)
   for i = 1,10 do
      local s = '0123456789'
      write_str(b, s)
      assert_avail(b, #s, 16-#s)
      assert(read_str(b, #s) == s)
      assert_avail(b, 0, 16)
   end

   local ptr, avail = b:peek()
   assert(avail == 0)
   write_str(b, "foo")
   local ptr, avail = b:peek()
   assert(avail > 0)

   -- Test wrap of indices.
   local s = "overflow"
   b.read_idx = to_uint32(3 - #s)
   b.write_idx = b.read_idx
   assert_avail(b, 0, 16)
   write_str(b, s)
   assert_avail(b, #s, 16-#s)
   assert(read_str(b, #s) == s)
   assert_avail(b, 0, 16)

   print('selftest: ok')
end

return {
    new = new,
    selftest = selftest
}

With luajit's ffi or https://github.com/jmckaskill/luaffi I get the following:

$ lua
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio
> buffer = require 'buffer'; buffer.selftest()
selftest: lib.buffer
selftest: ok
> 

But with cffi the selftest() causes the interpreter to abort

$ lua
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio
> buffer = require 'buffer'; buffer.selftest()
selftest: lib.buffer
free(): invalid size
Aborted
$

[q66]

i'm not sure how you got it to work to that point, as broken handling of variable-length structs (https://github.com/q66/cffi-lua/commit/c9fa529ac5c33ff18b1d5fb63df323f5e956e0d1) would prevent it from even allocating properly (it would initialize read_idx to 16 and leave the buffer empty, leading to assertion errors)

that said, after that fix, i was able to reproduce your condition and pushed some fixes, can you test that on your side?

[lePereT]

Could it be the Arm64 architecture that I'm using? Will test asap :)

[q66]

there is also an issue i just discovered; cdata on most platforms must be 16-aligned, but lua_newuserdata returns pointers that are 8-aligned; in practice this will usually not manifest (hence no failing tests in CI even on platforms with strict alignment requirements) but UBsan (rightly) complains about that (so i need to figure out some way to fix it, and probably add UBsan to CI)

the obvious way is to overallocate by 16 bytes and then align the pointer (while making sure to adjust any potential calls to userdata rawlen, because that needs the original pointer and may return wrong size), but that's both a waste and bug-prone, so i'll need to think about this

[q66]

and no, i don't think so

[lePereT]

Hey q66 can confirm that your fixes enable the tests to work, as well as other code that relies on the library. Thank you! Unless you wanna keep this issue open for the lua_newuserdata alignment issue, happy to close.

(I've two other short questions but I'll make separate issues to help discoverability).

On Wed, 4 Jan 2023, 17:33 q66, @.***> wrote:

and no, i don't think so

— Reply to this email directly, view it on GitHub https://github.com/q66/cffi-lua/issues/36#issuecomment-1371221859, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFUW5ENNQEKCEBWGOMQIMTWQWX53ANCNFSM6AAAAAATQNRASU . You are receiving this because you authored the thread.Message ID: @.***>

[q66]

landed the big memory management rework, so i will close this