Re: [Tarantool-patches] [PATCH luajit] FFI: Shrink container of packed bitfield.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Evgeniy!
Thanks for the review!
Please, see my answers below.

On 28.07.26, Evgeniy Temirgaleev wrote:
> Hi, Sergey! Thanks for the patch!
> 
> LGTM after updating the test. I tried the test and it passed without the fix.

You need to build LuaJIT with ASAN support, see the comment below.

> 
> To check container bit size calculation we can add a field at bit 16. It’s byte offset with invalid calculation will be 0 and it will be 2 with the fixed calculation.

Nice idea, I like it.

> 
> The output of the test with check printing:
> 
> Without the fix:
> 
> TAP version 13
> 1..2
> XX-R: 4
> ok - correct 0-initialization
> XX: 4
> XX-R: 4
> ok - bitfield set correctly
> # align=2 sz=4
> # bitfield ofs=0 bitofs=0 bitsize=1
> # bitfield15 ofs=0 bitofs=1 bitsize=15
> # bitfield16 ofs=0 bitofs=16 bitsize=1
> 
> With the fix:
> 
> TAP version 13
> 1..2
> XX-R: 2
> ok - correct 0-initialization
> XX: 2
> XX-R: 2
> ok - bitfield set correctly
> # align=2 sz=4
> # bitfield ofs=0 bitofs=0 bitsize=1
> # bitfield15 ofs=0 bitofs=1 bitsize=15
> # bitfield16 ofs=2 bitofs=0 bitsize=1
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua b/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
> index 07e53f76..bdd5c69e 100644
> --- a/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
> +++ b/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
> @@ -12,6 +12,8 @@ ffi.cdef[[
> #pragma pack(push, 2)
> typedef struct {
> unsigned int bitfield:1;
> +  unsigned int bitfield15:15;
> +  unsigned int bitfield16:1;
> } packed_struct;
> #pragma pack(pop)
> ]]
> @@ -25,4 +27,16 @@ test:is(packed.bitfield, 0, 'correct 0-initialization')
> packed.bitfield = 1
> test:is(packed.bitfield, 1, 'bitfield set correctly')
> 
> +local ps = ffi.typeof('packed_struct')
> +test:diag('align=%d sz=%d', ffi.alignof(ps), ffi.sizeof(ps))
> +local o, bo, bs
> +o, bo, bs = ffi.offsetof(ps, 'bitfield')
> +test:diag('bitfield ofs=%d bitofs=%d bitsize=%d', o, bo, bs)
> +o, bo, bs = ffi.offsetof(ps, 'bitfield15')
> +test:diag('bitfield15 ofs=%d bitofs=%d bitsize=%d', o, bo, bs)
> +o, bo, bs = ffi.offsetof(ps, 'bitfield16')
> +test:diag('bitfield16 ofs=%d bitofs=%d bitsize=%d', o, bo, bs)
> +
> +test:fail()
> +
> test:done(true)

The test fails if run under ASAN. Build LuaJIT with its support like the
following:

| cmake -DLUAJIT_ENABLE_GC64=ON -DLUAJIT_USE_SYSMALLOC=ON -DLUAJIT_USE_ASAN=ON -DLUAJIT_USE_UBSAN=ON -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON . && make -j

But I like the idea to check memory layout of the given structure. See
the iterative patch below:

===================================================================
diff --git a/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua b/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
index 07e53f76..76483b0d 100644
--- a/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
+++ b/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
@@ -10,13 +10,21 @@ local ffi = require('ffi')
 
 ffi.cdef[[
 #pragma pack(push, 2)
+
 typedef struct {
   unsigned int bitfield:1;
 } packed_struct;
+
+typedef struct {
+  unsigned int bitfield0:1;
+  unsigned int bitfield15:15;
+  unsigned int bitfield16:1;
+} packed_struct2;
+
 #pragma pack(pop)
 ]]
 
-test:plan(2)
+test:plan(5)
 
 local packed = ffi.new('packed_struct')
 
@@ -25,4 +33,10 @@ test:is(packed.bitfield, 0, 'correct 0-initialization')
 packed.bitfield = 1
 test:is(packed.bitfield, 1, 'bitfield set correctly')
 
+-- Check correct structure layout.
+local byteoffset, bitpos, bitsize = ffi.offsetof('packed_struct2', 'bitfield16')
+test:is(byteoffset, 2, 'byteoffset is correct')
+test:is(bitpos, 0, 'bitpos is correct')
+test:is(bitsize, 1, 'bitsize is correct')
+
 test:done(true)
===================================================================

Branch is force-pushed.
> 
> --
> Best regards,
> Evgeniy Temirgaleev
> 
> > 

<snipped>

> > b/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
> > new file mode 100644
> > index 00000000..07e53f76
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1451-ffi-packed-bitfield.test.lua
> > @@ -0,0 +1,28 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate LuaJIT's incorrect behaviour of the
> > +-- `#pragma` pack directive for bitfields in structures.
> > +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1451.
> > +
> > +local test = tap.test('lj-1451-ffi-packed-bitfield')
> > +
> > +local ffi = require('ffi')
> > +
> > +ffi.cdef[[
> > +#pragma pack(push, 2)
> > +typedef struct {
> > + unsigned int bitfield:1;
> > +} packed_struct;
> > +#pragma pack(pop)
> > +]]
> > +
> > +test:plan(2)
> > +
> > +local packed = ffi.new('packed_struct')
> > +
> > +test:is(packed.bitfield, 0, 'correct 0-initialization')
> > +
> > +packed.bitfield = 1
> > +test:is(packed.bitfield, 1, 'bitfield set correctly')
> > +
> > +test:done(true)
> > --
> > 2.55.0
> >

-- 
Best regards,
Sergey Kaplun