Add a clang-specific impl->projection conversion operator by DHowett · Pull Request #1274 · microsoft/cppwinrt

DHowett · Feb 14, 2023

In Clang, the conversion from producer_convert to const producer_ref
does not allow for the move-initialization of I via I(I&&) (implicit
or explicit) due to a language misspecification¹.

In brief², we cannot offer zero-cost initialization of projected types
from implementation types in all meaningful cases in projects compiled
with clang.

Until it improves (which is tracked in llvm/llvm-project#17056), this
conversion operator will allow for C++/WinRT code to compile with Clang.

Closes #1273

¹ http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2077 documents
the language standard bug that results in this.

² In non-brief, https://stackoverflow.com/a/70654432

The conversion from `producer_convert` to `const producer_ref` does not improve const-correctness, and in compliant compilers¹ prevents the conversion of `producer_convert` (ala `winrt::implements<D, I>`) to `producer_ref<T>`'s base type `T`. In brief², this is because conversion acts as rvalue reference construction via `T(T&&)`; a `const`-qualified rvalue cannot bind an rvalue reference. The `const`ness here doesn't impart any additional safety, either: - It contains only an untyped pointer, the constness of which does not restrict its holder from any additional access to either the implementation or projected types. - It exists to convert to its base type T, and it will do so by copy. That copy will contain (hidden) the same pointer, but will often not be `const`-qualified; any code that holds it will likely have unfettered access to that In short, it's a `const`-qualified temporary that exists to convert a `winrt::implements<D, I>` from non-`const` `D` to non-`const` `I`. Given that holding on to a `producer_ref` is in violation of the C++/WinRT `impl` namespace contract _anyway_, be it `const`-qualified or not, this should be a safe change to make. Closes microsoft#1273 ¹ http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2077 ² https://stackoverflow.com/a/70654432

DHowett · Feb 14, 2023

/cc @DefaultRyan @oldnewthing as mentioned in #1273.

DHowett · Feb 14, 2023

(I'll dig in on the ASAN failures, which didn't reproduce when I ran the test suite locally.)

lhecker · Feb 14, 2023

Edit: With the included build_test_all.cmd I can reproduce the issue.

DHowett · Feb 14, 2023

Well, it looks like that is a load-bearing const. Thanks for the explanation, @lhecker!

In short, here's what happens during conversion from implements<D, I> to I.

With `const producer_ref`

producer_convert::operator producer_ref const calls ctor...
... producer_ref::producer_ref which calls base type ctor...
I::I(void*, take_ownership_from_abi_t)
That producer_ref is used in an expr where an I is accepted.
The resultant I is copy-constructed, increasing the reference count, because the source I (nee producer_ref) is const.
The producer_ref destructs, nulling out its ptr member.

With `producer_ref`

Same as above, except step 5 differs:

The resultant I is move-constructed, which does not increase the reference count. This produces a projected type that does not hold a strong reference to its member.

OK?

It seems like producer_ref exists to allow you to pass an implementation type off as a projected type without an AddRef/Release in the common parameter-passing case where something consumes a const I&.

Is AddRef expensive enough to justify that? This code doesn't work in spec-compliant compilers... which I had believed was C++/WinRT's main goal 😄

kennykerr · Feb 14, 2023

Yes, the producer_ref conversion exists to support const& call sites from within an implementation. Injecting AddRef/Release bumps would be a deal breaker. Whether Clang is more compliant than MSVC is debatable. Anyway, you could try adding an overload that is non-const without actually removing the existing conversion. Not sure if that will have other negative consequences though.

DHowett · Feb 14, 2023

My next couple avenues of exploration are...

Adding an operator producer_ref&& to enable explicit rvalue-ref conversion
Adding a non-const overload to see if it gets picked up in the appropriate places
Adding an operator I that does result in an AddRef/Release but scoping it down to only clang

I know that the last one would be a dealbreaker, but would you be amenable to it if it didn't impact the golden path MSVC scenario?

kennykerr · Feb 14, 2023

Presumably we don't want to inject spurious ref count bumps into Clang builds either but if it only impacts cases where a ref count is needed anyway, such as emplace_back/push_back in your examples, then that should be fine.

DHowett · Feb 22, 2023

Okay, so, here's my contention. I've dug and dug and I can't find a way to eliminate the AddRef/Release when doing a converting copy from impl to projection with Clang.

I'd like to propose two things:

We offer an operator I() for use only with Clang. It imposes an extra AddRef/Release.
That operator is gated behind a new define, (named similar to) CPPWINRT_I_ACCEPT_THE_COST_SO_BUILD_WITH_CLANG

See, at the end of the day... all I need is for my code to appear to be well-formed and compile. I'm not shipping a product based on this, but right now we would need to fix up ~151 sites across our codebase (and annotate them with a comment explaining why the normal C++/WinRT way doesn't work so they don't get reverted) and in every one of those cases introduce our own extra AddRef/Release.

If you'd be amenable to a gated "make the code work, sub-optimally, if the user opts in" experience... I'd be happy to write it up.

kennykerr · Feb 22, 2023

Hey, thanks for looking into it. Obviously, compiling sub-optimally is better than not compiling at all. Can we just hide it behind an #ifdef clang? That way we can always improve things later as smart developers help out. I just have a strong aversion to adding preprocessor definitions as they're hard to manage and version over time.

DHowett · Feb 22, 2023

I'm totally cool with that. Thanks @kennykerr. I'll update this PR inplace.

DHowett · Feb 23, 2023

Alright, updated the PR title, body and contents.

kennykerr

Looks good - thanks!

alvinhochun · Feb 23, 2023

Can you add some tests for this?

kennykerr · Feb 23, 2023

A little surprised this isn't already covered, unless a test is disabled for clang?

(cherry picked from commit 675f8b7)

DHowett · Feb 23, 2023

Good call. I added a test, unconditional for all compilers. 😄

I could not find one that had been disabled for clang that would have exercised this.

kennykerr · Feb 23, 2023

test/old_tests/UnitTests/as_implements.cpp

+    // uniform initialization relies on IStringable(IStringable&&),
+    // which requires non-const rvalue semantics.
+    com_ptr<Foo> foo = make_self<Foo>();
+    IStringable stringable{ foo.as<IStringable>() };


Suggested change

IStringable stringable{ foo.as<IStringable>() };

IStringable stringable{ *foo };

Isn't something like this required to actually invoke the implicit conversion operator? Otherwise you're just calling the explicit as method provided by com_ptr which in turn calls QI.

kennykerr requested review from DefaultRyan and oldnewthing February 14, 2023 15:56

DHowett added 2 commits February 22, 2023 19:31

Switch to a clang-only operator I

b48a2f3

Merge remote-tracking branch 'origin/master'

a21756f

DHowett changed the title ~~Remove unnecessary const qualifier on conversion returns~~ Add a clang-specific impl->projection conversion operator Feb 23, 2023

kennykerr previously approved these changes Feb 23, 2023

View reviewed changes

DHowett dismissed kennykerr’s stale review via 7e69a1a February 23, 2023 18:54

Add a uniform initialization test

7e69a1a

(cherry picked from commit 675f8b7)

kennykerr reviewed Feb 23, 2023

View reviewed changes

Did you not have enough coffee?

42c8bb7

kennykerr approved these changes Feb 23, 2023

View reviewed changes

kennykerr merged commit 72b30cc into microsoft:master Feb 23, 2023

	IStringable stringable{ foo.as<IStringable>() };
	IStringable stringable{ *foo };

Search code, repositories, users, issues, pull requests...

Conversation

DHowett commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Feb 14, 2023

Uh oh!

DHowett commented Feb 14, 2023

Uh oh!

lhecker commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

With const producer_ref<I>

With producer_ref<I>

OK?

Uh oh!

kennykerr commented Feb 14, 2023

Uh oh!

DHowett commented Feb 14, 2023

Uh oh!

kennykerr commented Feb 14, 2023

Uh oh!

DHowett commented Feb 22, 2023

Uh oh!

kennykerr commented Feb 22, 2023

Uh oh!

DHowett commented Feb 22, 2023

Uh oh!

DHowett commented Feb 23, 2023

Uh oh!

kennykerr left a comment

Choose a reason for hiding this comment

Uh oh!

alvinhochun commented Feb 23, 2023

Uh oh!

kennykerr commented Feb 23, 2023

Uh oh!

DHowett commented Feb 23, 2023

Uh oh!

kennykerr Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

DHowett Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DHowett commented Feb 14, 2023 •

edited

Loading

lhecker commented Feb 14, 2023 •

edited

Loading

DHowett commented Feb 14, 2023 •

edited

Loading

With `const producer_ref<I>`

With `producer_ref<I>`