Split Variant marshalling tests #53035

kant2002 · 2021-05-20T18:06:49Z

These tests split to be able test ComWrappers,
built-in COM enabled/disable mode.
See #50500, #51265

kant2002 · 2021-05-20T18:08:05Z

Initially discovered as part of dotnet/runtimelab#1142
I do not know how to pass IsComSupported to test and how to read. Would like that somebody help me with that.
This PR to gather feedback on the implementation details, code split approach.

AaronRobinsonMSFT · 2021-05-20T19:45:20Z

I do not know how to pass IsComSupported to test and how to read. Would like that somebody help me with that.

Adding a feature flag to a test project is the same as passing it in the product. See https://github.com/dotnet/runtime/pull/52940/files#diff-c25cd81bf60eaf73ca87747baafae7fbfd0ac8a787da03b22382eff45316c9e5 and look for RuntimeHostConfigurationOption.

AaronRobinsonMSFT · 2021-05-20T19:45:33Z

/cc @LakshanF @elinor-fung

kant2002 · 2021-05-21T09:52:38Z

I think I overcomplicate PR. I will revert changes, and will create couple projects in same folder. Seems to be that would be easier to maintain.

LakshanF · 2021-05-21T12:57:48Z

@kant2002, thanks for taking a look! Please also take a look at some test changes that are already present for library, and the pending PR, #52940, that has some tests for the runtime.

kant2002 · 2021-05-21T13:48:41Z

@LakshanF this changes just for Variant marshalling. Hopefully current version is in spirit of what was for #52940

kant2002 · 2021-05-24T16:03:10Z

Build was failed, since results was not sent to Helix

LakshanF · 2021-05-24T16:38:49Z

I see these 2 tests failing when built-in COM is disabled,

Interop\PInvoke\Variant\VariantTestBuiltInComDisabled\VariantTestBuiltInComDisabled.cmd
Interop\PInvoke\Variant\VariantTestComWrappers\VariantTestComWrappers.cmd

The first test has the following output,

BEGIN EXECUTION
"C:\h\w\AC52093C\p\corerun.exe" -p "System.Runtime.InteropServices.BuiltInComInterop.IsSupported=false" VariantTestBuiltInComDisabled.dll
Built-in COM Disabled?: True
Test failed: System.NotSupportedException: Built-in COM has been disabled via a feature switch. See https://aka.ms/dotnet-illink/com for more information.
at System.StubHelpers.ObjectMarshaler.ConvertToNative(Object objSrc, IntPtr pDstVariant)
at VariantNative.Marshal_ByValue_Object(Object obj)
at Test.TestByValue(Boolean hasComSupport) in VariantTestBuiltInComDisabled.dll:token 0x6000059+0x223
at Test.Main() in VariantTestBuiltInComDisabled.dll:token 0x600005e+0x34
Expected: 100
Actual: 101
END EXECUTION - FAILED

kant2002 · 2021-05-25T04:17:12Z

Thanks for pointing me in the error log. I was able to find it afterwards. I see now only one error

    Interop\PInvoke\Variant\VariantTestComWrappers\VariantTestComWrappers.cmd [FAIL]
      
      Return code:      1
      Raw output file:      C:\h\w\AF5C0958\w\B3E30948\e\Interop\PInvoke\Reports\Interop.PInvoke\Variant\VariantTestComWrappers\VariantTestComWrappers.output.txt
      Raw output:
      BEGIN EXECUTION
       "C:\h\w\AF5C0958\p\corerun.exe" -p "System.Runtime.InteropServices.BuiltInComInterop.IsSupported=false" VariantTestComWrappers.dll 
      Test failed: TestLibrary.AssertTestException: Assert.IsTrue: 
         at TestLibrary.Assert.HandleFail(String assertionName, String message) in TestLibrary.dll:token 0x6000021+0x11
         at TestLibrary.Assert.IsTrue(Boolean condition, String message) in TestLibrary.dll:token 0x600000a+0x3
         at Test.TestByValue(Boolean hasComSupport) in VariantTestComWrappers.dll:token 0x6000059+0x223
         at Test.Main() in VariantTestComWrappers.dll:token 0x600005e+0xc
      Invalid format. Expected VT_DISPATCH.

Does that means that ComWrappers has slightly different marshalling, or this is a bug? Should I fix that bug as part of PR?

src/tests/Interop/PInvoke/Variant/VariantTest.ComWrappers.cs

AaronRobinsonMSFT · 2021-05-25T18:48:36Z

Does that means that ComWrappers has slightly different marshalling, or this is a bug? Should I fix that bug as part of PR?

@kant2002 It isn't clear to me which statement in the test is failing. Can you narrow it down to which one is the issue?

kant2002 · 2021-05-25T19:16:03Z

@AaronRobinsonMSFT this line produce exception
https://github.com/dotnet/runtime/pull/53035/files#diff-1c679eb2a695bcba0de0c236544a305004b9e816151a677657a2528b2a039608R46
because there printed message from

runtime/src/tests/Interop/PInvoke/Variant/VariantNative.cpp

Line 152 in 57bdb95

printf("Invalid format. Expected VT_DISPATCH.\n");

kant2002 · 2021-05-25T19:19:07Z

Honestly I did not run that. It's just reading logs. I'm yet to build single test project locally. Otherwise it's hard to me to allocate time (~1 hour). If you strictly need me to build, and validate my last statement, let me know. I will compile and troubleshoot in more details. But I think I point location to you properly.

AaronRobinsonMSFT · 2021-05-25T22:33:46Z

@kant2002 The issue here is a bug in the ComWrappers conversion logic for IDispatch. It is a part of the absolute mess that is the OLE support and how enum values are used as both request and result – yuck.

In the both case we detect if IDispatch is implemented but don't reduce the returned enum value to just ComIpType_Dispatch but return the input - which is the request not the result. The solution here is to return either ComIpType_Dispatch or ComIpType_Unknown but not ComIpType_Both.

runtime/src/coreclr/vm/interopconverter.cpp

Lines 183 to 205 in 959c327

    
           if (TryGetComIPFromObjectRefUsingComWrappers(*poref, &pUnk)) 
        
           { 
        
               hr = S_OK; 
        
               IUnknown* pvObj; 
        
               if (ReqIpType & ComIpType_Dispatch) 
        
               { 
        
                   hr = SafeQueryInterface(pUnk, IID_IDispatch, &pvObj); 
        
                   pUnk->Release(); 
        
               } 
        
               else 
        
               { 
        
                   pvObj = pUnk; 
        
               } 
        
               if (FAILED(hr)) 
        
                   COMPlusThrowHR(hr); 
        
               if (pFetchedIpType != NULL) 
        
                   *pFetchedIpType = ReqIpType; 
        
               RETURN pvObj; 
        
           }

AaronRobinsonMSFT · 2021-05-25T22:36:22Z

Should I fix that bug as part of PR?

Yes, that would be much appreciated.

kant2002 · 2021-05-27T04:53:45Z

@AaronRobinsonMSFT can you take a look?

src/coreclr/vm/interopconverter.cpp

kant2002 · 2021-05-27T13:37:42Z

Do not understand test failures. Please advice if this something which I'm responsible with?

src/coreclr/vm/interopconverter.cpp

AaronRobinsonMSFT

@kant2002 I would take a step back here and walk through this logic more fully. I think there was some confusion on when this was originally written and support IDispatch has uncovered that in the ComWrappers scenario.

/cc @elinor-fung

AaronRobinsonMSFT · 2021-05-27T15:31:13Z

src/coreclr/vm/interopconverter.cpp

@@ -189,17 +189,26 @@ IUnknown *GetComIPFromObjectRef(OBJECTREF *poref, ComIpType ReqIpType, ComIpType
        {
            hr = SafeQueryInterface(pUnk, IID_IDispatch, &pvObj);
            pUnk->Release();


This Release shouldn't be called indiscriminately here. We are checking for IDispatch now and it could fail, but ComIpType_Unknown must still be returned. Move this release into the success path below.

src/coreclr/vm/interopconverter.cpp

kant2002 · 2021-05-27T17:16:24Z

Do I understand correctly that in discussed part of code, I should follow these rules?

ReqIpType = ComIpType_Both

QI to IDispatch - ok

pUnk->Release();
FetchedIpType = ComIpType_Dispatch;

QI to IDispatch - failed

pvObj = pUnk;
FetchedIpType = ComIpType_Unknown;

ReqIpType = ComIpType_IDispatch

QI to IDispatch - ok

pUnk->Release();
FetchedIpType = ComIpType_Dispatch;

QI to IDispatch - failed

COMPlusThrowHR(hr); // ????

ReqIpType = ComIpType_IUnknown

pvObj = pUnk;
FetchedIpType = ComIpType_Unknown;

AaronRobinsonMSFT · 2021-05-27T22:14:49Z

@kant2002 Yes.

kant2002 · 2021-05-28T17:55:52Z

@AaronRobinsonMSFT is it possible that ComIpType_OuterUnknown would be passed to that location? Technically that will be handled by IUnknown case, but maybe there special processing needed.

AaronRobinsonMSFT · 2021-05-28T20:40:38Z

@kant2002 I don't think that ComIpType_OuterUnknown should be a concern here. The ComWrappers API does support aggregation but at present I am not sure how we would integrate at this point. I am okay not dealing with it at present.

kant2002 · 2021-05-29T03:40:00Z

@AaronRobinsonMSFT then can you take a look?

AaronRobinsonMSFT

LGTM. Thank you.

…asm_debugger_and_use_debugger_agent * upstream/main: (597 commits) Fix42292 (dotnet#52463) [mono] Remove some obsolete emscripten flags. (dotnet#53486) Fixed path to projects (dotnet#53435) support ServerCertificateContext in quic (dotnet#53175) Socket: delete unix local endpoint filename on Close (dotnet#52103) [mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364) Refactor MsQuic's native IP address types. (dotnet#53461) Re-enabled optimizations for gtFoldExprConst (dotnet#53347) Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361) Fix issues with virtuals and mdarrays (dotnet#53400) Split Variant marshalling tests (dotnet#53035) Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470) remove WSL checks in tests (dotnet#53475) Always spawn message loop thread for SystemEvents (dotnet#53467) add AcceptAsync cancellation overloads (dotnet#53340) Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425) Add CookieContainer.GetAllCookies (dotnet#53441) Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235) [wasm] Re-enable Wasm.Build.Tests (dotnet#53433) [libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253) ...

dotnet-issue-labeler bot added the area-Interop-coreclr label May 20, 2021

kant2002 force-pushed the kant/split-variant-test branch from 9f6bd1e to 024ce9b Compare May 21, 2021 13:45

Rework of variant tests split

6fa61f1

kant2002 force-pushed the kant/split-variant-test branch from 024ce9b to 6fa61f1 Compare May 21, 2021 13:45

Add missing file

3306160

Fix testing of disabled built-in COM

39d52a5

AaronRobinsonMSFT reviewed May 25, 2021

View reviewed changes

src/tests/Interop/PInvoke/Variant/VariantTest.ComWrappers.cs Outdated Show resolved Hide resolved

Address PR feedback

0961092

kant2002 added 3 commits May 26, 2021 07:41

Return fetched type

c5e050f

Fix compilation errors

8d25983

Fix compilation error

23ca8fa

AaronRobinsonMSFT reviewed May 27, 2021

View reviewed changes

src/coreclr/vm/interopconverter.cpp Show resolved Hide resolved

Address PR feedback

05fbfc7

AaronRobinsonMSFT reviewed May 27, 2021

View reviewed changes

src/coreclr/vm/interopconverter.cpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed May 27, 2021

View reviewed changes

runfoapp bot mentioned this pull request May 27, 2021

FileSystemWatcher_DirectorySymbolicLink_TargetsFile_Fails #53366

Open

Fixes to logic

3409bc5

AaronRobinsonMSFT approved these changes May 30, 2021

View reviewed changes

AaronRobinsonMSFT merged commit 8538a75 into dotnet:main May 30, 2021

kant2002 deleted the kant/split-variant-test branch May 31, 2021 09:38

This was referenced Jun 1, 2021

Test failure Loader/classloader/StaticVirtualMethods/GenericContext/GenericContextTest/GenericContextTest.sh #53161

Closed

Host tests often fail on text file busy issues #53587

Closed

LakshanF mentioned this pull request Jun 4, 2021

Linker warning IL2050 doesn't get suppressed by adding RequiresUnreferencedCode dotnet/linker#1989

Closed

ghost locked as resolved and limited conversation to collaborators Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split Variant marshalling tests #53035

Split Variant marshalling tests #53035

kant2002 commented May 20, 2021

kant2002 commented May 20, 2021

AaronRobinsonMSFT commented May 20, 2021

AaronRobinsonMSFT commented May 20, 2021

kant2002 commented May 21, 2021

LakshanF commented May 21, 2021

kant2002 commented May 21, 2021

kant2002 commented May 24, 2021

LakshanF commented May 24, 2021

kant2002 commented May 25, 2021

AaronRobinsonMSFT commented May 25, 2021

kant2002 commented May 25, 2021

kant2002 commented May 25, 2021

AaronRobinsonMSFT commented May 25, 2021 •

edited

Loading

AaronRobinsonMSFT commented May 25, 2021

kant2002 commented May 27, 2021

kant2002 commented May 27, 2021

AaronRobinsonMSFT left a comment

AaronRobinsonMSFT May 27, 2021

kant2002 commented May 27, 2021 •

edited

Loading

AaronRobinsonMSFT commented May 27, 2021

kant2002 commented May 28, 2021

AaronRobinsonMSFT commented May 28, 2021

kant2002 commented May 29, 2021

AaronRobinsonMSFT left a comment

Split Variant marshalling tests #53035

Split Variant marshalling tests #53035

Conversation

kant2002 commented May 20, 2021

kant2002 commented May 20, 2021

AaronRobinsonMSFT commented May 20, 2021

AaronRobinsonMSFT commented May 20, 2021

kant2002 commented May 21, 2021

LakshanF commented May 21, 2021

kant2002 commented May 21, 2021

kant2002 commented May 24, 2021

LakshanF commented May 24, 2021

kant2002 commented May 25, 2021

AaronRobinsonMSFT commented May 25, 2021

kant2002 commented May 25, 2021

kant2002 commented May 25, 2021

AaronRobinsonMSFT commented May 25, 2021 • edited Loading

AaronRobinsonMSFT commented May 25, 2021

kant2002 commented May 27, 2021

kant2002 commented May 27, 2021

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

AaronRobinsonMSFT May 27, 2021

Choose a reason for hiding this comment

kant2002 commented May 27, 2021 • edited Loading

ReqIpType = ComIpType_Both

ReqIpType = ComIpType_IDispatch

ReqIpType = ComIpType_IUnknown

AaronRobinsonMSFT commented May 27, 2021

kant2002 commented May 28, 2021

AaronRobinsonMSFT commented May 28, 2021

kant2002 commented May 29, 2021

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

AaronRobinsonMSFT commented May 25, 2021 •

edited

Loading

kant2002 commented May 27, 2021 •

edited

Loading