MochiLibraries / Biohazrd

A framework for automatically generating binding wrappers for C/C++ libraries
MIT License
60 stars 9 forks source link

AttributedType should be reduced using EquivalentType instead of CanonicalType #130

Open PathogenDavid opened 3 years ago

PathogenDavid commented 3 years ago

(Continued from https://github.com/InfectedLibraries/Biohazrd/issues/124)

Originally I thought the only way to implement this feature was to use the canonical type. This type is not ideal because it erases typedefs, which means things like function pointers with a calling convention attribute involving typedefs will loose information.

Looking at the Clang documentation, it appears that what we really want is the equivalent type.

This property is exposed in newer versions of ClangSharp, but no NuGet packages exist for them yet.

We need to check to see how Clang handles situations where the equivalent type might not be representable. For example: AttributedType_CallingConventionOnTypedef_AllTypedefsRemoved tests

typedef void (*function_pointer_t)(int);
typedef __stdcall function_pointer_t function_pointer_stdcall_t;
void Test(function_pointer_stdcall_t function);

What is the EquivalentType of function_pointer_stdcall_t?

PathogenDavid commented 3 years ago

It turns out we've been unknowingly opting-in to dealing with these types:

https://github.com/InfectedLibraries/Biohazrd/blob/f986212c7c463c6f0ef81755a0aaeb99a330821f/Biohazrd/TranslatedLibraryBuilder.cs#L219

If this flag isn't present, the only difference is that libclang will automatically resolve AttributedType using the equivalent type:

https://github.com/InfectedLibraries/llvm-project/blob/6d5c430eb3c0bd49f6f5bda4b0d2d8aa79b0fa3f/clang/tools/libclang/CXType.cpp#L129-L136

I think this flag was enabled with the thought that it was needed for getting the deprecated attributes for https://github.com/InfectedLibraries/Biohazrd/issues/87. That may be true, but for now they're more of an annoyance than they're worth. If it turns out this flag is needed for https://github.com/InfectedLibraries/Biohazrd/issues/87, we can resolve this issue when it is implemented.

For now I am going to disable this flag, add a note, and add a Debug.Fail to the AttributedType handling.

PathogenDavid commented 3 years ago

For whatever reason, disabling this flag eliminates typedef types in some situations for reasons I can't really fathom. For instance, looking at this code from AttributedType_CallingConventionOnTypedef_NoTypedefRemoved:

typedef void (*function_pointer_t)(int);
typedef __stdcall function_pointer_t function_pointer_stdcall_t;
void Test(function_pointer_stdcall_t function);

Without removing any typedefs I'd expect the following to have a function Test with a parameter function with a TypedefType referencing function_pointer_stdcall_t.

With CXTranslationUnit_IncludeAttributedTypes enabled, that is what you get. Unfortunately without the flag, it becomes a function Test with a parameter function with a PointerType of FunctionProtoType of void (int) [stdcall]

Looking at the libclang source code, I can't really see why this would be. I think this bug is less impactful than the bugs caused by this issue so I'm still going to disable this flag as a workaround for this issue and resolve it properly later on.

PathogenDavid commented 3 years ago

Also worth mentioning that I disabled this flag and checked the impact on InfectedPhysX and InfectedImGui. Neither had any codegen or diagnostic differences so I don't think this flag was really enabled intentionally.

PathogenDavid commented 3 years ago

Workaround implemented in 8f179a30531cb063128887d1aa607178b62c09cd, leaving this issue open until we can properly resolve it due to the typedef issue.