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

Provide an first-class mechanism for emitting trampolines #236

Closed
28 of 38 tasks
PathogenDavid opened this issue Feb 12, 2022 · 0 comments
Closed
28 of 38 tasks

Provide an first-class mechanism for emitting trampolines #236

PathogenDavid opened this issue Feb 12, 2022 · 0 comments
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Area-Trampolines Issues concerning the trampolines API Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output Language-C# Issues specific to C#

Comments

@PathogenDavid
Copy link
Member

PathogenDavid commented Feb 12, 2022

Right now developing custom trampolines (such as ImGuiStringFunctionWrapperDeclaration) is slightly brittle because you end up taking a dependency on how Biohazrd emitted its own trampoline.

For instance, while working on #235 I broke most of the string trampolines generated for Dear ImGui since they didn't know to start emitting things as byrefs.

We should support an extensible API for developing trampoline methods. This could also be used to simplify the spaghetti mess that is the current output generation for functions.

Good candidates to use the new API:

Since a lot of work is happening in function emit, I'm doing this at the same time:

These were also resolved while performing this work:


Remaining work:

  • Create and use adapter for adapting C++ references to C# byrefs for returns
    • Decide if this should be enabled by default. (byref returns are easy to unintentionally dereference and C# developers are not necessarily used to dealing with this. Dereferencing an unmanaged type might not necessarily be a valid thing to do. Additionally they can't be stored in fields yet.)
      • Decided not to enable this by default, I think this style of reference is simply too problematic for the typical C# developer and converting them to pointers is much more awkward than it is in C++.
      • Additionally not being able to store the references is quite a pain.
      • It's unfortunate that the const-ness of const ref returns is lost, but we're already losing them for pointers anyway.
      • I decided not to add an in-between setting where it's only used for const ref returns since I think a better path forward would be to allow extensible metadata which overrides the setting for specific methods. This would allow generator authors to accomplish in-between behaviors of any kind on their own. (For instance, methods which return something not meant to be stored might be better candidates for C# ref returns.)
    • Validate that passthrough works with them. (PassthroughReturnAdapter has special logic for handling them.)
  • Rework how default parameter values are handled
    • CanEmitDefaultValue should only be true if a default value would be emitted
    • Trampoline should not check if DefaultValue is not null (Adapters may provide their own default value that doesn't fit into our model.)
  • Verification of trampolines:
    • Trampoline collection exists
    • Trampoline names match
      • Parameter names too
    • Trampoline native types match
    • Trampoline graph is complete
  • Consider having trampoline names inherit from their function by default instead of copying it. Decided not to do this because it's very annoying for parameter names and I think it's probably better for things to just be consistent.
    • Same thing for parameters. (Slightly complicates determining temporary names ahead of time. This isn't a huge deal except I had the idea to eventually add an API for ensuring temporaries were uniquely named.) This is a pain due to how adapters work (adapters don't necessarily 1:1 correlate with parameters) so I think it's better to just not.
  • Add tests around the trampolines API
    • TrampolineCollection
    • TrampolineBuilder
    • Trampoline?
  • Add tests around trampoline verification
  • Resolve remaining TODOs in the Trampolines folder.
  • Consider making adapter targeting during trampoline building an implementation detail. (Right now you kinda need to specify the target adapter twice.)
  • Consider replacing BoolToByteAdapter/ByteToBoolReturnAdapter with a more general UnsafeAsAdapter/UnsafeAsReturnAdapter instead.
    • Or maybe a CastAdapter/CastReturnAdapter with three modes: Explicit, Implicit, UnsafeAs.
    • This could maybe replace NonBlittableTypeAdapter and NonBlittableTypeReturnAdapter too, although we'd need to add type references for the NativeBoolean/NativeChar to handle the late generation of those types. Decided not to do this, these adapters only sort-of fit into this shape. They only appear on the native function trampolines so it feels weird to have cast adapters in that context.
    • We lost UsePedanticConversion in the process of doing this. Not a huge deal, let's wait and see if anyone actually needs it and we can consider adding a dedicated CastKind just for it. (Or maybe it can just be its own adapter. Generator authors could create this adapter pretty easily if the wanted to.)
  • Consider renaming ReturnByReferenceAdapter to ReturnByBufferAdapter ReturnByImplicitBufferAdapter to disambiguate it from ByRefReturnAdapter. (The former is for implicit return buffers, the latter is for C# byref returns.)
  • Even though we added a workaround for #239, I think we probably also subtly changed how emit happens for vtable entires with regards to the function pointer. We should consider modifying ThisPointerAdapter to not hold on to the type for this and instead infer it from the context like we did before.
  • Add removing and replacing synthetic adapters

Other things which may wait:

  • Attribute extensibility
  • Synthetic adapters (adapters which have no output. Could be used to introduce additional parameters.)
  • Wrapper adapters for things like SetLastError (might just be synthetic adapters with no input.)
  • Allowing type transformation to participate in types within trampolines
    • Let's wait until a compelling use-case comes up for this one. It's not really clear how it'd work since TransformationContext can't really express that it's inside a Trampoline. It's also an un-fun edge case. Ideally types should be settled by the time trampolines are added (hence why we verify the types didn't change.)
  • (Safe) Mutability of trampolines. (Trampoline is a record with some init properties and TrampolineCollection offers some safe mutation, maybe there's more we could add? I didn't have it in mind very much while designing the API. Allowing adapter mutation could be burdensome on implementers.) I think the current level of mutability is fine. Anything more complicated than what we have right now would require proper transformation-like support and I don't think there's a strong need. Can revisit if we ever find a compelling use-case.
  • Decide whether to remove the now-dead function emit logic from CSharpCodeGenerator. (Might keep for temporary backwards compatibility.) -- Let's keep it for now and remove it when pipelines are finished.
  • Implement ToString on adapter implementations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Area-Trampolines Issues concerning the trampolines API Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output Language-C# Issues specific to C#
Projects
None yet
Development

No branches or pull requests

1 participant