Why is this change being made?
Someone discovered a subtle compatibility issue with the cast result checking changes and certain projected methods. The variables in the consume_ method, such as the generically-named code variable, can shadow the parameters to the function. This led to a build break where a function took an int16_t named code and that was shadowed by the int32_t named code with the HRESULT in it. Fortunately the compiler had our back and flagged the size truncation as a build break so it was noticed.
Briefly summarize what changed
The variable names in these generated functions are now much uglier (and therefore less likely to collide by coincidence). They have an underscore prefix and then lowercase which should put them into an unofficial namespace that shouldn't collide with anything else. The code variable is now named _winrt_cast_result_code for example. While I was renaming everything I realized that my previous names mismatched the cppwinrt naming convention of snake_case so I fixed them up.
arm32 is not supported anymore by Windows. There has not been a native arm32 OS since before Win11. The wow32 backcompat on arm64 devices to run arm32 programs was removed with Win11 24H2. Most notably, the ability to build arm32 with newer SDKs and toolsets has seemingly been removed. This is now breaking the CI build.
The easiest fix is to simply remove arm32 support.
These failures were pointed out yesterday when the CLA policy was stuck. They are not blocking but have seemingly been failing for a long time and the failure is suppressed.
The issue seems to be that some of these test projects reference an old NuGet package while also referencing the props/targets that are about to go into the new NuGet package. Double-including essentially the same files twice is what caused the build breaks in these projects.
Some projects also had build breaks that seem to be related to a very old Windows SDK minimum version. I increased the floor to 10.0.18362.0 (early 2019; almost 6 years ago) and that fixed the remaining breaks. The random packages.config that downloaded random builds of cppwinrt from 2019 or 2020 have been deleted too.
This set of changes aims to fix the build breaks and un-suppress this failure. It now builds locally on my device but I'll need an official PR build to ensure that the GitHub Actions flow is also passing.
Why is this change being made?
I have been trying to ingest the HEAD of cppwinrt for some large internal projects and one of them had some test failures with the new version. The failures are because there is a try_as cast in their code that fails and is handled fine, but it leaves a COM error context floating around on that thread. Subsequent code fails to originate an error because it sees a context already active on the thread and NOOP'ed.
What this boils down to is that try_as should not have a behavioral change to store context when the cast fails.
Briefly summarize what changed
To address this problem I am taking a PR suggestion from @oldnewthing to have a new try_as_with_reason method that returns both the cast result as well as the HRESULT. The error context logic was only there to smuggle the HRESULT out of a call without an HRESULT return value, so if it is a direct return we don't need that anymore. The new method directly returns the HRESULT so there is no ambiguity.
The previous approach relied on a cast operator to call try_as (or just addref when the type is already a match). Thanks to the recent if constexpr code gen change those cases are now separated out. We have a code block where we know a cast is needed so try_as_with_reason can be called unconditionally. The code path where no cast is needed already circumvents this and is nicely unaffected.
This also made check_cast_result equiavelnt to check_hresult so it was deleted in favor of just calling check_hresult.
How was this change tested?
I did local builds of cppwinrt (both Debug and Release) and ran the tests. I am also compiling some large internal projects with it to ensure that there are no obvious breaks or regressions. I am expecting minimal to no binary size impact from this change.
Why is this change being made?
I have been doing some local builds of an internal component against the latest cppwinrt.exe, which includes the fixes in #1442. I have noticed some modest (1%) binary size grown when comparing the April 2024 release and the latest. Using SizeBench to analyze binary size differences I determined that the inlined code gen seems to explain the difference. This change eliminates that increase, at least for the binary I'm testing against, and even shrinks it a bit further.
Briefly summarize what changed
The various winrt::impl::consume_THING methods get heavily inlined in release builds. The current code gen has some casts for when the types don't match (and is an AddRef/Release when they do match). The small amount of new cast result checking ends up at many call sites and slowly adds up a bit. I think we can do better in the cases where the types already match.
This seems to have proven true in practice. Using if constexpr to determine if the type is a match allows us to skip any casts when this is true. In fact that is a net improvement over the original baseline because we don't need to AddRef/Release either. We can directly call the appropriate method. When a cast is necessary the code gen is identical to the previous baseline. The QueryInterface call is unavoidable and so is checking the result.
The code writer format string is definitely starting to creak under the weight of many arguments. I don't want to refactor that as part of this PR but it would be good to simplify in the future.
Also included is late feedback from the previous PR from @oldnewthing. The check_cast_result method can take void* instead of a template argument because it only null checks it. In my local measurements the optimizer already folded them so it makes no binary size difference but it is still a nice change to take.
How was this change tested?
Ran the full suite of cppwinrt tests locally. I also debugged into Windows.Foundation.IStringable.ToString for cases where it both is and is not the correct type. They run the expected code paths. I also checked the binary size of a large example to confirm the expected reduction. The disassembly for a Release build similarly matches expectations.
Why is this change being made?
Someone is trying to upgrade the cppwinrt version used by a large project that has many static libraries using cppwinrt. Some binaries in that project are mixing static libs with different cpp language versions. (This seems like not a great idea generally but it is the state of the world so to some degree we have to live with it). In those binaries the ODR checks for cppwinrt source_location usage are breaking the build. For good reason, it is not safe to mix this functionality across language versions. This set of changes is aimed at making that upgrade process easier without losing any useful functionality.
Briefly summarize what changed
We already have winrt::impl::slim_source_location which is a lot like std::source_location, minus always containing the very impactful FUNCTION data. This type is powered by the same intrinsics as the STL version so it works as well as the STL library. The existence of this class means that we can avoid the ODR violations by always using winrt::impl::slim_source_location.
Some new macros are used to control what goes into the constructor. cpp20 code that does not suppress source_location will get valid source information passed in. Code that is cpp17, or suppresses this feature, will have zero's passed in. Furthermore, when compiling as _DEBUG this will also include the FUNCTION data, matching previous behavior.
The net result is that there is no more ODR violation because the function signatures are always the same.
How was this change tested?
build_test_all.cmd for both release and debug. I also created a little project that mixes cpp17 and cpp20 libraries. With the latest public release of cppwinrt this project does not build because of the ODR violations. With these changes it builds and runs as expected.
# Why is this change being made?
The generated projection code for interfaces that the metadata declares as required for a runtimeclass assume that QueryInterface never fails. Assuming the metadata is correct in the first place, this is a valid assumption for inproc calls.
However, for cross-process calls the QI can fail even if the metadata is correct and the class really does implement all of the required interfaces. It can fail with E_ACCESSDENIED and a variety of other RPC error codes.
When this happens there is a nullptr crash in the generated consume method. This can be very painful to debug because the HRESULT is lost by the time it crashes.
# Briefly summarize what changed
This set of changes fixes the crash by detecting the QueryInterface error and throwing an exception when that occurs during one of these required casts. The try_as method was changed to capture COM error context when the QI fails. The code gen surrounding WINRT_IMPL_STUB was changed to save the result into a temporary variable and then pass it to a new check_cast_result method. check_cast_result is marked noinline so that the binary size impact of throwing exceptions is limited to a single function instead of inlining into high-volume generated code.
If the cast succeeded then nothing happens. If the cast failed, returning null, then the stored COM exception is retrieved. Assuming it is available the HRESULT is pulled out of it and it is thrown. This then propagates like any other exception. Callers are free to try and catch it or let it go uncaught and crash. Now they have the choice.
I also added a new file of test code that exercises this code path. The test_component IDL declares a runtimeclass that implements IStringable. And then the implementation fails to implement IStringable. When ToString is called on this object it hits the failure path. The cppwinrt code gen will not allow this to happen so I had to directly use winrt::implements.
# How was this change tested?
Besides the new test cases I also wrote a little console app that crashes this way. I built and ran it using the latest stable cppwinrt as well as my private new one. As expected the debugger blame is far more useful with these changes.
The original attempt at a fix for this was too aggressive:
https://github.com/microsoft/cppwinrt/pull/1381/files
This fix is targeted specifically at removing references to stale winmd files that have yet to be copied from referenced projects (e.g., from a runtime component to an app).
* Merged PR 9973274: Auto-generated baselines by 1ES Pipeline Templates
This pull request includes baselines **with an expiration date of 180 days from now** automatically generated for your 1ES PT-based pipelines. Complete this pull request as soon as possible to make sure that your pipeline becomes compliant. Longer delays in completing this PR can trigger additional emails or S360 alerts in the future.
1ES PT Auto-baselining feature helps capture existing violations in your repo and ensures to break your pipeline only for newly introduced SDL violations after baselining. Running SDL tools in break mode is required for your pipeline to be compliant. Go to https://aka.ms/1espt-autobaselining for more details.
* Merged PR 9973274: Auto-generated baselines by 1ES Pipeline Templates
This pull request includes baselines **with an expiration date of 180 days from now** automatically generated for your 1ES PT-based pipelines. Complete this pull request as soon as possible to make sure that your pipeline becomes compliant. Longer delays in completing this PR can trigger additional emails or S360 alerts in the future.
1ES PT Auto-baselining feature helps capture existing violations in your repo and ensures to break your pipeline only for newly introduced SDL violations after baselining. Running SDL tools in break mode is required for your pipeline to be compliant. Go to https://aka.ms/1espt-autobaselining for more details.
---------
Co-authored-by: MerlinBot <MerlinBot>
Incremental builds fail when a referenced project's winmd has been updated. This is because the CppWinRT reference projection is properly using project's referenced winmds as inputs. But the MarkupCompilePass2 target is using XamlReferencesToCompile, which has been set here to use previously copied output files.
We have found that a very common pattern for event handlers is
to capture a weak reference into a lambda, and in the event handler,
try to upgrade the weak reference to a strong one, and if so, do some work:
```cpp
widget.Closed([weak = get_weak(), data](auto&& sender, auto&& args)
{
if (auto strongThis = weak.get())
{
strongThis->do_all_the_things(data);
}
});
```
This commit extends the existing delegate constructors to permit a
`winrt::weak_ref` + lambda (or `std::weak_ptr` + lambda), which
simplifies the above to
```cpp
widget.Closed({ get_weak(), [this, data](auto&& sender, auto&& args)
{
do_all_the_things(data);
} });
```
## Implementation notes
A lambda and pointer to member function are hard to distinguish
in a template parameter list. In theory, we could use SFINAE or
partial specialization, but a simpler solution is to distinguish
the two inside the body of the constructor, via
`std::is_member_function_pointer_v`.
The `com_ptr` and `shared_ptr` variants of the test were
unified, since I found myself editing two nearly identical tests.
Fixes#1371
Co-authored-by: Jon Wiswall <jonwis@microsoft.com>
`resume_agile` exposes the ability to save the `await_adapter`
in a variable. This was not possible without `resume_agile` because
the `await_adapter` had previously been available only via
`operator co_await`, which means that it is created only in
response to an immediate attempt to `co_await` it, so we knew
that it would be consumed before its argument (possibly a temporary)
was destructed.
`resume_agile` returns the `await_adapter`, and we expect people
to await it immediately, but it's possible that they decide to
save it in a variable and await it later. In that case, we have
to record the `Async` as a value instead of a reference. We forward
the `resume_agile` argument into the `Async` so that it moves
if given an rvalue reference, or copies if given an lvalue reference.
This ensure that the common case where somebody does
`co_await resume_agile(DoSomething())`, we do not incur any additional
AddRefs or Releases.
Now that it's possible to `co_await` the `await_adapter` twice,
we have to worry about `await_suspend` being called twice. It had
previously assumed that `suspending` was true (since that's how it
was constructed), but that is no longer valid in the `resume_agile`
case if somebody tries to await the `resume_agile` twice. So we have
to force it to `true`. (Now, the second await will fail with
"illegal delegate assignment", but our failure to set `suspending`
to `true` led to double-resumption, which is super-bad.)
* Enable faster dev cycle in Visual Studio
* Oops, remove one more run
* Fix linux build temporarily - see also #1341
---------
Co-authored-by: Jon Wiswall <jonwis@ntdev.microsoft.com>
* Add SDKReference-sourced WinMDs when building
* Add solution items so they're more easily edited
* Update development guidance slightly
* First attempt at template folding
* Make packages easier to build, remove warning about unreferenced static
* Lift delegate creation out for better folding
* More folding of events
* Another delegate folded
* PR feedback
* Speculative improvement for QueryInterface
* PR feedback
---------
Co-authored-by: Jon Wiswall <jdwiswall@hotmail.com>
Co-authored-by: Kenny Kerr <kekerr@microsoft.com>
Co-authored-by: Jon Wiswall <jonwis@ntdev.microsoft.com>
David Machaj
Ryan Shepherd
Duncan Horn
Nate Thorn
Manodasan Wignarajah
Michael Maltsev
Felix Patschkowski
Dependabot
Josh Soref
Scott Jones
Kenny Kerr
dependabot[bot]
TDBuild
Dan Legg
Charles Milette
Raymond Chen
Jon Wiswall
David Lechner