Enable ComWrappers API for cross-platform

[alesomas]

This pull requests contains changes to enable feature_comwrappers on linux, in order to be able to successfully build the clr subset of dotnet runtime. The next step is to fix the test projects in order to be able to try the product changes.

Fixes #36582

[jkoritzinsky]

cc: @AaronRobinsonMSFT @elinor-fung

[AaronRobinsonMSFT]

Some of the build failures are from:

runtime/src/coreclr/vm/gcenv.ee.cpp

Lines 358 to 360 in 685eda6

	#if (defined(FEATURE_COMINTEROP) || defined(FEATURE_COMWRAPPERS)) && defined(FEATURE_OBJCMARSHAL)
	#error COM and Objective-C are not supported at the same time.
	#endif

I think we can just remove the above entirely.

[alesomas]

Some of the build failures are from:

runtime/src/coreclr/vm/gcenv.ee.cpp

Lines 358 to 360 in 685eda6

	#if (defined(FEATURE_COMINTEROP) || defined(FEATURE_COMWRAPPERS)) && defined(FEATURE_OBJCMARSHAL)
	#error COM and Objective-C are not supported at the same time.
	#endif

I think we can just remove the above entirely.

Ok I'll try to remove it even if I think that FEATURE_OBJCMARSHAL should be defined only for CLR_CMAKE_TARGET_OSX OR CLR_CMAKE_TARGET_MACCATALYST OR CLR_CMAKE_TARGET_IOS OR CLR_CMAKE_TARGET_TVOS

[alesomas]

I'm trying to build also WeakReference test prj and after some changes it seems to work; but during the build process there are errors on Managed tests build (see below). Any idea?

/runtime/src/tests/Loader/binding/tracing/BinderTracingTest.targets(50,5): error MSB3677: Unable to move file "/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Loader/binding/tracing/BinderTracingTest.Basic/DependentAssemblies/fr-FR/AssemblyToLoad_Subdirectory.resources.dll" to "/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Loader/binding/tracing/BinderTracingTest.Basic//DependentAssemblies/fr-FR/AssemblyToLoad_Subdirectory.resources.dll". Could not find file '/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Loader/binding/tracing/BinderTracingTest.Basic/DependentAssemblies/fr-FR/AssemblyToLoad_Subdirectory.resources.dll'. [/runtime/src/tests/Loader/binding/tracing/BinderTracingTest.Basic.csproj]
/runtime/src/tests/Loader/binding/tracing/BinderTracingTest.targets(50,5): error MSB3677: Unable to move file "/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Loader/binding/tracing/BinderTracingTest.ResolutionFlow/DependentAssemblies/fr-FR/AssemblyToLoad_Subdirectory.resources.dll" to "/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Loader/binding/tracing/BinderTracingTest.ResolutionFlow//DependentAssemblies/fr-FR/AssemblyToLoad_Subdirectory.resources.dll". Could not find file '/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Loader/binding/tracing/BinderTracingTest.ResolutionFlow/DependentAssemblies/fr-FR/AssemblyToLoad_Subdirectory.resources.dll'. [/runtime/src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.csproj]
/runtime/src/tests/Common/dir.traversal.targets(25,5): error : (No message specified) [/runtime/src/tests/Common/dirs.proj]
/runtime/src/tests/Common/dir.traversal.targets(25,5): error : (No message specified) [/runtime/src/tests/build.proj]

[jkoritzinsky]

@elinor-fung any ideas about the BinderTracingTest build failure?

@alesomas here's a few commands you can work around to managed test build failures in tests other than the ones you are planning to run:

You can run ./src/tests/build.sh skipmanaged to rebuild the native tests and the test layout without doing the managed test build.

You can run ./dotnet.sh msbuild src/tests/Interop/path/to/test.csproj with the path to a test .csproj file to build a single test individually.

As part of each individual test build, the test produces a script to run the test. That script will be located right next to the produced .dll file. You'll need to run it through bash and pass a -coreroot=/path/to/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Tests/Core_Root/ where Linux.x64.Debug is the configuration of the runtime and test build you've done (x64/Debug is the default)

[elinor-fung]

I was able to repro the BinderTracingTest build failure locally - occurs for a non-clean build of the test on Linux. I should have a fix today: #55225. Meanwhile, you should be able to just build/run what you need with @jkoritzinsky's instructions.

[alesomas]

@jkoritzinsky using skipmanaged parameter I'm able to bulild native test. Thanks!!

[alesomas]

@jkoritzinsky @elinor-fung I've pushed also WeakReference test project changes; next step is to try fix checks pipelines with errors? Are you reviewing code changes?

[jkoritzinsky]

Instead of adding all of the COM support directly to xplatform.h, would it be possible to instead extend the ComHelpers.h header to work on both Windows and non-Windows? I see a lot of duplication between the two headers.

[alesomas]

Instead of adding all of the COM support directly to xplatform.h, would it be possible to instead extend the ComHelpers.h header to work on both Windows and non-Windows? I see a lot of duplication between the two headers.

Ok I'll move back the code taken from ComHelpers.h and manage it there with conditional inclusion

[alesomas]

The pipeline check error "CoreCLR GCC Product Build Linux x64" has the following errors in the raw log:

2021-07-16T09:01:28.3833894Z /__w/1/s/src/coreclr/interop/comwrappers.hpp:139:25: error: 'constexpr size_t ManagedObjectWrapperRefCountOffset()' was declared 'extern' and later 'static' [-fpermissive]
2021-07-16T09:01:28.3835083Z 139 | static constexpr size_t ManagedObjectWrapperRefCountOffset()
2021-07-16T09:01:28.3836062Z | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2021-07-16T09:01:28.3837489Z /__w/1/s/src/coreclr/interop/comwrappers.hpp:51:29: note: previous declaration of 'constexpr size_t ManagedObjectWrapperRefCountOffset()'
2021-07-16T09:01:28.3838541Z 51 | friend constexpr size_t ManagedObjectWrapperRefCountOffset();
2021-07-16T09:01:28.3839379Z | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2021-07-16T09:01:28.3840831Z /__w/1/s/src/coreclr/interop/comwrappers.hpp:175:20: error: extra qualification 'NativeObjectWrapperContext::' on member 'Create' [-fpermissive]
2021-07-16T09:01:28.3841886Z 175 | static HRESULT NativeObjectWrapperContext::Create(
2021-07-16T09:01:28.3842574Z | ^~~~~~~~~~~~~~~~~~~~~~~~~~

Could we insert -fpermissive compile option in src/coreclr/interop/CMakesLists.txt to fix errors?

[elinor-fung]

We should be able to just actually fix the errors:

• Add a forward declaration at line 42 / somewhere before the friend declaration:
  static constexpr size_t ManagedObjectWrapperRefCountOffset();
• Remove the extra NativeObjectWrapperContext:: at line 175

[elinor-fung]

You can disable the tests on mono with something like what is in EventListenerThreadPool:

runtime/src/tests/tracing/eventlistener/EventListenerThreadPool.csproj

Line 7 in 95f4fd0

<DisableProjectBuild Condition="'$(RuntimeFlavor)' == 'mono'">true</DisableProjectBuild>

[alesomas]

You can disable the tests on mono with something like what is in EventListenerThreadPool:

runtime/src/tests/tracing/eventlistener/EventListenerThreadPool.csproj

Line 7 in 95f4fd0

<DisableProjectBuild Condition="'$(RuntimeFlavor)' == 'mono'">true</DisableProjectBuild>

It doesn't seem to work

[elinor-fung]

Apparently the Mono test build job in CI builds with RuntimeFlavor=CoreCLR... The Android and WASM legs build tests themselves with RuntimeFlavor=Mono, which is why they started passing after your change, but the other Mono legs that use the artifacts from the Mono test build still failed. Trying to address that in #56434.

Meanwhile, you can add an exclusion in issues.targets under the ItemGroup for '$(RuntimeFlavor)' == 'mono', which should result in the tests projects being built, but not run.

Right below this:

runtime/src/tests/issues.targets

Lines 1183 to 1185 in 2e10087

	<ExcludeList Include="$(XunitTestBinBase)/Interop/COM/Reflection/Reflection/**">
	<Issue>https://github.com/dotnet/runtime/issues/34371</Issue>
	</ExcludeList>

You can add an exclusion for all the ComWrappers tests - something like:

 <ExcludeList Include="$(XunitTestBinBase)/Interop/COM/ComWrappers/**"> 
     <Issue>Not supported on Mono</Issue> 
 </ExcludeList>

[elinor-fung]

I went through the test failure for ValidateAggregationWithReferenceTrackerObject. The issue is that the clean up of the interop sync block was still only under FEATURE_COMINTEROP, resulting in the object not being release. I pushed up a branch (based on your PR) with some changes that I tried locally: elinor-fung@77f4608

While doing that, I also realised that the WeakReferenceTest is still disabled. I tried enabling it. The part of the test that validates using a local (not globally registered) ComWrappers instance passes (after a fix in the test project). The parts trying to use a globally registered instance to re-hydrate after the managed object is collected fail. elinor-fung@01c4a62 enables the test, but for non-Windows, only does validation using a local ComWrappers instance.

Turns out we still have a bunch of functionality around setting/getting COM weak reference info that is just under FEATURE_COMINTEROP. I tried for a bit to enable it - it definitely has more layers than the sync block cleanup - will unpeel for a bit longer.

@jkoritzinsky / @AaronRobinsonMSFT - thoughts on treating the WeakReference as support to be added after this PR (like the global instance for marshalling / Marshal APIs)? You've both dealt with the WeakReference side of this more than me - if we did that, would we be leaving a terrible inconsistency / gap that will bite us?

[elinor-fung]

Okay, this is a quick attempt at getting the COM weak reference info enabled and the test running: elinor-fung@ac73fa9

Didn't try to make it pretty and have only run it against the ComWrappers tests on Linux, but general idea should be there - thoughts, @AaronRobinsonMSFT / @jkoritzinsky?

[alesomas]

After applying @elinor-fung's commits the previous errors are disapperead. Are the new errors due to infrastructure problems?

[AaronRobinsonMSFT]

@elinor-fung Thanks for helping out here! Sad that we missed these FEATURE_COMWRAPPERS locations.

@alesomas I will restart the failing legs. I think these are infrastructure issues. I am going to do another pass looking at @elinor-fung's updates. Looks like we are really close! We should get the issue with the "missing features" filed so we can track that.

[b-iotti]

Hi,

I created issue #56474 to track the features, I might need some help to make sure that all the desired information is in there.

Thank you!

[AaronRobinsonMSFT]

Thanks @b-iotti. I will update as appropriate.

@alesomas This is looking good. I will flip this over to "ready for review".

[AaronRobinsonMSFT]

LGTM. Thanks @alesomas and @b-iotti.

@jkoritzinsky and @elinor-fung, Please give this PR one more look and see if there is anything that is a big concern. I think there are some minor testing clean-up nits that we can handle later.

[elinor-fung]

Looks good - thanks!

No concerns from me. We can do some small clean-up/style things after this goes in.

[jkoritzinsky]

LGTM