Thoughts on `hs-bindgen`'s support for flexible array memebrs

[jorisdral]

For a struct with a FLAM such as this, with an accompanying function:

struct vector {
  int len;
  char data[];
};

static int reverse (const struct vector * input, struct vector * output) {
  int i;
  int n = input->len;
  if (n != output->len) return -1;
  for (i = 0; i < n; i++)
    output->data[i] = input->data[n-1-i];
  return 0;
};

We generate roughly the following Haskell bindings:

data Vector = Vector { vector_len :: FC.CInt }

instance F.Storable Vector where
  sizeOf = \_ -> (4 :: Int)
  alignment = \_ -> (4 :: Int)
  peek = \ptr ->Vector <$> peekCField (Proxy @"vector_len") ptr
  poke = \ptr -> Vector x ->pokeCField (Proxy @"vector_len") ptr x

instance HasFlexibleArrayMember CChar Vector where
  flexibleArrayMemberOffset = \_ty0 -> 4

instance HasCField Vector "vector_len" where
  type CFieldType Vector "vector_len" = CInt
  offset# = \_ -> \_ -> 0

instance ( ty ~ CFieldType Vector "vector_len")
         ) => HasField "vector_len" (Ptr Vector) (Ptr ty) where
  getField = ptrToCField (Proxy @"vector_len")

foreign import ... reverse :: Ptr Vector -> Ptr Vector -> IO CInt

Note in particular that the Vector datatype only stores the vector length, not the incomplete array. Moreover, the reverse foreign import requires passing Ptr Vector. It's not unthinkable that someone would try to call reverse as follows:

foo = do
  with (Vector 3) $ \(input :: Ptr Vector) ->
    with (Vector 3) $ \(output :: Ptr Vector) -> do
      reverse input output
      peek output >>= print

This possibly leads to a segfault because of undefined behaviour The input and output vector say they have length 3 but the FLAMs themselves are not initialised, so the reverse function accesses memory that is out of bounds.

Instead, this seems the way to invoke reverse correctly:

instance HasFlexibleArrayLength CChar Gen.Vector where
  flexibleArrayMemberLength x = fromIntegral (Gen.vector_len x)

foo = do
  let sz = sizeOf (undefined  :: Vector) + 3 * sizeOf (undefined :: CChar)
  allocaBytes sz $ \input ->
    allocaBytes sz $ \output -> do
      poke input (Vector 3)
      pokeWithFLAM input (WithFlexibleArrayMember (Vector 3) (fromList [1,2,3]))
      poke output (Vector 3)
      ret <- reverse input output
      print ret
      peekWithFLAM output >>= print

This feels a bit on the arduous side. I think we should add more utilities for marshalling structs with FLAMs.

Also, should the foreign import use Ptr (WithFlexibleArrayMember CChar Vector) instead of Ptr Vector? Or should Vector include a vector_data field, with proper utilities to construct Haskell Vectors and allocate Ptr Vector with the right number of elements in the FLAM? I'm leaning towards the second option in favour of removing WithFlexibleArrayMember, but I could be overlooking reasons why we'd want to keep it. Yes, we would also lose a Storable instance for Vector, but it is error-prone to use that Storable instance anyway.

Note: the C reference at https://en.cppreference.com/w/c/language/struct.html mentions a bunch of restrictions for FLAMs, but gcc and clang allow some constructions anyway. For example, having an array of structs with FLAMs should be disallowed, but gcc and clang happily compile this:

struct vector { int len; int data[]; };
struct vectors { struct vector[3] };

Should hs-bindgen disallow this construction if the C reference disallows it, even if gcc and clang don't complain?

Zero-copy bindings

How we support FLAMs also affects zero-copy bindings that we support now since #1247. I had initially implemented zero-copy bindings for structs with a FLAM such that it would generate (in addition the bindings listed at the start of this post):

instance HasCField Vector "vector_data" where
  type CFieldType Vector "vector_data" = IncompleteArray FC.CChar
  offset# = \_ -> \_ -> 4

instance ( ty ~ CFieldType Vector "vector_data" )
         ) => HasField "vector_data" (Ptr Vector) (Ptr ty) where
  getField = ptrToCField (Proxy @"vector_data")

This feels intuitive to me: it follows the structure of the vector struct. There is a len field and data field and both are accessed the same way. Still, depending on how we decide to support FLAMs going forward, the generated bindings might have to change. Hence, #1247 does not generate zero-copy bindings for FLAMs, and we should add support for this as part of resolving #1286 instead. The written manual and executable manual sections should also be updated.

Below is the code that can be included in HsBindgen.Backend.Hs.Translation to generate these bindings.

Implementation

structDecs ... =
    ....
    structFlamDecls structName (C.structFlam struct)

-- | 'HasCField' and 'HasField' instances for a FLAM field of a struct
-- declaration
--
-- Given a struct:
--
-- > struct myStruct { int len; char data[] };
--
-- We generate roughly this datatype:
--
-- > newtype MyStruct = MyStruct { myStruct_len :: CInt }
--
-- Then, 'structFlamDecls' will generate roughly the following class instances
-- for the fields @len@ and @data@ respectively:
--
-- > instance HasCField "myStruct_len" MyStruct where
-- >   type CFieldType "myStruct_len" MyStruct = CInt
-- > instance HasField "myStruct_len" (Ptr MyStruct) (Ptr CInt)
--
-- > instance HasCField "myStruct_data" MyStruct where
-- >   type CFieldType "myStruct_data" MyStruct = IncompleteArray CChar
-- > instance HasField "myStruct_data" (Ptr MyStruct) (Ptr (IncompleteArray CChar))
structFlamDecls :: Hs.Name Hs.NsTypeConstr -> Maybe C.StructField -> [Hs.Decl]
structFlamDecls structName =
    maybe [] $ \f ->
      structFieldDecls structName $
        -- For a FLAM struct field, the field type stores the array /element/
        -- type. For the HasField-related decls, we need to re-introduce the
        -- array type as well.
        f { C.structFieldType = C.TypeIncompleteArray (C.structFieldType f) }