Skip to content
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

"Alias" style typedefs are being omitted in bindgen for not having a definition, but then generating bad bindings #509

Open
kkukshtel opened this issue Dec 7, 2023 · 4 comments

Comments

@kkukshtel
Copy link
Contributor

I'm wrapping cimgui.h and one thing it does is typedef common types for its wrapped library (imgui) so they work in the "API" layer it builds. So things like:

typedef FILE* ImFileHandle;
typedef struct StbTexteditRow StbTexteditRow;
struct ImBitVector;
struct ImRect;
struct ImDrawDataBuilder;
struct ImDrawListSharedData;
struct ImGuiColorMod;
struct ImGuiContext;
struct ImGuiContextHook;
struct ImGuiDataVarInfo;

I don't totally understand it, and I think it all works for ClangSharp, but an issue I'm having specifically with the ImFileHandle typedef is that ClangSharp is skipping the typedef and (trying) to use FILE* directly, so instead of getting a struct sig like this:

struct ImGuiContext
{
    ...
    ImFileHandle LogFile;
    ...
}

It's generating

struct ImGuiContext
{
    ...
    __sFILE* LogFile;
    ...
}

And then the __sFILE* symbol isn't actually defined anywhere, so it's erroring.

Would love to know if there is any way around this or if I'm missing something — thanks!

@tannergooding
Copy link
Member

By default ClangSharp resolves down to the root type. This allows things like operators to work, avoids ABI issues, perf issues, etc.

There are command line switches such as --remap and --with-transparent-struct that allow users to override this and choose a better mapping for their own bindings where desirable.

It is expected that users override such typedefs where that is necessary, with ClangSharp handling only a few special cases like wchar_t, intptr_t, size_t, and other standard language defined typedefs itself.

@kkukshtel
Copy link
Contributor Author

This makes sense and I'll definitely try some of those flags, but also it seems like maybe there is a bug where if a typedef "hides" an underlying "normal" C++ type like FILE*, ClangSharp doesn't try to convert that underlying type to its blittable version but instead assumes that it is instead some other user-defined type. Also I'm not sure what the _S prefix is meant to mean in this case.

Basically I'd have assumed that if ClangSharp didn't like the typedef, it would still convert FILE* to IntPtr or some equivalent handle type.

@tannergooding
Copy link
Member

ClangSharp only handles a subset of very well-defined and common types, not all C/C++ defined types.

There are many cases, like FILE, where the type is defined by the spec but not most other information. For example, FILE is only ever meant to be accessed as FILE* and the actual shape of the underlying structure is implementation defined.

I'm open to additional types being recognized implicitly, but they need to be well reasoned about and with well understood behavior when encountered. FILE* would have to map to void* by default, for example. While something like fpos_t can't default to anything because some platforms use int32_t, some int64_t, some even allow it to vary based on macros.

The default remappings are currently handled here:

_remappedNames.Add("intptr_t", "nint");
_remappedNames.Add("ptrdiff_t", "nint");
_remappedNames.Add("size_t", "nuint");
_remappedNames.Add("uintptr_t", "nuint");

Basically I'd have assumed that if ClangSharp didn't like the typedef, it would still convert FILE* to IntPtr or some equivalent handle type.

The ClangSharp P/Invoke Generator is intentionally "unopinionated". That is, it intentionally does not describe a correct default behavior except for where that is trivially provable as correct. This pots a bit more onus on the user to ensure the headers are parsed correctly, but ultimately reduces bugs.

IntPtr is also, notably, not a handle type. It is an integer type that is the same width as a pointer (Int16 is 16-bits, Int32 is 32-bits, Int64 is 64-bits, IntPtr is ptr-bits, etc). It has historically been used in many of the same places as opaque handles out of convenience, but that does not make it "correct" and in many cases void* is a more correct type, particularly when viewing it from the ABI level and ensuring values are passed around correctly.

@kkukshtel
Copy link
Contributor Author

All makes sense! I also had converted locally FILE* to void*, and can just add it to my own remappings for now. As you said I
I'm not sure it makes sense to move it to the bindings proper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants