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

Properly fix mis-associated function with vtable entry #239

Open
PathogenDavid opened this issue Feb 21, 2022 · 0 comments
Open

Properly fix mis-associated function with vtable entry #239

PathogenDavid opened this issue Feb 21, 2022 · 0 comments
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd TechDebt Workaround

Comments

@PathogenDavid
Copy link
Member

The trampolines api work revealed a subtle bug in our logic for associating VTable entries with their corresponding functions.

Right now we resolve using TranslatedVTableEntry.MethodReference. This works most of the time but in situations where the function gets duplicated (such as with LiftBaseMethodsTransformation in Mochi.DirectX) it ends up selecting the wrong function.

This wasn't noticed before because the logic in EmitFunctionContext would select the this pointer type based on the containing record regardless of where the TranslatedFunction actually came from. However with the new trampoline API the type of the this pointer is effectively determined during CreateTrampolinesTransformation.

For now I've added a hacky workaround which prefers to associate with a function which is on the same record as the vtable, but we either need to:

  • A) Formalize the __HACK__CouldResolveTo APIs.
    • I've had desire for this API before so this wouldn't be such a bad idea.
  • B) Provide a TryResolve which can be scoped to a specific declaration
  • C) Handle the situation better in transformations like LiftBaseMethodsTransformation (by duplicating the methods and re-associating the vtable entries.
    • This is probably the most correct thing to do in the current model -- and is maybe even a good idea regardless of what we do -- but I think this creates a weird gotcha if we don't also do one of the other solutions.
@PathogenDavid PathogenDavid added Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Workaround TechDebt labels Feb 21, 2022
PathogenDavid added a commit that referenced this issue Feb 21, 2022
PathogenDavid added a commit that referenced this issue Mar 9, 2022
…y trampoline methods.

This commit also adds partial support for .NET 7 / C# 11.

Closes #236
Closes #79
Fixes #200
Contributes to #233
Contributes to #237
Workaround for #239
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 TechDebt Workaround
Projects
None yet
Development

No branches or pull requests

1 participant