Skip to content
This repository was archived by the owner on Jan 28, 2024. It is now read-only.

Support for passing/returning Structs by value. #134

Merged
merged 5 commits into from
Jan 8, 2021

Conversation

mannprerak2
Copy link
Contributor

Closes dart-lang/native#540

  • Added support for passing/returning structs by value.
  • Added, updated tests, examples.
  • Updated changelog, readme, version.

@mannprerak2 mannprerak2 requested a review from dcharkes January 8, 2021 09:02
@mannprerak2
Copy link
Contributor Author

mannprerak2 commented Jan 8, 2021

@dcharkes Hey, I wanted to know what will happen if we pass a struct (with an incomplete definition by value), but the function was not really expecting that struct
E.g - suppose I call a function - void func(Struct1 s);. But Struct1 is supposed to be

struct Struct1{
  int a;
  int b;
};

But instead, suppose I remove int b, what happens then? And what can possibly happen due to these sort of errors?

Currently, I am making sure to not generate any functions which pass/return these incomplete structs by value. Should I be doing this?

@dcharkes
Copy link
Contributor

dcharkes commented Jan 8, 2021

I'm assuming C would give a static error on trying to do something with an incomplete type? If that is the case, then not generating the functions and emitting a warning/error is the right approach.

Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to get rid of lib/src/clang_library/wrapper.c in this CL right?

@mannprerak2
Copy link
Contributor Author

We should be able to get rid of lib/src/clang_library/wrapper.c in this CL right?

We can but I think we should do that in another PR.

Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a smaller PR than I would have expected. 👍

lgtm

You might want to make the next PR which removes c wrapper just to dogfood test this PR before we merge it.

@mannprerak2
Copy link
Contributor Author

mannprerak2 commented Jan 8, 2021

I'm assuming C would give a static error on trying to do something with an incomplete type? If that is the case, then not generating the functions and emitting a warning/error is the right approach.

I meant, that sometimes when we remove all struct members if any of the members are unsupported (like bitfield).
So the struct we generate is empty. So in that case when a user passes this struct by value, how does the dart VM act on this?

E.g for this struct

struct S{
  int a;
  int b:4;
}

we generate this class S extend ffi.Struct{}.

But this can't be passed by value, so I am skipping any function which uses this struct by value.

@mannprerak2
Copy link
Contributor Author

mannprerak2 commented Jan 8, 2021

You might want to make the next PR which removes C wrapper just to dogfood test this PR before we merge it.

Okay sure.

@dcharkes
Copy link
Contributor

dcharkes commented Jan 8, 2021

I'm assuming C would give a static error on trying to do something with an incomplete type? If that is the case, then not generating the functions and emitting a warning/error is the right approach.

I meant, that sometimes when we remove all struct members if any of the members are unsupported (like bitfield).
So the struct we generate is empty. So in that case when a user passes this struct by value, how does the dart VM act on this?

That would result in segfaults because we would be talking about different registers/stack values. However, empty structs are undefined behavior on C, so dart:ffi rejects empty structs in asFunction and fromFunction. So if you would continue generating, you would generate code that would then be rejected by the Dart compiler.

@mannprerak2 mannprerak2 mentioned this pull request Jan 8, 2021
2 tasks
@mannprerak2 mannprerak2 requested a review from dcharkes January 8, 2021 11:45
@dcharkes dcharkes merged commit 0b68594 into dart-archive:dart-dev Jan 8, 2021
mannprerak2 added a commit to mannprerak2/ffigen that referenced this pull request Mar 1, 2021
* Added support for structs by value, fixed tests and example
* updated version, readme, changelog
* Added more tests
* Added struct return by value to native test
* Fix struct by value in nested typedefs
@mannprerak2 mannprerak2 deleted the struct-by-value branch March 1, 2021 12:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants