try_as casts should not store COM error context; consume method cast checking should use return codes directly

[dmachaj]

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.

Here is what the latest code gen is for IStringable::ToString()

    template <typename D> auto consume_Windows_Foundation_IStringable<D>::ToString() const
    {
        void* value{};
        if constexpr (!std::is_same_v<D, winrt::Windows::Foundation::IStringable>)
        {
            auto const [castedResult, code] = impl::try_as_with_reason<winrt::Windows::Foundation::IStringable, D const*>(static_cast<D const*>(this));
            check_hresult(code);
            auto const abiType = *(abi_t<winrt::Windows::Foundation::IStringable>**)&castedResult;
            check_hresult(abiType->ToString(&value));
        }
        else
        {
            auto const abiType = *(abi_t<winrt::Windows::Foundation::IStringable>**)this;
            check_hresult(abiType->ToString(&value));
        }
        return hstring{ value, take_ownership_from_abi };
    }

[sylveon]

Is try_as_reason a new public API? Would try_as_with_reason be a better name?

[dmachaj]

Is try_as_reason a new public API? Would try_as_with_reason be a better name?

I like that name better. I renamed it everywhere.

[sylveon]

Is there a specific reason for using try_as + check_hresult instead of just calling as?

[dmachaj]

Is there a specific reason for using try_as + check_hresult instead of just calling as?

Yes, the goal is to have the throw statement inside a noinline method so the binary size impact is mitigated. If .as were called then the winrt::impl::consume_X code that gets inlined to callers would have a new throw location and the compiler would need to add some exception unwind information, increasing binary size.

[sylveon]

Yes, the goal is to have the throw statement inside a noinline method so the binary size impact is mitigated.

as calls check_hresult (which calls throw_hresult, which is noinline).

[dmachaj]

Yes, the goal is to have the throw statement inside a noinline method so the binary size impact is mitigated.

as calls check_hresult (which calls throw_hresult, which is noinline).

Yeah, you're right. I didn't originally use that because it wouldn't have worked right before the if constexpr change. The .as call would have added a copy when the types were already compatible. That's no longer a concern and is why I was able to use try_as_with_reason in this PR. Which means I can also just use .as I think.

This seems to split the difference on binary size. It is larger than try_as but it is not as big as the spurious copy that was present in the previous commit.

Edit: I did some binary size analysis. The growth is from compiler inlining decision differences. In the before (commit d296af6) there were function calls to consume_X methods. In the after many of those were inlined. I'm inclined to accept this as-is and then once PGO counts are updated it will pick and choose the best approach on a case-by-case basis.

[DefaultRyan]

/AzurePipelines help

[azure-pipelines]

Supported commands

• help:
• Get descriptions, examples and documentation about supported commands
• Example: help "command_name"
• list:
• List all pipelines for this repository using a comment.
• Example: "list"
• run:
• Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
• Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
• where:
• Report back the Azure DevOps orgs that are related to this repository and org
• Example: "where"

See additional documentation.

[DefaultRyan]

/azp list

[azure-pipelines]

CI/CD Pipelines for this repository:

• microsoft.cppwinrt

[DefaultRyan]

@microsoft-github-policy-service rerun