wil::to_array_view provides safer access to IBuffer / IMemoryBuffer #359
Conversation
The existing `.data()` member gives you a pointer to the start of the buffer but you are on your own to get the buffer size by other means. The new `wil::to_array_view` function returns the buffer bytes in the form of a `winrt::array_view`, which encodes both the start of the buffer as well as its length. By default, you get an `array_view<uint8_t>`, but you can customize the data type by calling `to_array_view<T>` to get an `array_view<T>`. Any partial `T`s are ignored. For example, if the buffer is 6 bytes long, then calling `to_array_view<int32_t>` will give you a view of size 1. To get the `IMemoryBuffer` and `IMemoryBufferReference` versions, you must include `winrt/Windows.Foundation.h` before `wil/cppwinrt_helpers.h`, and you must include `MemoryBuffer.h` at some point before using `to_array_view`. To get the `IBuffer` version, you must include `winrt/Windows.Storage.Streams.h` before `wil/cppwinrt_helpers.h`. The `IBuffer` version defaults to returning an `array_view` for the buffer's length. If you want an `array_view` for the buffer's capacity, you can use `to_array_view_for_capacity`. We follow the pattern of allowing `cppwinrt_helpers.h` to be included multiple times, and each time you do it, new features light up based on what headers you have included prior to including `cppwinrt_helpers.h`.
|
One thing to possibly consider: the language really wants you to use |
|
We seem to be sporadically hitting a lot of |
We can make it a dependent type on the existing template type parameter T. Also update the test to (1) avoid endianness, (2) let the compiler deal with the magic numbers.
|
Suggestion: add an |
I don't think so. You don't always control who gave you the buffer, and you don't want to crash if somebody gave you a wrong-sized buffer. We assume you just want to ignore the fractional bytes. This can happen if, for example, somebody creates a buffer based on GlobalSize(), which is allowed to return a value larger than the value passed to GlobalAlloc. |
|
For some reason, your changes are really giving the pipeline troubles. I think every build/re-build has failed with an AV in the same place and I just re-ran master which got passed that point. Can you make sure to build & run the tests with Clang targeting x86 w/ RelWithDebInfo to see if it repros locally for you? |
|
I don't really like having basic helpers on WinRT types in WIL - this should be in C++/WinRT. |
|
C++/WinRT is evolving at a glacial pace, so it makes sense to focus C++/WinRT itself on the projection only and keep helpers in a repo that can evolve faster. I wouldn't be surprised if an eventual C++/WinRT 3 extracts most built-in helpers to WIL. |
|
|
Edit: woops, wrong stack initially That's Debug, not RelWithDebInfo :). I checked out your repo and looks like this repros for me pretty reliably: I'll start looking into it to see what's triggering the issue; IIRC issues like this are generally due to Clang handling SEH a bit weird |
|
@dunhor Oops, sorry, missed that detail. I cannot repro with clang32relwithdebinfo either.
From the stack, it appears to be an order-of-uninitialization issue when an app fails to uninitialize COM, and COM is forced to uninitialize itself from inside It tries to clean up the CComCLBCatalog, and that calls CoTaskMemFree, but the CoTaskMemFree mutex has already been destroyed. CoTaskMemFree then starts a new round of delay-initialization, and that tries to store some state in the TLS, which has already been torn down. It appears to be related to CoRegisterMallocSpy. Let me see if any tests use that. (time passes) Okay, I found it. It turns out that CoRegisterMallocSpy is sticky. If you register a malloc spy and then unregister it, the state does not completely revert to the "malloc spy not registered" state. Unregistering the malloc spy just replaces the malloc spy with a dummy one, but the "active malloc" vtable is still set to "Check for an active malloc spy", and we try to delay-initialize the malloc-spy mutex. I think this is a COM bug, exacerbated by the fact that C++/WinRT intentionally leaks a COM initialization, which forces COM to do "desperation cleanup" inside |
| @@ -182,6 +184,44 @@ TEST_CASE("CppWinRTTests::VectorToVector", "[cppwinrt]") | |||
| winrt::uninit_apartment(); | |||
| } | |||
|
|
|||
| TEST_CASE("CppWinRTTests::BufferToArrayView", "[cppwinrt]") | |||
| { | |||
| std::array<int32_t, 2> testData = { 314159265, 27182818 }; | |||
There was a problem hiding this comment.
| std::array<int32_t, 2> testData = { 314159265, 27182818 }; | |
| winrt::init_apartment(); | |
| auto uninit = wil::scope_exit([]{ winrt::uninit_apartment(); }); | |
| std::array<int32_t, 2> testData = { 314159265, 27182818 }; |
This seems to avoid the crash. Note that I still hit the crash locally for other tests, however AFAIK we haven't hit the same in the build labs
There was a problem hiding this comment.
OMG! This comment exists for the CppWinRTAuthoringTests and I didn't even know about it!:
// This test cannot run in the same process as the malloc spies tests in wiTest.cpp
// MSFT_internal: https://task.ms/44191550Making that [LocalOnly] isn't really the correct move, though...
There was a problem hiding this comment.
@dunhor Can we proceed with this PR? We sort of gave up on the NotifyPropertyChanged test (PRs 368, 374).
There was a problem hiding this comment.
I've disabled the NotifyPropertyChanged test to run during CI. We can either do the same thing here or we can update the test to call CoInitialize to avoid the crash during shutdown.
There was a problem hiding this comment.
Err, actually disabling for just CI is probably not the best move. If we go the disable route, we should disable even when run locally so that the scripts don't break.
ChrisGuzak
requested review from
ChrisGuzak
and removed request for
ChrisGuzak
|
Once the build finished again I'll poke the merge button. Thanks for your patience @oldnewthing. |
@jonwis see the discussion above. There's a crash in the tests caused by a bug in COM that's caused by a combination of using
I'm personally a fan of option 1: it's simple and effective. Option 3 would help us avoid these issues moving forward, however. |
|
Filed #393 to address the discovered issue |
The existing
.data()member gives you a pointer to the start of the buffer but you are on your own to get the buffer size by other means.The new
wil::to_array_viewfunction returns the buffer bytes in the form of awinrt::array_view, which encodes both the start of the buffer as well as its length.By default, you get an
array_view<uint8_t>, but you can customize the data type by callingto_array_view<T>to get anarray_view<T>. Any partialTs are ignored. For example, if the buffer is 6 bytes long, then callingto_array_view<int32_t>will give you a view of size 1.To get the
IMemoryBufferandIMemoryBufferReferenceversions, you must includewinrt/Windows.Foundation.hbeforewil/cppwinrt_helpers.h, and you must includeMemoryBuffer.hat some point before usingto_array_view.To get the
IBufferversion, you must includewinrt/Windows.Storage.Streams.hbeforewil/cppwinrt_helpers.h.The
IBufferversion defaults to returning anarray_viewfor the buffer's length. If you want anarray_viewfor the buffer's capacity, you can useto_array_view_for_capacity.We follow the pattern of allowing
cppwinrt_helpers.hto be included multiple times, and each time you do it, new features light up based on what headers you have included prior to includingcppwinrt_helpers.h.Fixes #360