On Wed Oct 12 18:06:20 2022 +0000, Nikolay Sivov wrote:
Certainly worth a second, or third opinion, but to me this test looks like something to keep out of a tree.
I assume by "keep out of a tree" you're saying you'd want to merge the fix but not all of the tests? Which part do you want separated: the IMallocSpy, the cleanups to IRecordInfoImpl::RecordClear, something else?
Seeing the allocations via IMallocSpy was the best way I could think of to actually demonstrate VariantCopy was using CoTaskMemAlloc/IMalloc, but I'm fine if we don't think there needs to be a permanent conformance test to justify that choice. It is kind of the obvious choice for code in oleaut32, albeit someone before apparently overlooked it and used a HeapAlloc instead (which doesn't match windows, and matters if anyone is mixing IRecordInfo::RecordCreate and IRecordInfo::RecordDestroy with VariantCopy/VariantClear.
I don't think the test code as written before was really even correct - there's no particular reason to be confident VariantClear reloads pvRecord from the original VARIANT after calling RecordClear; it seemingly does, but per strict-aliasing it would certainly have been allowed to just reuse the value already in a register or some such too. Just up to how the compiler optimized things (or, in practice, seemingly didn't). And it seems clearly wrong that VariantClear(&varDest) actually modified varSrc too (via the RecordClear modifying varSrc.brecVal via IRecordInfoImpl::rec - https://gitlab.winehq.org/wine/wine/-/merge_requests/1035/diffs?commit_id=02...). There was no test actually verifying varSrc (why would there be?), and it really didn't concern anything done wrong by oleauto, but it seems like a bad behavior to have in the test fixture.
And if we remove that hack from IRecordInfoImpl (or someday when MS's implementation gets a little more compiler optimization), the existing tests start crashing on windows (as seen in https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9110). And after fixing the mising free/memleak in wine's VariantClear, then it crashes on wine too.
If you'd rather, we could eliminate 0xdeadbeef/validsrc/validptr etc and instead implement `RecordInfo_RecordCreate`, so that the pvRecord values used are all actually allocated. That's a bigger change to the actual coverage of the tests, though.