Skip to content

ffigen generating Pointer<Int> instead of Array<...> #438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tvolkert opened this issue Nov 6, 2023 · 8 comments
Closed

ffigen generating Pointer<Int> instead of Array<...> #438

tvolkert opened this issue Nov 6, 2023 · 8 comments

Comments

@tvolkert
Copy link

tvolkert commented Nov 6, 2023

https://github.com/libexif/libexif/blob/master/libexif/exif-data.h#L47 contains the following struct:

/*! Represents the entire EXIF data found in an image */
struct _ExifData
{
	/*! Data for each IFD */
	ExifContent *ifd[EXIF_IFD_COUNT];

	/*! Pointer to thumbnail image, or NULL if not available */
	unsigned char *data;

	/*! Number of bytes in thumbnail image at \c data */
	unsigned int size;

	ExifDataPrivate *priv;
};

When you run it through ffigen, it generates the following binding code:

/// ! Represents the entire EXIF data found in an image
final class _ExifData extends ffi.Struct {
  /// ! Data for each IFD
  external ffi.Pointer<ffi.Int> ifd;

  /// ! Pointer to thumbnail image, or NULL if not available
  external ffi.Pointer<ffi.UnsignedChar> data;

  /// ! Number of bytes in thumbnail image at \c data
  @ffi.UnsignedInt()
  external int size;

  external ffi.Pointer<ExifDataPrivate> priv;
}

I found it weird that ExifContent *ifd[EXIF_IFD_COUNT] translated to external ffi.Pointer<ffi.Int> ifd, and it made it hard to get at the element I wanted... I tried the following:

final Pointer<ExifContent> nativeContent = foo.elementAt(ExifIfd.EXIF_IFD_GPS).cast<ExifContent>();

... but the program crashed whenever I tried to use that nativeContent pointer.

So I then tried to write a simple C library that distilled this into a case I could study more. Here is that code:

typedef struct _TestData        TestData;
typedef struct _TestContent     TestContent;
typedef struct _TestEntry       TestEntry;

typedef enum {
	IFD_0 = 0,
	IFD_1,
	IFD_EXIF,
	IFD_GPS,
	IFD_INTEROPERABILITY,
	IFD_COUNT
} Ifd;

struct _TestData
{
	TestContent *ifd[IFD_COUNT];

	unsigned int size;
};

struct _TestContent
{
        TestEntry **entries;
        unsigned int count;

	TestData *parent;
};

struct _TestEntry {
        unsigned int value;

	TestContent *parent;
};

TestData *test_data_new (void);

I then ran that through ffigen, figuring that the TestContent *ifd[IFD_COUNT] would again turn into external ffi.Pointer<ffi.Int> ifd.... but it didn't. It instead generated the following binding code:

final class _TestData extends ffi.Struct {
  @ffi.Array.multi([5])
  external ffi.Array<ffi.Pointer<TestContent>> ifd;

  @ffi.UnsignedInt()
  external int size;
}

So on the one hand, this generated code makes much more sense! But on the other hand, why didn't it do this for the libexif code??

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 6, 2023

Thanks for the report @tvolkert!

ExifContent *ifd[EXIF_IFD_COUNT]; should indeed be generated to a fixed length Array<Pointer<...>>.

@mannprerak2 Any chance you could take a look?

@mannprerak2
Copy link
Contributor

Sure I'll take a look today.

@mannprerak2
Copy link
Contributor

Looks like a case of missing headers.

I could replicate the same behaviour when the constant for array length is not defined, libclang corrects it to just a pointer.

@tvolkert ffigen should be printing out some logs with SEVERE prefix, you should look into those. You can use compiler opts to pass the include header directory (See compiler-opts in ffigen readme)

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 6, 2023

Thanks @mannprerak2! 🙏

This is not the first time users are ignoring SEVERE, I wonder if we should refuse to generate the dart bindings file (and delete it if it already exists) on SEVERE errors.

@mannprerak2
Copy link
Contributor

I agree, perhaps we can refuse to generate the file initially, then prompt user to fix the errors or pass in a flag such as --ignore-errors.

Or a slightly non breaking way would be just the last log informing that the generated file might contain some invalid bindings.

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 6, 2023

Or a slightly non breaking way would be just the last log informing that the generated file might contain some invalid bindings.

Users are just going to ignore it again. We should force users to fix the errors or to manually ignore them with a flag.

I agree, perhaps we can refuse to generate the file initially, then prompt user to fix the errors or pass in a flag such as --ignore-errors.

👍

@tvolkert Can you confirm that adding the right include paths solves your issue?

@tvolkert
Copy link
Author

tvolkert commented Nov 8, 2023

Yep, confirmed that adding a compiler-opts directive in my config fixed this issue. Thanks!

And yes, I agree that we should fail to generate the bindings if there's an error - people are just going to ignore stdout messages (especially when they're so verbose)

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 8, 2023

Tracking refusing to generate in https://github.com/dart-lang/ffigen/issues/640.

@dcharkes dcharkes closed this as completed Nov 8, 2023
@liamappelbe liamappelbe transferred this issue from dart-archive/ffigen Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants