On Wed Oct 12 18:07:13 2022 +0000, Kevin Puetz wrote:
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.
Yes, I meant IMallocSpy part, usual concern is to look too much into implementation details. In this case it's really easy to do, and tracing API is of course public. But now that you know how to allocate/free correctly, you don't really need spying part. It's just my opinion, I'd wait for someone else to comment on this too.