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

Add get_iids_tearoff() hook. #1467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlech
Copy link
Contributor

@dlech dlech commented Jan 8, 2025

Add a get_iids_tearoff() hook to complement the query_interface_tearoff() hook.

query_interface_tearoff() allows dynamically extending types with additional interfaces that can be accessed with the QueryInterface() method. However, these interfaces were not discoverable by the GetIids() method (and therefore not by the winrt::get_interfaces() function either).

The get_iids_tearoff() hook allows adding the GUIDs of interfaces to the array returned by GetIids().

This also fixes a bug in the implementation of the root_implements_type::is_composing branch of NonDelegatingGetIids() where *array was updated causing the local iids to be unreachable by the caller and risking the caller reading past the end of the array.

strings/base_implements.h Outdated Show resolved Hide resolved
strings/base_implements.h Show resolved Hide resolved
test/old_tests/UnitTests/Composable.cpp Outdated Show resolved Hide resolved
test/old_tests/UnitTests/Composable.cpp Outdated Show resolved Hide resolved
REQUIRE(object.as<IClosable>());
REQUIRE(object.as<IStringable>());
// IBaseOverrides is repeated twice for some reason, so actual size is 7 but there are only 6 unique interfaces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems odd and possibly worth understanding before proceeding. Would you be willing to check the behavior of this test case with your get_iid_tearoff logic commented out?

Copy link
Contributor Author

@dlech dlech Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't come from get_iid_tearoff. It is likely due to this being a composable type and the get_local_iids() of the outer class also picks up the overrideable interfaces from the inner class since overrideable interfaces get special handling.

test/test/tearoff.cpp Outdated Show resolved Hide resolved

winrt::array_view<winrt::guid> get_iids_tearoff() const noexcept final
{
return winrt::array_view(tearoff_iids);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to have one of the test cases use an inline definition instead of deferring to a class field. Something like return winrt::array_view({winrt::guid_of<winrt::IStringable>()}); just to prove that it is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that result in a dangling pointer? winrt::array_view doesn't make a copy of the underlying array AFAIKT, so the returned array view would be pointing to stack allocated memory that is no longer valid from the function that just returned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would depend on whether it is stack allocated or compiletime. I would expect an embedded array to be compiletime and therefore non-dangling because it is part of the DLL itself.

Copy link
Contributor Author

@dlech dlech Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best info I could find says:

The backing array has the same lifetime as any other temporary object, except that initializing an std::initializer_list object from the backing array extends the lifetime of the array exactly like binding a reference to a temporary.

And

An object of type std::initializer_list is a lightweight proxy object that provides access to an array of objects of type const T (that may be allocated in read-only memory).

So it sounds like it can't be relied upon.

Probably using winrt::array_view for a return value was a bad idea since it could lead to dangerous usage and I should pick something else. On the other hand, there is lots of passing things by raw pointers in this part of the code anyway, so maybe it isn't any worse than what is already there.

test/test/tearoff.cpp Show resolved Hide resolved
Add a `get_iids_tearoff()` hook to complement the `query_interface_tearoff()`
hook.

`query_interface_tearoff()` allows dynamically extending types with
additional interfaces that can be accessed with the `QueryInterface()`
method. However, these interfaces were not discoverable by the `GetIids()`
method (and therefore not by the `winrt::get_interfaces()` function
either).

The `get_iids_tearoff()` hook allows adding the GUIDs of interfaces
to the array returned by `GetIids()`.

This also fixes a bug in the implementation of the
`root_implements_type::is_composing` branch of `NonDelegatingGetIids()`
where `*array` was updated causing the local iids to be unreachable by
the caller and risking the caller reading past the end of the array.
@dlech dlech force-pushed the get-iids-tearoff branch from a501448 to 13bdb80 Compare January 9, 2025 04:07
@dlech
Copy link
Contributor Author

dlech commented Jan 9, 2025

Updated:

  • Use std::vector instead of winrt::array_view as return value to avoid memory safety pitfalls.
  • Drop const& on local_count.
  • Fix mixed tabs and spaces.
  • Add assertions for RuntimeType test.

@kennykerr
Copy link
Collaborator

I would caution against doing this. GetIids in general can and does cause code bloat due to all the static data tables it produces. @DefaultRyan did a bunch of work to mitigate the cost without turning it off entirely.

I have never seen a practical use for GetIids beyond a very obscure fallback if-all-else-fails scenario in CLR debugging that is never actually used in practice. I always noop the implementation as a result. Far better to rely on metadata for "reflection" rather than run time reflection via GetIids. 😊

@dlech
Copy link
Contributor Author

dlech commented Jan 9, 2025

run time reflection

The use case for this is for the PyWinRT projection, hence the need for runtime reflection. Since Python is dynamic, we need a way to add interfaces at runtime, so can't rely on static data. I can probably make something work without this if it isn't really useful for anyone else, but hooking into the existing code just makes that easier.

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

Successfully merging this pull request may close these issues.

3 participants