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

Vtbl indices are sometimes incorrect in MSVC ABI #207

Closed
andrew-boyarshin opened this issue Mar 17, 2021 · 7 comments · Fixed by #213
Closed

Vtbl indices are sometimes incorrect in MSVC ABI #207

andrew-boyarshin opened this issue Mar 17, 2021 · 7 comments · Fixed by #213

Comments

@andrew-boyarshin
Copy link
Contributor

Demo. OutputVtblHelperMethod is assuming vtblIndex is monotonously increasing. That's correct only in simple cases, not with e.g. overloads. Better receive correct information from Clang itself.

@andrew-boyarshin andrew-boyarshin changed the title Vtbl indices are incorrect in MSVC Vtbl indices are sometimes incorrect in MSVC ABI Mar 17, 2021
@tannergooding
Copy link
Member

Thanks for the reference. Looks like I will need to wrap and expose MicrosoftVTableContext in libClangSharp as it is not exposed by libClang.

@andrew-boyarshin
Copy link
Contributor Author

Does Itanium ABI guarantee ordering of vtbl pointers? If not, maybe it makes sense to expose ItaniumVTableContext too. Or wherever the results are stored from both ABIs.

@tannergooding
Copy link
Member

Does Itanium ABI guarantee ordering of vtbl pointers

I'm unsure and so I'll be updating it to just query the context always.

I've got a PR for TerraFX here: terrafx/terrafx.interop.windows#181 and will be submitting the ClangSharp changes after completing some of the other libClangSharp improvements I'm working on.

@tannergooding
Copy link
Member

CC. @sotteson1, how should this be handled on the win32metadata side?

I don't recall which setup win32metadata is using for vtbls. Is it:

  • Explicit vtbls with IntPtr
  • Explicit vtbls with Fnptr
  • Implicit vtbls

I believe win32metadata is skipping generation for the helper methods that invoke the underlying fnptr, is that right?

@sotteson1
Copy link
Contributor

sotteson1 commented Mar 17, 2021

CC. @sotteson1, how should this be handled on the win32metadata side?

I don't recall which setup win32metadata is using for vtbls. Is it:

  • Explicit vtbls with IntPtr
  • Explicit vtbls with Fnptr
  • Implicit vtbls

I believe win32metadata is skipping generation for the helper methods that invoke the underlying fnptr, is that right?

I keep the generation of the helper methods because this gives me the correct method signatures to add to the interface. When I'm emitting an interface, I loop through the method declarations and assume they are always in the correct vtbl order. Is that something that isn't safe to assume? If I had to, I could look deeper into the syntax tree of the body of the function to figure out which element of the lpVtbl the method is calling.

@tannergooding
Copy link
Member

Is that something that isn't safe to assume?

Apparently not. Andrew raised that http://web.archive.org/web/20141206094130/http://support.microsoft.com/kb/131104 exists and this means that they may not be in declaration order when overloads with the same name exists (as frequently occurs in DComp and D2D1).

If I had to, I could look deeper into the syntax tree of the body of the function to figure out which element of the lpVtbl the method is calling.

I think the available options are:

  • Have ClangSharp emit in vtbl order
    • This means it may not match the C++ order
    • This won't work with more complex C++ code, although this may not be a problem for the Windows SDK
  • Have ClangSharp do nothing additional
    • win32metadata and other tools may need to analyze the IL to determine the correct vtbl index
  • Have ClangSharp emit an explicit [VtblIndex(#)] attribute
    • This seems the most flexible and least error prone. Tools would then just look for this attribute to determine which vtbl entry to lookup
    • This would likely be more extensible for more complex C++ code, such is if you have multiple vtbls

@tannergooding
Copy link
Member

@sotteson1, I went with the third option in #215.

There is now a new generate-vtbl-index-attribute config switch which adds a VtblIndex(#) attribute to the helper method which can be used by downstream generates such as CsWin32 or RsWin32.
There is also the explicit-vtbls option which will also emit things in the correct order using function pointers, but only under latest-codegen (while I believe win32metadata uses compatible-codegen)

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