gnzlbg / ctest

Automatic testing of FFI bindings for Rust
https://docs.rs/ctest
Apache License 2.0
122 stars 29 forks source link

Skip type and roundtrip tests for function types #85

Open valtog opened 5 years ago

valtog commented 5 years ago

Function types can mean pointers in Rust, but in C they are not interchangeable. This causes jemallocator to fail its test.

gnzlbg commented 5 years ago

@valtog thanks for the PR, what do you mean that they are not interchangeable ? Could you describe which problem does this solve ?

valtog commented 5 years ago

@gnzlbg Function pointers are like pointers (with some exception), so you can do

struct {
  void* (*f)(int a);
};

but function types are not: this doesn't compile (because C is not object-oriented)

struct {
  void* (f)(int a);
};

Likewise, this code generated by ctest

typedef struct {
  unsigned char c;
  extent_alloc_t v;
} type;

with extent_alloc_t defined in jemalloc as

typedef void *(extent_alloc_t)(extent_hooks_t *, void *, size_t, size_t, bool *,
  bool *, unsigned);

doesn't compile.

You might think of converting them to function pointers. But we don't even need to test function pointers, because it doesn't make sense to allocate or align them.

I want to fix this because I see jemallocator is stuck with an outdated version partly because of it (gnzlbg/jemallocator#130). I'm happy to tackle any other blocker for that PR.

gnzlbg commented 5 years ago

I'm not sure how this will fix that. When parsing a struct containing a function type in the jemalloc crate, we just see a struct Foo { x: extent_alloc_t } and have no idea whether extent_alloc_t is a function pointer or not at that point.

I think that the right fix for this is to add a cfg.is_fn_ty(|x| x == "extent_alloc_t"); method to the ctest builder, and then use that information when generating the tests.

valtog commented 5 years ago

@gnzlbg

(Never mind) > I'm not sure how this will fix that. I've tested this with the `jemallocator` PR. > have no idea whether extent_alloc_t is a function pointer I was proposing to remove tests for both.

I think that the right fix for this is to add a cfg.is_fn_ty(|x| x == "extent_alloc_t")

Well I'm fine with that too.

valtog commented 5 years ago

On second thought, we don't need cfg.is_fn_ty because we can detect that ourselves. See #86.

gnzlbg commented 5 years ago

Hi valtog, I’ll take a deep look at this tomorrow and will try to get jemallocator working again.